Introduce JUnit Platform Test Kit #1392

dotCipher · 2018-04-25T18:19:22Z

Overview

Addresses issue #1356 by building an ExecutionRecorder that returns an ExecutionsResult from the platform run based on a provided TestEngine. This should allow us to ask both micro and macro questions against the ExecutionsResult for answering questions like:

How many tests were skipped? What were their ids?
How many dynamic tests were registered?
What events happened when?

Replaces #1380

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

Definition of Done

There are no TODOs left in the code
Method preconditions are checked and documented in the method's Javadoc
Coding conventions (e.g. for logging) have been followed
Change is covered by automated tests
Public API has Javadoc and @API annotations
Change is documented in the User Guide and Release Notes
All continuous integration builds pass

Pull from head of fork

dotCipher · 2018-04-30T22:37:00Z

@sbrannen / @sormuras most of the high level feature work is done, but I am slowly going through the DoD checklist above. Would love some input as to overall direction / guidance to make sure I am in line with what you guys would like.

Question though, I am still running into the issue of NoClassDefFoundError but can't seem to find what I am missing (do I need to add a dep somewhere? Here is the stacktrace:

> Task :junit-jupiter-engine:junitPlatformTest
15:33:20.395 [main] WARN  org.junit.platform.launcher.core.DefaultLauncher - TestEngine with ID 'junit-jupiter' failed to discover tests
java.lang.NoClassDefFoundError: org/junit/platform/test/ExecutionRecorder
        at java.lang.Class.getDeclaredMethods0(Native Method) ~[?:?]
        at java.lang.Class.privateGetDeclaredMethods(Class.java:3139) ~[?:?]
        at java.lang.Class.getDeclaredMethods(Class.java:2266) ~[?:?]
        at org.junit.platform.commons.util.ReflectionUtils.getDeclaredMethods(ReflectionUtils.java:1120) ~[junit-platform-commons-1.3.0-SNAPSHOT.jar:1.3.0-SNAPSHOT]
        [...]
        at org.junit.platform.console.ConsoleLauncher.main(ConsoleLauncher.java:39) [junit-platform-console-1.3.0-SNAPSHOT.jar:1.3.0-SNAPSHOT]
Caused by: java.lang.ClassNotFoundException: org.junit.platform.test.ExecutionRecorder
        at jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:582) ~[?:?]
        at jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:185) ~[?:?]
        at java.lang.ClassLoader.loadClass(ClassLoader.java:496) ~[?:?]
        ... 31 more

> Task :junit-jupiter-params:junitPlatformTest FAILED

Failures (1):
  JUnit Jupiter:AggregatorIntegrationTests:reportsExceptionForErroneousAggregator()
    MethodSource [className = 'org.junit.jupiter.params.aggregator.AggregatorIntegrationTests', methodName = 'reportsExceptionForErroneousAggregator', methodParameterTypes = '']
    => java.lang.NoClassDefFoundError: org/junit/platform/test/ExecutionRecorder
       org.junit.jupiter.params.aggregator.AggregatorIntegrationTests.execute(AggregatorIntegrationTests.java:198)
       org.junit.jupiter.params.aggregator.AggregatorIntegrationTests.reportsExceptionForErroneousAggreg
      [...]
       org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:115)
       org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeTestMethod$6(TestMethodTestDescriptor.java:170)
       org.junit.jupiter.engine.execution.ThrowableCollector.execute(ThrowableCollector.java:40)
       [...]

Also visible as a build failure here: https://travis-ci.org/junit-team/junit5/jobs/373259220

marcphilipp

The build fails because the jar task of junit-platform-test is disabled. That's because it's not in mavenizedProjects in the root build.gradle. Adding it there causes some other problems that I'll attempt to solve in master before we merge this PR. For now, manually enabling it in junit-platform-test.gradle via jar.enabled = true should suffice.

marcphilipp · 2018-05-01T13:17:27Z

junit-platform-test/src/main/java/org/junit/platform/test/ExecutionGraph.java

