Run tests using JUnit5 platform and remove SpockRunner #770

anuraaga · 2020-07-23T09:10:58Z

I noticed that we're adding junit platform tests in the spring instrumentation and figured it'd be good if we go ahead and upgrade to give us the option of using it. I'm still interested in doing a side-by-side comparison of spock and junit5 just to see :)

This also tries to simplify the tests by removing SpockRunner - only three tests actually depended on its behavior, and it seems OK to let Gradle take care of the classloader isolation instead of using a JUnit runner which constrains options for the future.

iNikem · 2020-07-27T15:58:12Z

@anuraaga what are your plans about this?

anuraaga · 2020-07-27T23:22:56Z

I'm going to examine it more - didn't expect content length and exceptions to eat up my days :P

anuraaga · 2020-07-30T05:54:24Z

I'm going to play a bit more with this, most tests work. But some still have the dreaded "SEVERE: java.lang.LinkageError: loader constraint violation: loader 'bootstrap' wants to load class io.opentelemetry.trace.Span$Kind. A different class with the same name was previously loaded by 'app'. (io.opentelemetry.trace.Span$Kind is in unnamed module of loader 'app')" type of error, so may have to give up. Seems to be a difference in behavior of class loading when using the junit5 runner, even when delegating to vintage.

anuraaga · 2020-08-03T08:52:43Z

testing-common/src/main/groovy/io/opentelemetry/auto/test/base/HttpClientTest.groovy

@@ -42,7 +42,7 @@ import static org.junit.Assume.assumeTrue
 abstract class HttpClientTest extends AgentTestRunner {
  protected static final BODY_METHODS = ["POST", "PUT"]
  protected static final CONNECT_TIMEOUT_MS = 1000
-  protected static final BASIC_AUTH_KEY = "custom authorization header"
+  protected static final BASIC_AUTH_KEY = "custom-authorization-header"


In this PR PlayJavaWEClientTest fails because of invalid header name characters (space) which looks correct to me... The Play tests have a couple other assertions that fail (TimeoutException vs ConnectException) that seem similarly like they should be failing, as if on current master we're not actually running these tests so I need to investigate more what's going on...

as if on current master we're not actually running these tests

oh, wow, they were not, before this PR!!!

cc: @tylerbenson

@anuraaga could you elaborate... which task isn't failing that should be? Any idea why they aren't being executed?

I didn't end up digging on the why, but I think it was this test that started failing with this PR

https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/874b157fe5fdaad80fd82f3ece0600b6860924e7/instrumentation/play-ws/play-ws-2.1/src/test/groovy/PlayWSClientTest.groovy

Netty threw a validation error because of the spaces in this header name which seems right to me.

…-instr-java into junit-platform

trask · 2020-08-04T06:10:34Z

it's gone green!!! 🟢🟩🟢🟩🟢

agent-tooling/src/test/groovy/io/opentelemetry/auto/test/HelperInjectionTest.groovy

…-instr-java into junit-platform

…ockRunner, but Grizzly?

anuraaga · 2020-08-04T07:28:16Z

instrumentation/grizzly-client-1.9/src/test/groovy/GrizzlyAsyncHttpClientTest.groovy

 import spock.lang.AutoCleanup
 import spock.lang.Shared

+@RunWith(SpockRunner)


When I thought I finally had a good mental model of what's going on in our tests, a grizzly shows up 🐻

I think one of the reasons a couple of test failed is because with JUnit5, there is no way to intercept class loading
junit-team/junit5#201

This is a fundamental aspect of JUnit5 so presumably even when using the vintage engine, the limitation applies.

I noticed only two tests in the entire repo actually seem to rely on this behavior though, so I gave them special treatment. At that point, it seemed SpockRunner doesn't actually do anything anymore, so I tried removing it completely. Everything worked, except for this one test.

@trask if you feel like it, maybe you could look at this test without and with SpockRunner and teach me what function it's serving and why it would only affect this test :)

Yeah, that's a tricky one...

There are two things conspiring against that test:

the test code itself gets instrumented, not only the library under test (GrizzlyAsyncHttpClientTest$AsyncCompletionHandlerMock)

the instrumentation that gets injected into that test code uses InstrumentationContext, and tries to put a null value into it

Because AsyncCompletionHandlerMock is a nested class inside of the test, it appears that class is loaded before we install bytebuddy, which means we can only retransform it, which means InstrumentationContext cannot use the (fast) field-backed provider, and instead has to use the WeakMap.

This is fine.

But then comes (2) and tries to put a null value into the WeakMap, which is what then causes the NPE.

Moving AsyncCompletionHandlerMock out into a top-level class resolves the issue.

I'll also open an issue about the WeakMap implementation not supporting null values, though in general we don't use the the WeakMap when running as -javaagent:..., since in that case we are typically able to do initial transform (not structure limited re-transform) and so we can inject a field into the class and use the field-backed provider.

iNikem · 2020-08-05T07:38:30Z

@anuraaga can you please summarise the intent of this PR? I mean, you want to run tests with junit5. Ok, but what tests? All tests? Some tests?

anuraaga · 2020-08-05T07:48:48Z

The intent is to run all the tests with JUnit5 - I think it's more future-proof, including if we want to move to Spock 3 which is based on JUnit5 at some point.

I was hoping the vintage runner would work the exact same way but of course hopes don't always pan out :) I'm not married to this PR, ~~it is a significant regression for assumeTrue to not work correctly so I'll need to follow up with JUnit team to get that fixed during the 5.7 milestones.~~ I do lean towards doing it though to keep up with JUnit. But if we don't merge it it's ok too, during investigating this I learned a lot about the agent :D

iNikem · 2020-08-05T07:59:53Z

I am confused :) What is a relationship between Spock and Junit? You don't propose to remove Spock, do you?

