Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(prometheus/test): fix test assertion message received from SpinnakerHttpException during upgrade to groovy 3.x #993

Closed
wants to merge 1 commit into from

Conversation

j-sandy
Copy link
Contributor

@j-sandy j-sandy commented Oct 4, 2023

While upgrading groovy 3.0.10 and spockframework 2.0-groovy-3.0, encounter below error in test execution of kayenta-prometheus module:

java.lang.AssertionError:
Expecting throwable message:
  "Status: 500, URL: http://localhost:41703/-/healthy, Message: Internal Server Error"
to contain:
  "500 Internal Server Error"
but did not.

Throwable that failed the check:

com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException: Status: 500, URL: http://localhost:41703/-/healthy, Message: Internal Server Error
	at com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler.handleError(SpinnakerRetrofitErrorHandler.java:55)
	at retrofit.RestAdapter$RestHandler.invoke(RestAdapter.java:242)
	at com.sun.proxy.$Proxy71.isHealthy(Unknown Source)
	at com.netflix.kayenta.prometheus.service.PrometheusRemoteServiceTest.lambda$isHealthyReturnsInternalServerError$0(PrometheusRemoteServiceTest.java:88)
	at org.assertj.core.api.ThrowableAssert.catchThrowable(ThrowableAssert.java:62)
	at org.assertj.core.api.AssertionsForClassTypes.catchThrowable(AssertionsForClassTypes.java:877)
	at org.assertj.core.api.Assertions.catchThrowable(Assertions.java:1306)
	at org.assertj.core.api.Assertions.assertThatThrownBy(Assertions.java:1178)
	at com.netflix.kayenta.prometheus.service.PrometheusRemoteServiceTest.isHealthyReturnsInternalServerError(PrometheusRemoteServiceTest.java:88)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:688)
        ...
Caused by: retrofit.RetrofitError: 500 Internal Server Error
	at retrofit.RetrofitError.httpError(RetrofitError.java:40)
	at retrofit.RestAdapter$RestHandler.invokeRequest(RestAdapter.java:388)
	at retrofit.RestAdapter$RestHandler.invoke(RestAdapter.java:240)
	... 90 more

	at com.netflix.kayenta.prometheus.service.PrometheusRemoteServiceTest.isHealthyReturnsInternalServerError(PrometheusRemoteServiceTest.java:91)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:688)
	...

To fix this issue, edit the assertion message.

…pinnakerHttpException during upgrade to groovy 3.x

While upgrading groovy 3.0.10 and spockframework 2.0-groovy-3.0, encounter below error in test execution of kayenta-prometheus module:

