llarsson
commented
5 years ago Perhaps unsurprisingly as there are no changes that should be relevant to change the outcome, the build also fails for the development branch.
Closed llarsson closed 5 years ago
llarsson
commented
5 years ago Perhaps unsurprisingly as there are no changes that should be relevant to change the outcome, the build also fails for the development branch.
SimonEismann
commented
5 years ago Thank you for the very detailed bug report. Could you check if the same error occurs with jdk8? We will definitely look into this.
llarsson
commented
5 years ago Using Java 8 works better (interestingly enough, since the pom file specifies Java 8 to be used?), but generates other errors as below:
Tests run: 5, Failures: 3, Errors: 0, Skipped: 0, Time elapsed: 5.645 sec <<< FAILURE! - in tools.descartes.teastore.registry.RegistryTest
testUnregisterSuccess(tools.descartes.teastore.registry.RegistryTest) Time elapsed: 0.843 sec <<< FAILURE!
java.lang.AssertionError: null
at org.junit.Assert.fail(Assert.java:86)
at org.junit.Assert.assertTrue(Assert.java:41)
at org.junit.Assert.assertTrue(Assert.java:52)
at tools.descartes.teastore.registry.RegistryTest.testUnregisterSuccess(RegistryTest.java:125)
testRegisterFail(tools.descartes.teastore.registry.RegistryTest) Time elapsed: 1.229 sec <<< FAILURE!
java.lang.AssertionError: null
at org.junit.Assert.fail(Assert.java:86)
at org.junit.Assert.assertTrue(Assert.java:41)
at org.junit.Assert.assertTrue(Assert.java:52)
at tools.descartes.teastore.registry.RegistryTest.testRegisterFail(RegistryTest.java:157)
testRegister(tools.descartes.teastore.registry.RegistryTest) Time elapsed: 0.836 sec <<< FAILURE!
java.lang.AssertionError: null
at org.junit.Assert.fail(Assert.java:86)
at org.junit.Assert.assertTrue(Assert.java:41)
at org.junit.Assert.assertTrue(Assert.java:52)
at tools.descartes.teastore.registry.RegistryTest.testRegister(RegistryTest.java:102)
Results :
Failed tests:
RegistryTest.testUnregisterSuccess:125 null
RegistryTest.testRegisterFail:157 null
RegistryTest.testRegister:102 nullI was able to reproduce the exact same test failures with java8. I'll look into a fix for this and why our build server didn't catch this.
llarsson
commented
5 years ago It seems the response codes have changed in your API, because when I add some output to one of the failing tests, I see the reason:
testUnregisterSuccess(tools.descartes.teastore.registry.RegistryTest) Time elapsed: 2.331 sec <<< FAILURE!
java.lang.AssertionError: Status not OK, was 201My simple code to check:
/**
* Test if unregistering a service actually removes it from the registry.
*/
@Test
public void testUnregisterSuccess() {
Response response1 = ClientBuilder.newBuilder().build().target("http://localhost:"
+ getTomcatPort() + "/test/rest/services/service2/abbaasd")
.request(MediaType.APPLICATION_JSON).put(Entity.text(""));
Assert.assertNotNull(response1);
Assert.assertNotNull(response1.getStatus());
Assert.assertNotNull(Response.Status.OK.getStatusCode());
Assert.assertTrue("Status not OK, was " + response1.getStatus(), response1.getStatus() == Response.Status.OK.getStatusCode());...and so on.
llarsson
commented
5 years ago (Obviously a constant should not be able to be null, I just wanted to rule out stupid possibilities here, given that I did not trust my build environment in the least.)
SimonEismann
commented
5 years ago Good work, a quick check reveals that in commit 70f9e98d893ff2709fdb95cc4f56190cbabe7ed1 the corresponding status codes did indeed change to better accomodate the istio/jaeger tracing. It seems this is simply a matter of updating the expected values in the test and then figuring out how our Buildserver did not catch this for 4(!) months...
llarsson
commented
5 years ago Yup. Will this be fixed on development, or master, or... ? :) I am unfamiliar with how this project works, having only cloned it yesterday.
SimonEismann
commented
5 years ago I've merged the changes into the development branch and it will move to master with the next release.
Generally, our procedure is to create bugfix/feature branches from development and merge them back upon completion.
The master branch only contains the official releases/versions to ensure that each version points to exactly one software artifact. So if somebody reports performance results using Teastore 1.3.2. it is clearly defined what this refers to and the results can be replicated.
SimonEismann
commented
5 years ago Thank you for your feedback on this and let us know if you run into any further issues.
On a clean checkout of the
masterbranch, using Ubuntu 16.04 withopenjdk-9-jdkandmaveninstalled from the Ubuntu repositories, a simplemvn clean installfails with the following output regarding the Registry's tests:Attempted mitigation
Adding
javax.annotations/javax.annotation-apias a dependency to the Registry'spom.xmlfile does not alleviate the problem.