anuraaga · 2020-08-05T08:11:14Z

Nope :)

JUnit is a test runner framework as well as a framework for writing tests
Spock is a framework for writing tests
Sputnik is a JUnit4 custom test runner for running Spock tests (and is the standard way of running them)

So this PR changes from

JUnit4 + Sputnik -> Spock
to
JUnit5 + Vintage Runner + Sputnik -> Spock

And from my reading of Spock 2.0, I think (not sure) sputnik is gone too and it becomes
JUnit5 -> Spock directly (JUnit4 would not be supported)

…-instr-java into junit-platform

iNikem · 2020-08-07T04:50:30Z

gradle/dependencies.gradle

@@ -24,6 +24,7 @@ ext {
    kotlin            : "1.3.72",
    coroutines        : "1.3.0",
    springboot        : "2.3.1.RELEASE",
+    junit5            : "5.7.0-M1",


Do we need a milestone or current stable is enough?

Yeah this milestone fixed a spock issue

iNikem · 2020-08-07T04:50:56Z

gradle/java.gradle

+  testImplementation group: 'org.junit.jupiter', name: 'junit-jupiter-api'
+  testImplementation group: 'org.junit.jupiter', name: 'junit-jupiter-params'
+  testRuntimeOnly group: 'org.junit.jupiter', name: 'junit-jupiter-engine'
+  testRuntimeOnly group: 'org.junit.vintage', name: 'junit-vintage-engine'


Do we need both old and new engines?

We need new engine for any Java tests, we have one PatchLoggerTest. If we get to Spock 2.0 we'll remove vintage at that point.

iNikem · 2020-08-07T04:51:41Z

instrumentation/annotations/annotations.gradle

+  }
+}
+
+// Needs a fresh classloader.


No clue :P Filed an issue

…-instr-java into junit-platform

…y#770) * Run tests with JUnit platform * Matcher * Restore * Don't mix JUnit4 with JUunit5 in spring tests * Separate out tests that need custom class loader. * Separate out test for a couple classes that need it, try to remove SpockRunner, but Grizzly? * Remove SpockRunner * Fix grizzly * Remove assumeTrue workaround. * Comments

anuraaga force-pushed the junit-platform branch from cc5200d to ca6e717 Compare July 23, 2020 09:12

Run tests with JUnit platform

d5549e2

anuraaga force-pushed the junit-platform branch from 2358e49 to d5549e2 Compare August 3, 2020 08:48

Matcher