```
java.lang.AssertionError:
Expecting throwable message:
  "Status: 500, URL: http://localhost:41703/-/healthy, Message: Internal Server Error"
to contain:
  "500 Internal Server Error"
but did not.

Throwable that failed the check:

com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException: Status: 500, URL: http://localhost:41703/-/healthy, Message: Internal Server Error
	at com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler.handleError(SpinnakerRetrofitErrorHandler.java:55)
	at retrofit.RestAdapter$RestHandler.invoke(RestAdapter.java:242)
	at com.sun.proxy.$Proxy71.isHealthy(Unknown Source)
	at com.netflix.kayenta.prometheus.service.PrometheusRemoteServiceTest.lambda$isHealthyReturnsInternalServerError$0(PrometheusRemoteServiceTest.java:88)
	at org.assertj.core.api.ThrowableAssert.catchThrowable(ThrowableAssert.java:62)
	at org.assertj.core.api.AssertionsForClassTypes.catchThrowable(AssertionsForClassTypes.java:877)
	at org.assertj.core.api.Assertions.catchThrowable(Assertions.java:1306)
	at org.assertj.core.api.Assertions.assertThatThrownBy(Assertions.java:1178)
	at com.netflix.kayenta.prometheus.service.PrometheusRemoteServiceTest.isHealthyReturnsInternalServerError(PrometheusRemoteServiceTest.java:88)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:688)
        ...
Caused by: retrofit.RetrofitError: 500 Internal Server Error
	at retrofit.RetrofitError.httpError(RetrofitError.java:40)
	at retrofit.RestAdapter$RestHandler.invokeRequest(RestAdapter.java:388)
	at retrofit.RestAdapter$RestHandler.invoke(RestAdapter.java:240)
	... 90 more

	at com.netflix.kayenta.prometheus.service.PrometheusRemoteServiceTest.isHealthyReturnsInternalServerError(PrometheusRemoteServiceTest.java:91)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:688)
	...
```
@@ -87,7 +87,7 @@ public void isHealthyReturnsInternalServerError() {

assertThatThrownBy(() -> prometheusRemoteService.isHealthy())
.isInstanceOf(SpinnakerServerException.class)
.hasMessageContaining("500 Internal Server Error");
.hasMessageContaining("Internal Server Error");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really due to the groovy upgrade? I'd have figured spinnaker/kork#1098 would have broken this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It got caught during groovy upgrade process, I checked in master branch the test is not throwing any error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we need to tag orca on master...Here's what I get from current HEAD of master when I run:

./gradlew kayenta-prometheus:dependencies
+--- io.spinnaker.kork:kork-swagger -> 7.188.0

and from the commit history in kork:

* 536ea404 - (tag: v7.192.0) fix(artifact): Handle exceptions better in artifact store (#1103) (29 hours ago) <xibz>
* 792ada80 - chore(deps): bump actions/checkout from 3 to 4 (#1102) (34 hours ago) <dependabot[bot]>
* 1e8a519e - (tag: v7.191.0) feat(artifacts): Add ArtifactReferenceURI to artifact store (#1101) (7 days ago) <xibz>
* 31c51f6c - (tag: v7.190.0) feat(retrofit): add SpinnakerConversionException for SpinnakerRetrofitErrorHandler to handle kind Retrofit.CONVERSION (#1099) (3 weeks ago) <Sheetal Atre>
* 825c81a1 - (tag: v7.189.0) fix(retrofit): remove duplicate http response code from SpinnakerHttpException message (#1098) (4 weeks ago) <David Byron>
* e1542335 - fix(retrofit): swallow exceptions when SpinnakerHttpException is unable to convert a response body to a map (#1097) (4 weeks ago) <David Byron>
* c63ea930 - test(retrofit): add missing @Test annotation for testSpinnakerHttpExceptionFromRetrofitError (#1096) (6 weeks ago) <David Byron>
* 5b33c24e - (tag: v7.188.0, origin/release-1.32.x, release-1.32.x) fix(retrofit): change retrofit and retrofit2 into api dependencies (#1094) (6 weeks ago) <David Byron>

The current orca version is 8.36.0, which, no surprise uses 7.188.0 of kork.

And yes, it looks like it's been awhile since we tagged orca on master:

* c6a98958a - (HEAD -> master, origin/master, origin/HEAD) chore(dependencies): Autobump korkVersion (#4552) (28 hours ago) <spinnakerbot>
* 02675ef34 - chore(deps): bump actions/checkout from 3 to 4 (#4548) (3 days ago) <dependabot[bot]>
* 185e7084f - chore(deps): bump docker/build-push-action from 4 to 5 (#4547) (3 days ago) <dependabot[bot]>
* daa85ed17 - chore(deps): bump docker/login-action from 2 to 3 (#4551) (3 days ago) <dependabot[bot]>
* 12df8f748 - chore(deps): bump docker/setup-qemu-action from 2 to 3 (#4549) (3 days ago) <dependabot[bot]>
* 7d5ebb4b4 - chore(deps): bump docker/setup-buildx-action from 2 to 3 (#4550) (3 days ago) <dependabot[bot]>
* 3101d8bd8 - refactor(clouddriver/test): refactor Spy.with{} closure during upgrade to groovy 3.x (#4545) (6 days ago) <Sandesh>
* b34be0729 - refactor(core/test): fix cardinality issue by replacing Stub to Mock during upgrade to groovy 3.x (#4544) (7 days ago) <Sandesh>
* d55949b4c - refactor(clouddriver): fix scope resolution of static method explictly calling by class name during upgrade to groovy 3.x (#4543) (7 days ago) <Sandesh>
* b54874e54 - chore(dependencies): Autobump korkVersion (#4542) (7 days ago) <spinnakerbot>
* 8830c6c04 - chore(dependency): add explicit dependency of groovy-json in orca-web and orca-webhook during upgrade of groovy 3.x (#4539) (7 days ago) <Sandesh>
* 778989e70 - refactor(flex/test): fix conflict of variable scope by renaming local variables during upgrade of groovy 3.x (#4541) (8 days ago) <Sandesh>
* bc0e35b8f - refactor(cloudriver): fix enum constructor modifier as private during upgrade to groovy 3.x (#4540) (8 days ago) <Sandesh>
* dd8ddafe2 - fix(execution): do not copy 'alias' from parent context to restrictTimeWindow so that the synthetic task works on aliased stages (#4172) (8 days ago) <Tina>
* 9a1fa511b - fix(vpc): add data annotation to vpc (#4534) (12 days ago) <David Martin>
* 8523343b9 - fix: duplicate entry exception for correlation_ids table. (#4521) (13 days ago) <armory-abedonik>
* 98b814b56 - fix(artifacts): Resolving is broken in multiple places (#4526) (2 weeks ago) <xibz>
* 6ae6207c9 - refactor(retrofit): use SpinnakerRetrofitErrorHandler as a step towards simplifying exception handling (#4529) (2 weeks ago) <Kiran Godishala>
* 6dfe7ddca - feat(clouddriver): use SpinnakerRetrofitErrorHandler for retrofit client beans in CloudDriverConfiguration (#4528) (2 weeks ago) <David Byron>
* 4666ce015 - refactor(bakery/test): wiremock cleanup (#4527) (2 weeks ago) <David Byron>
* ec14ce10c - chore(dependencies): Autobump korkVersion (#4525) (3 weeks ago) <spinnakerbot>
* 83d2d3a6a - feat(retrofit): add SpinnakerServerExceptionHandler (#4524) (3 weeks ago) <David Byron>
* 8050e9edd - TargetServerGroupResolver cleanup (#4523) (3 weeks ago) <David Byron>
* 5ef8a2ee0 - chore(dependencies): Autobump korkVersion (#4518) (4 weeks ago) <spinnakerbot>
* b34b257f9 - fix(redirect): Allow for redirects instead of throwing exception (#4498) (4 weeks ago) <Krystian>
* 27b697eca - fix(orca): tying exceptions to tasks (#4517) (5 weeks ago) <Susmitha>
* 73a1c6f50 - fix(test): fix test placeholders to show unrolled iteration names (#4516) (5 weeks ago) <Sandesh>
* cb269b64c - (tag: v8.36.0, release-1.32.x) chore(dependencies): Autobump fiatVersion (#4515) (5 weeks ago) <spinnakerbot>

Tagging 8.37.0 of orca now.

@dbyron-sf
Copy link
Contributor

included in #994.

@dbyron-sf dbyron-sf closed this Oct 4, 2023
@j-sandy j-sandy deleted the fix-assert-msg branch October 4, 2023 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants