Support bridged methods #647

sormuras · 2017-02-11T21:32:19Z

Overview

In preparation of fixing #333, convert testBridge() junit-team/junit4#1413 test from JUnit 4 and disable it because the current Jupiter implementation produces this call sequence:

() -> assertEquals("static parent.beforeAll()", bridgeMethodNameSequence.get(0)),
() -> assertEquals("parent.beforeEach()", bridgeMethodNameSequence.get(1)),
() -> assertEquals("child.anotherBeforeEach()", bridgeMethodNameSequence.get(2)),
() -> assertEquals("child.test()", bridgeMethodNameSequence.get(3)),
() -> assertEquals("parent.afterEach()", bridgeMethodNameSequence.get(4)),  // should be 5
() -> assertEquals("child.anotherAfterEach()", bridgeMethodNameSequence.get(5)), // should be 4
() -> assertEquals("static parent.afterAll()", bridgeMethodNameSequence.get(6))

instead of the expected:

() -> assertEquals("static parent.beforeAll()", bridgeMethodNameSequence.get(0)),
() -> assertEquals("parent.beforeEach()", bridgeMethodNameSequence.get(1)),
() -> assertEquals("child.anotherBeforeEach()", bridgeMethodNameSequence.get(2)),
() -> assertEquals("child.test()", bridgeMethodNameSequence.get(3)),
() -> assertEquals("child.anotherAfterEach()", bridgeMethodNameSequence.get(4)),
() -> assertEquals("parent.afterEach()", bridgeMethodNameSequence.get(5)),
() -> assertEquals("static parent.afterAll()", bridgeMethodNameSequence.get(6))

Analyze current implementation regarding bridge methods - ignore them in the result set of ReflectionUtils.findAllMethodsInHierarchy(Class<?>, MethodSortOrder)
Ensure the Jupiter engine behaves correctly for bridge methods

I hereby agree to the terms of the JUnit Contributor License Agreement.

Based on "Ignore bridge methods when scanning for annotated methods" [1] from JUnit 4. [1] junit-team/junit4#1413

kcooney · 2017-02-12T16:42:48Z

junit-platform-commons/src/main/java/org/junit/platform/commons/util/ReflectionUtils.java

-		List<Method> localMethods = Arrays.asList(clazz.getDeclaredMethods());
+		// @formatter:off
+		List<Method> localMethods = Arrays.stream(clazz.getDeclaredMethods())
+				.filter(Method::isBridge) // [#333] don't collect bridge methods


Doesn't this make localMethods only include bridge methods (removing the others)?

I don't understand how the existing tests pass with this. Does JUnit 5 have any JUnit4-style self tests?

You are totally right! Look at the low number of tests processed ... and it didn't yield a big fat red light screaming: 🔴 HALT 🔴

Will add a test for this situation and revert to a former version, which I replaced with this inversed logic by error. It used to be:

List<Method> localMethods = Arrays.stream(clazz.getDeclaredMethods()).collect(toList()); localMethods.removeIf(Method::isBridge);

With the wrong predicate, only including bridge methods, the ReflectionUtilsTests#findMethodsInObject test

@Test void findMethodsInObject() { List<Method> methods = ReflectionUtils.findMethods(Object.class, method -> true); assertNotNull(methods); assertTrue(methods.size() > 10); }

reports within IDEA 2016.3.4:

Internal Error occurred. org.junit.platform.commons.util.PreconditionViolationException: Could not find method with name [findMethodsInObject] in class [org.junit.platform.commons.util.ReflectionUtilsTests]. at org.junit.platform.engine.discovery.MethodSelector.lambda$lazyLoadJavaMethod$2(MethodSelector.java:171) at java.util.Optional.orElseThrow(Optional.java:290) at org.junit.platform.engine.discovery.MethodSelector.lazyLoadJavaMethod(MethodSelector.java:169) at org.junit.platform.engine.discovery.MethodSelector.getJavaMethod(MethodSelector.java:136) at org.junit.jupiter.engine.discovery.DiscoverySelectorResolver.lambda$resolveSelectors$3(DiscoverySelectorResolver.java:62) at java.util.ArrayList.forEach(ArrayList.java:1249) at org.junit.jupiter.engine.discovery.DiscoverySelectorResolver.resolveSelectors(DiscoverySelectorResolver.java:61) at org.junit.jupiter.engine.JupiterTestEngine.resolveDiscoveryRequest(JupiterTestEngine.java:68) at org.junit.jupiter.engine.JupiterTestEngine.discover(JupiterTestEngine.java:61) at org.junit.platform.launcher.core.DefaultLauncher.discoverRoot(DefaultLauncher.java:113) at org.junit.platform.launcher.core.DefaultLauncher.discover(DefaultLauncher.java:79) at com.intellij.junit5.JUnit5IdeaTestRunner.startRunnerWithArgs(JUnit5IdeaTestRunner.java:47) at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:51) at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:237) at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at com.intellij.rt.execution.application.AppMain.main(AppMain.java:147)