4a8ba9a

anuraaga commented Aug 3, 2020

View reviewed changes

Anuraag Agrawal added 4 commits August 3, 2020 18:16

Restore

b0cb339

Merge branch 'master' of github.com:open-telemetry/opentelemetry-auto…

c70bd27

…-instr-java into junit-platform

Don't mix JUnit4 with JUunit5 in spring tests

29cb1fc

Merge branch 'spring-no-mix-junit4' into junit-platform

5dc18c1

anuraaga commented Aug 4, 2020

View reviewed changes

agent-tooling/src/test/groovy/io/opentelemetry/auto/test/HelperInjectionTest.groovy Outdated Show resolved Hide resolved

Anuraag Agrawal added 3 commits August 4, 2020 15:20

Merge branch 'master' of github.com:open-telemetry/opentelemetry-auto…

f9521d6

…-instr-java into junit-platform

Separate out tests that need custom class loader.

65e07fc

Separate out test for a couple classes that need it, try to remove Sp…

c6b3540

…ockRunner, but Grizzly?

anuraaga commented Aug 4, 2020

View reviewed changes

Anuraag Agrawal added 2 commits August 5, 2020 13:55

Remove SpockRunner

fcc1d8f

Fix grizzly

9fae9f6

anuraaga marked this pull request as ready for review August 5, 2020 07:46

anuraaga requested review from iNikem, jkwatson and tylerbenson as code owners August 5, 2020 07:46

anuraaga changed the title ~~Run tests using JUnit5 platform~~ Run tests using JUnit5 platform and remove SpockRunner Aug 5, 2020

Merge branch 'master' of github.com:open-telemetry/opentelemetry-auto…

2178f04

…-instr-java into junit-platform

Remove assumeTrue workaround.

85b9d10

trask approved these changes Aug 7, 2020

View reviewed changes

iNikem reviewed Aug 7, 2020

View reviewed changes

Anuraag Agrawal added 3 commits August 7, 2020 13:56

Comments

f02afd2

Merge branch 'master' of github.com:open-telemetry/opentelemetry-auto…

298c7a0

…-instr-java into junit-platform

Merge branch 'master' of github.com:open-telemetry/opentelemetry-auto…

bf2ba66

…-instr-java into junit-platform

anuraaga merged commit 0eafc28 into open-telemetry:master Aug 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run tests using JUnit5 platform and remove SpockRunner #770

Run tests using JUnit5 platform and remove SpockRunner #770

anuraaga commented Jul 23, 2020 •

edited

Loading

iNikem commented Jul 27, 2020

anuraaga commented Jul 27, 2020

anuraaga commented Jul 30, 2020

anuraaga Aug 3, 2020

trask Aug 13, 2020

tylerbenson Aug 13, 2020

anuraaga Aug 13, 2020

trask commented Aug 4, 2020

anuraaga Aug 4, 2020 •

edited

Loading

trask Aug 5, 2020

iNikem commented Aug 5, 2020

anuraaga commented Aug 5, 2020 •

edited

Loading

iNikem commented Aug 5, 2020

anuraaga commented Aug 5, 2020 •

edited

Loading

iNikem Aug 7, 2020

anuraaga Aug 7, 2020

iNikem Aug 7, 2020

anuraaga Aug 7, 2020

iNikem Aug 7, 2020

anuraaga Aug 7, 2020

Run tests using JUnit5 platform and remove SpockRunner #770

Run tests using JUnit5 platform and remove SpockRunner #770

Conversation

anuraaga commented Jul 23, 2020 • edited Loading

iNikem commented Jul 27, 2020

anuraaga commented Jul 27, 2020

anuraaga commented Jul 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trask commented Aug 4, 2020

anuraaga Aug 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iNikem commented Aug 5, 2020

anuraaga commented Aug 5, 2020 • edited Loading

iNikem commented Aug 5, 2020

anuraaga commented Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga commented Jul 23, 2020 •

edited

Loading

anuraaga Aug 4, 2020 •

edited

Loading

anuraaga commented Aug 5, 2020 •

edited

Loading

anuraaga commented Aug 5, 2020 •

edited

Loading