+				ExecutionEvent.byTestDescriptor(notNull(predicate, "TestDescriptor Predicate cannot be null"))));
+	}
+
+	public static class Builder {


Is any of the Builder's methods besides addEvent(event) ever called?

Nope, they aren't, I'll delete for now.

marcphilipp · 2018-05-01T13:19:13Z

junit-platform-test/src/main/java/org/junit/platform/test/ExecutionGraph.java

+ * Represents the entirety of multiple test or container execution runs.
+ */
+@API(status = API.Status.EXPERIMENTAL, since = "1.2.0")
+public class ExecutionGraph {


I fail to see how the list of events represents a graph. Am I missing something? 🤔

Yeah you're right. The intention here originally was to build hierarchy for the resulting Executions, but the name was mainly a placeholder for that. I can revert to something like ExecutionsResult for now, then if I follow up with adding hierarchy later if that's a blocker for me.

marcphilipp · 2018-05-01T13:24:11Z

junit-platform-test/src/main/java/org/junit/platform/test/ExecutionEvent.java

+	private Type type;
+	private TestDescriptor testDescriptor;
+	private Object payload;
+	private LocalDateTime dateTimeOccurred;


I think Instant would be more appropriate.

Fair enough, will switch.

marcphilipp · 2018-05-01T13:25:34Z

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/JavaElementsResolver.java

@@ -140,7 +140,7 @@ else if (numSegmentsResolved != numSegmentsToResolve) {
 					List<Segment> unresolved = segments.subList(1, segments.size()); // Remove engine ID
 					unresolved = unresolved.subList(numSegmentsResolved, unresolved.size()); // Remove resolved segments
 					return format("Unique ID '%s' could only be partially resolved. "
-							+ "All resolved segments will be executed; however, the "
+							+ "All resolved segments will be finished; however, the "


Did you change this because they might get skipped?

Ah sorry, this might have actually been a find/replace typo. Will revert

marcphilipp · 2018-05-01T17:31:48Z

In the meantime, I have updated master to make the baseline configuration optional. Please rebase and add junit-platform-test to mavenizedProjects.

codecov · 2018-05-24T17:00:13Z

Codecov Report

Merging #1392 into master will decrease coverage by 0.09%.
The diff coverage is 87.36%.

@@             Coverage Diff             @@
##             master    #1392     +/-   ##
===========================================
- Coverage     91.91%   91.82%   -0.1%     
- Complexity     3490     3618    +128     
===========================================
  Files           321      331     +10     
  Lines          8329     8567    +238     
  Branches        722      727      +5     
===========================================
+ Hits           7656     7867    +211     
- Misses          501      525     +24     
- Partials        172      175      +3

Impacted Files	Coverage Δ	Complexity Δ
...ava/org/junit/platform/testkit/TestEngineStub.java	`75% <ø> (ø)`	`3 <0> (?)`
...latform/testkit/TestExecutionResultConditions.java	`77.77% <ø> (ø)`	`8 <0> (?)`
...java/org/junit/platform/testkit/TestEngineSpy.java	`100% <ø> (ø)`	`5 <0> (?)`
...org/junit/platform/testkit/TestDescriptorStub.java	`100% <ø> (ø)`	`3 <0> (?)`
.../org/junit/platform/testkit/ExecutionRecorder.java	`100% <100%> (ø)`	`9 <9> (?)`
...nit/platform/testkit/ExecutionEventConditions.java	`100% <100%> (ø)`	`29 <11> (?)`
...ain/java/org/junit/platform/testkit/Execution.java	`71.42% <71.42%> (ø)`	`4 <4> (?)`
...ava/org/junit/platform/testkit/ExecutionEvent.java	`83.33% <83.33%> (ø)`	`16 <16> (?)`
...a/org/junit/platform/testkit/ExecutionsResult.java	`85.71% <85.71%> (ø)`	`44 <44> (?)`
...va/org/junit/platform/testkit/TerminationInfo.java	`91.66% <91.66%> (ø)`	`7 <7> (?)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb6e1cb...ce61727. Read the comment docs.

dotCipher · 2018-05-24T20:51:00Z

This is ready for another round of CR if anyone has a chance! Appreciate the initial help @marcphilipp

dotCipher · 2018-05-24T20:52:34Z

Side question, should I add details about usage of this to the user guide in this same PR?

marcphilipp · 2018-05-25T09:59:22Z

Can you please check why generating Javadoc fails? You can run ./gradlew aggregateJavadocs locally.

Side question, should I add details about usage of this to the user guide in this same PR?

Ultimately, yes. But let us review first, so you won't have to rework the documentation should we see the need for additional changes.

dotCipher · 2018-05-26T02:36:39Z

Should be all set now with the fix, let me know if I should make further revisions. I am really excited to give back to the junit5 codebase, it's been a true pleasure working with it.

dotCipher · 2018-06-05T17:24:55Z

@sormuras / @marcphilipp does anyone have some bandwidth to help CR this and push it through? Thanks!

dotCipher · 2018-06-11T20:14:45Z

Any updates on getting this through?

sormuras · 2018-06-12T06:49:08Z

I'll test-drive and review this PR next week.

sormuras · 2018-06-12T11:11:07Z

This branch is 18 commits ahead, 56 commits behind junit-team:master.

Can you please rebase onto current junit-team:master. Thanks.

dotCipher · 2018-06-13T17:35:52Z

Should be all good to go for when you review. I will document usage in this PR if the current implementation looks good :)

sormuras · 2018-06-13T17:44:58Z

Thanks for restoring the ✔️

`testkit`

We decided to go with the directory name junit-platform-testkit and the related org.junit.platform.testkit base package and module name. Having a published package name ending with just test is too generic/errorprone for a testing framework/platform. Can you please rename the according files and folders?

Overall the PR looks good to me, diving deeper into it next week as promised.

junit-vintage-engine/junit-vintage-engine.gradle

sormuras · 2018-06-23T07:08:36Z

It's green again! 🙂

In general, I'd like to see more "package-only" refactorings. Like only replacing engine.test with testkit to keep the scope of the PR concise.

For example TestIdentifierTests.java with

import org.junit.platform.engine.test.TestDescriptorStub;

turning into

import org.junit.platform.testkit.TestDescriptorStub;

Changing a class name here and there, like you did with ExecutionEventRecorder ending up as ExecutionsResult is okay and easy to digest. Changing the names and meaning of methods within the new type are two steps in one. Please keep the old API intact. For example, getTestStartedCount() should still exist in the new type. Its implementation can of course use the new functionality.

Can you tell me more about why you replaced "number of tests started" with "number of tests finished"?

Coming from

assertEquals(1, eventRecorder.getTestStartedCount(), "# tests started");

and reading now

assertEquals(1, executionsResult.getExecutionsFinished().size(), "# tests started");

doesn't seem consistent for me. I expected to read:

assertEquals(1, executionsResult.getTestStartedCount(), "# tests started");

dotCipher · 2018-06-25T17:54:17Z

@sormuras,

Thanks for the CR, that all makes sense and apologies for combining a few steps in one here. Most of the refactorings of class names stemmed from having something short and memorable.

I replaced getTestStartedCount() with getExecutionsFinished().size() since I try to encapsulate the act of a test being ran within an Execution irregardless of whether or not said test failed, or completed successfully. The way I saw it was that a Test Execution was essentially logically identical to a Test started, because in order for an Execution to exist, a Test would needed to have been started. The only case where this wouldn't be true when calling getExecutionsFinished() would be when a Test Execution was skipped.

With that said, do you think it would be better to replace the call with getExecutions()? Since that's the super set of both Executions that were skipped, failed, or completed, and as such, is logically equivalent to Tests that were STARTED?

Or I could just rename getTests() to getTestsStarted() since those are equivalent?

dotCipher · 2018-06-25T17:58:22Z

Or rather, it might be better if I switch any getExecutions*() method to getTest*() and just return an Execution, to keep the API consistent as you mentioned?

I just wanted it to be clearly distinct from getTestEvents*(), since it's completely different. I am open to make any changes needed though.

I mainly removed the *Count() methods since they seemed a bit unnecessary with the call to .size() of the returned List and polluted the API since it was originally marked as EXPERIMENTAL, and I wanted to keep it as concise as possible. But I can add them back for now and we can go from there.

Thanks again for the feedback :)

dotCipher · 2018-07-03T18:50:29Z

I swear it was green a couple days ago 😢 I will take another look at it and see what the problem is. I might rebase off of master again.

sbrannen · 2018-08-14T10:09:34Z

Hi @dotCipher,

Thanks for getting things cleaned up.

Since we just released 5.3 RC1 last night, this has been pushed to 5.4 M1.

Thus, we won't be merging it until after 5.3 GA has been released, and we may not get a chance to review it until then either.

Just FYI...

dotCipher · 2018-08-14T22:58:31Z

@sbrannen ,

Ok, that makes sense, I'll stay tuned until then.

marcphilipp · 2018-09-14T10:56:26Z

@dotCipher Now that 5.3 and 5.3.1 have been released, could you please rebase your branch on master?

dotCipher · 2018-09-14T23:19:44Z

Sure thing, I'll have up-to-date by EoD Monday (Sept 17th). Thanks!

sbrannen · 2018-09-15T10:38:03Z

Sure thing, I'll have up-to-date by EoD Monday (Sept 17th).

Looking forward to it. 👍

…it-platform-testing

dotCipher · 2018-09-18T02:52:43Z

Green build and up-to-date now, let me know if I should start on the User Guide / Release Notes. 👍

sormuras · 2018-09-22T06:47:32Z

Green is always good! ✔️

My plan is to merge this into master "as-is", squashed into a single commit, apply some polishing and create an issue for the documentation part.

sormuras

Found one minor nit: please replace all 1.3.0 versions with 1.4.0

And a major one: the junit-platform-testkit is intended to be used in test scope. Thus all types of its API must reside under src/main/java.

sormuras · 2018-09-22T07:18:01Z

junit-vintage-engine/junit-vintage-engine.gradle

@@ -10,7 +10,8 @@ dependencies {
 	testImplementation(project(':junit-jupiter-api'))
 	testImplementation(project(':junit-platform-runner'))
 	testImplementation(project(path: ':junit-jupiter-engine', configuration: 'testArtifacts'))
-	testImplementation(project(path: ':junit-platform-engine', configuration: 'testArtifacts'))
+	testImplementation(project(':junit-platform-testkit'))
+	testImplementation(project(path: ':junit-platform-testkit', configuration: 'testArtifacts'))


Mh, I'd prefer to have all types of the Platform TestKit in src/man/java. Thus rendering this extra configuration not needed.

Please move all types from https://github.com/dotCipher/junit5/tree/feature/junit-platform-testing/junit-platform-testkit/src/test/java/org/junit/platform/testkit to https://github.com/dotCipher/junit5/tree/feature/junit-platform-testing/junit-platform-testkit/src/main/java/org/junit/platform/testkit -- in short, from test to main. Unless a type is a unit test for a testkit class.

What to do with ExecutionEventConditions and TestExecutionResultConditions and their dependency on AssertJ?

Not sure, yet. Perhaps leave them under src/test/java for now...

...or let users of the JUnit Platform TestKit transitively depend on AssertJ. Would be fine with me.

What do you think, @junit-team/junit-lambda ?

I think that's ok in this case.

Ok, so to be clear, I will move everything under test to main, including ExecutionEventConditions and TestExecutionResultConditions.

britter · 2018-10-01T15:00:58Z

Hello! I'm currently working on several extensions (@DefaultLocale and @DefaultTimeZone and @Testcontainers) and I've thought about how an API for the testkit could look like. I have implemented a proposal for an API that is based on annotations, rather than inheritance. Furthermore it just exposes the functionality to users that is relevant to them. I'd like to invite everybody to discuss my proposal and maybe we can combine some of the stuff that has already implemented here and some of the things I'm proposing to get the best out of the testkit :-)

sormuras · 2018-10-05T10:51:11Z

Team Decision: Merge this PR. Then it can be refactored and improved to support common use-cases.

…orm-testing

sormuras · 2018-10-05T12:16:22Z

Thanks for all the work on this, @dotCipher 👍

dotCipher · 2018-10-05T22:14:44Z

Thanks @sormuras, I'll start on writing up a new doc for the release notes, I submitted #1621 for tracking.

@britter , thanks for the input! I will take a look on your PR that you linked.

Merge pull request #1 from junit-team/master

47e4272

Pull from head of fork

dotCipher mentioned this pull request Apr 25, 2018

WIP Platform Technology Compatibility Kit #1380

Closed

7 tasks

dotCipher changed the title ~~Platform / Engine meta-testing~~ WIP Platform / Engine meta-testing Apr 25, 2018

marcphilipp reviewed May 1, 2018

View reviewed changes

dotCipher changed the title ~~WIP Platform / Engine meta-testing~~ Platform / Engine meta-testing May 24, 2018

dotCipher mentioned this pull request May 24, 2018

Introduce TestWatcher extension API for processing test execution results #542

Closed

sormuras self-assigned this Jun 12, 2018

sormuras added this to the 5.3 Backlog milestone Jun 12, 2018

sormuras added type: enhancement theme: extensions component: Jupiter component: Platform labels Jun 12, 2018

sormuras reviewed Jun 18, 2018

View reviewed changes

junit-vintage-engine/junit-vintage-engine.gradle Outdated Show resolved Hide resolved

dotCipher force-pushed the feature/junit-platform-testing branch 2 times, most recently from fc41f0a to 1f469ce Compare September 18, 2018 02:10

Cody Moore added 2 commits September 17, 2018 19:15

Merge branch 'master' of github.com:dotCipher/junit5 into feature/jun…

6abc0a1

…it-platform-testing

Updated from master with testkit changes

102b970

sormuras approved these changes Sep 22, 2018

View reviewed changes

sormuras changed the title ~~Platform / Engine meta-testing~~ [junit-platform-testkit] Platform and Engine meta-testing Sep 22, 2018

sormuras requested changes Sep 22, 2018

View reviewed changes

Cody Moore added 2 commits September 24, 2018 11:47

Merge all classes into main testkit package + bump API version to 1.4.0

50b1240

Fixed tests from CR changes

e8f7ba9

sormuras approved these changes Sep 25, 2018

View reviewed changes

Merge remote-tracking branch 'origin/master' into feature/junit-platf…

ce61727

…orm-testing

ghost assigned marcphilipp Oct 5, 2018

ghost added the status: in progress label Oct 5, 2018

sormuras merged commit e216de1 into junit-team:master Oct 5, 2018

ghost removed the status: in progress label Oct 5, 2018

dotCipher mentioned this pull request Oct 5, 2018

Document Test Kit API #1621

Closed

6 tasks

sbrannen changed the title ~~[junit-platform-testkit] Platform and Engine meta-testing~~ Introduce JUnit Platform Test Kit Nov 3, 2018

sbrannen added the component: Test Kit label Nov 3, 2018

sbrannen unassigned marcphilipp Nov 3, 2018

Introduce JUnit Platform Test Kit #1392

Introduce JUnit Platform Test Kit #1392

Conversation

dotCipher commented Apr 25, 2018 • edited Loading

Overview

Definition of Done

dotCipher commented Apr 30, 2018 • edited Loading

marcphilipp left a 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

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

marcphilipp commented May 1, 2018

codecov bot commented May 24, 2018 • edited Loading

Codecov Report

dotCipher commented May 24, 2018

dotCipher commented May 24, 2018 • edited Loading

marcphilipp commented May 25, 2018

dotCipher commented May 26, 2018

dotCipher commented Jun 5, 2018

dotCipher commented Jun 11, 2018

sormuras commented Jun 12, 2018

sormuras commented Jun 12, 2018

dotCipher commented Jun 13, 2018

sormuras commented Jun 13, 2018 • edited Loading

testkit

sormuras commented Jun 23, 2018 • edited Loading

dotCipher commented Jun 25, 2018 • edited Loading

dotCipher commented Jun 25, 2018 • edited Loading

dotCipher commented Jul 3, 2018 • edited Loading

sbrannen commented Aug 14, 2018

dotCipher commented Aug 14, 2018

marcphilipp commented Sep 14, 2018

dotCipher commented Sep 14, 2018

sbrannen commented Sep 15, 2018

dotCipher commented Sep 18, 2018

sormuras commented Sep 22, 2018 • edited Loading

sormuras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sormuras Sep 22, 2018 • 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

britter commented Oct 1, 2018 • edited Loading

sormuras commented Oct 5, 2018

sormuras commented Oct 5, 2018

dotCipher commented Oct 5, 2018

dotCipher commented Apr 25, 2018 •

edited

Loading

dotCipher commented Apr 30, 2018 •

edited

Loading

codecov bot commented May 24, 2018 •

edited

Loading

dotCipher commented May 24, 2018 •

edited

Loading

sormuras commented Jun 13, 2018 •

edited

Loading

`testkit`

sormuras commented Jun 23, 2018 •

edited

Loading

dotCipher commented Jun 25, 2018 •

edited

Loading

dotCipher commented Jun 25, 2018 •

edited

Loading

dotCipher commented Jul 3, 2018 •

edited

Loading

sormuras commented Sep 22, 2018 •

edited

Loading

sormuras Sep 22, 2018 •

edited

Loading

britter commented Oct 1, 2018 •

edited

Loading