But the Gradle-based build silently ignores the internal error.

@sormuras I believe you can still use streams here; you just can't use the method reference Method::isBridge as-is anymore, and instead you have to use a lamda expression like method -> !method.isBridge().

Or, even better, create a not(Predicate) method (similar to OpenGamma Strata's Guavate's own not method), put it in org.junit.platform.commons.util.FunctionUtils.java, and pass the Method::isBridge method ref to it like so:

import static org.junit.platform.commons.util.FunctionUtils.not; . . . List<Method> localMethods = Arrays.stream(clazz.getDeclaredMethods()) .filter(not(Method::isBridge)) .collect(toList());

Took the first road: method -> !method.isBridge()

Now, the last test summary reads better:

:platform-tests:junitPlatformTest Test run finished after 3391 ms [ 63 containers found ] [ 0 containers skipped ] [ 63 containers started ] [ 0 containers aborted ] [ 63 containers successful ] [ 0 containers failed ] [ 502 tests found ] [ 0 tests skipped ] [ 502 tests started ] [ 1 tests aborted ] [ 501 tests successful ] [ 0 tests failed ]

Previous iteration:

:platform-tests:junitPlatformTest Test run finished after 78 ms [ 4 containers found ] [ 0 containers skipped ] [ 4 containers started ] [ 0 containers aborted ] [ 4 containers successful ] [ 0 containers failed ] [ 6 tests found ] [ 0 tests skipped ] [ 6 tests started ] [ 0 tests aborted ] [ 6 tests successful ] [ 0 tests failed ] :platform-tests:test SKIPPED :platform-tests:check :platform-tests:build BUILD SUCCESSFUL Total time: 55.391 secs

Both builds were SUCCESSFUL. 😲

@sormuras this is why it's a good practice to see your tests fail first, and then make them pass :-)

They did fail.

The initial commit contained an @Disabled test, a bunch commits later it still failed ... just the last (flawed) polishing and the last green lightbulb saying BUILD SUCCESSFUL convinced me that everything's alright. #648 will be done tonight. Tomorrow, at last.

Never stop learning.

marcphilipp · 2017-02-12T18:12:26Z

Look at the low number of tests processed ... and it didn't yield a big fat red light screaming: 🔴 HALT 🔴

That's what you get occasionally when you test a testing framework with itself. The only way I can think of right now is to add a Gradle-based check for the total number of tests (e.g. total number must be greater than a certain number). However, it's not certain that would catch all such situations. For instance, I remember an occasion where not the number of tests changed but they were always successful.

marcphilipp · 2017-02-12T18:16:41Z

platform-tests/src/test/java/org/junit/platform/commons/util/ReflectionUtilsTests.java

+			() -> assertEquals("parent.afterEach()", bridgeMethodSequence.get(5)),
+			() -> assertEquals("static parent.afterAll()", bridgeMethodSequence.get(6)));
+	}
+


IMHO these tests are fine but in the wrong place. They are integration tests for execution aspects of the Jupiter engine, not unit tests for ReflectionUtils. I think we should change them to directly call ReflectionUtils and move these tests over to junit-jupiter-engine.

The test consists of two parts, namely findMethods and executeClass. The former only invokes ReflectionUtils.findMethods, the later cares about Jupiter does the expected thing.

So,

split the tests into the two aspects (and duplicate classes MethodBridgeParent, MethodBridgeChild and MethodNoBridgeChild or find a common place) or

move the entire tests over to junit-jupiter-engine?

I'd go with (1) and also simplify MethodBridgeParent, MethodBridgeChild and MethodNoBridgeChild in ReflectionUtilsTests to have one method each.

kcooney · 2017-02-12T18:54:05Z

@marcphilipp I suggest having some tests of the JUnit Jupiter framework that are written as JUnit4-style tests that ensure that basic functionality works. Perhaps you could take some of the existing tests and change them to be JUnit4-style. Testing the framework with the framework itself is dangerous, especially if the framework is still under development.

Edit: Alternatively, you could have some integration tests written in bash or python that run a small set of example tests (by spawning a Java process), process the XML output, and ensure that you get the correct number of passing and failing tests.

sormuras · 2017-02-12T18:58:54Z

We could also enhance the standaloneCheck [1] to really execute some tests (not only two empty containers) and insert real assertions there.

[1] https://github.com/junit-team/junit5/blob/master/junit-platform-console/junit-platform-console.gradle#L116

Implemented in #649

kcooney · 2017-02-12T19:06:44Z

I created #648 to track having an integration test to prevent these kinds of regressions.

marcphilipp

👍

Fixes #333 by preventing bridge methods to be included in the result of `ReflectionUtils#findMethod[s]` implementations. This enforces correct execution order of overridden `@BeforeEach`/`@AfterEach` methods when declared at multiple class hierarchy levels. It's now always: `super.before`, `this.before`, `this.test`, `this.after` and `super.after`. See also: junit-team/junit4#1411

This commit contains another test case regarding bridge methods which involves a parameterized base type and testing concrete implementations along with a parameter resolver. See #647 (which contained a public/private bridge test case)

sormuras added 6 commits February 11, 2017 22:13

Test handling of bridge methods

5476990

Based on "Ignore bridge methods when scanning for annotated methods" [1] from JUnit 4. [1] junit-team/junit4#1413

Minor cleanup

b8ae5ea

Add none-bridge method handling test and refactor field name

39502fb

Format

2eac4a4

Move execute logic into own helper method

fdd0748

Filter bridge methods and condensed tests

69022de

sormuras requested a review from marcphilipp February 12, 2017 12:58

sormuras self-assigned this Feb 12, 2017

sormuras added this to the 5.0 M4 milestone Feb 12, 2017

sormuras requested a review from kcooney February 12, 2017 13:12

kcooney reviewed Feb 12, 2017

View reviewed changes

Now filter bridge methods and collect normal ones

9b6e8f2

marcphilipp reviewed Feb 12, 2017

View reviewed changes

kcooney mentioned this pull request Feb 12, 2017

Add integration test to ensure Jupiter engine correctly runs tests #648

Closed

sormuras added 4 commits February 12, 2017 21:10

Copy bridge method tests to own files

a8e8169

Add missing files

18cdf9c

Moved tests to own class

bb0085d

Cleanup ReflectionUtilsTests class

999595e

sormuras mentioned this pull request Feb 12, 2017

Add integration test #649

Merged

marcphilipp approved these changes Feb 13, 2017

View reviewed changes

sormuras merged commit 376f82d into junit-team:master Feb 13, 2017

sormuras deleted the support_bridged_methods branch February 13, 2017 23:24

sormuras added type: bug type: enhancement labels Mar 12, 2017

sormuras added component: Jupiter component: Platform type: task labels Mar 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support bridged methods #647

Support bridged methods #647

sormuras commented Feb 11, 2017 •

edited

kcooney Feb 12, 2017

sormuras Feb 12, 2017

sormuras Feb 12, 2017 •

edited

jbduncan Feb 12, 2017 •

edited

sormuras Feb 12, 2017

sormuras Feb 12, 2017 •

edited

kcooney Feb 12, 2017

sormuras Feb 12, 2017 •

edited

marcphilipp commented Feb 12, 2017

marcphilipp Feb 12, 2017

sormuras Feb 12, 2017

marcphilipp Feb 12, 2017

kcooney commented Feb 12, 2017 •

edited

sormuras commented Feb 12, 2017 •

edited

kcooney commented Feb 12, 2017

marcphilipp left a comment

Support bridged methods #647

Support bridged methods #647

Conversation

sormuras commented Feb 11, 2017 • edited

Overview

kcooney Feb 12, 2017

Choose a reason for hiding this comment

sormuras Feb 12, 2017

Choose a reason for hiding this comment

sormuras Feb 12, 2017 • edited

Choose a reason for hiding this comment

jbduncan Feb 12, 2017 • edited

Choose a reason for hiding this comment

sormuras Feb 12, 2017

Choose a reason for hiding this comment

sormuras Feb 12, 2017 • edited

Choose a reason for hiding this comment

kcooney Feb 12, 2017

Choose a reason for hiding this comment

sormuras Feb 12, 2017 • edited

Choose a reason for hiding this comment

marcphilipp commented Feb 12, 2017

marcphilipp Feb 12, 2017

Choose a reason for hiding this comment

sormuras Feb 12, 2017

Choose a reason for hiding this comment

marcphilipp Feb 12, 2017

Choose a reason for hiding this comment

kcooney commented Feb 12, 2017 • edited

sormuras commented Feb 12, 2017 • edited

kcooney commented Feb 12, 2017

marcphilipp left a comment

Choose a reason for hiding this comment

sormuras commented Feb 11, 2017 •

edited

sormuras Feb 12, 2017 •

edited

jbduncan Feb 12, 2017 •

edited

sormuras Feb 12, 2017 •

edited

sormuras Feb 12, 2017 •

edited

kcooney commented Feb 12, 2017 •

edited

sormuras commented Feb 12, 2017 •

edited