RFC: Please critique this working PoC for an AfterDiscoveryCallback.

[smoyer64]

Rationale

JUnit 5 is trying to break the duopoly of Runners and @rules that limits the vintage JUnit solution. While JUnit 5 does eliminate the vintage system's inability to use more than one Runner through the use of Engines, the discovered Engines don't interact with each other. Many types of Extensions enable functionality that can be changed during a test's execution, but there are no extensions that allow a TestDescriptor to be changed before, during or after test discovery.

The changes that provide the AfterDiscoveryCallback allow an Extension to alter the engine's (root) TestDescriptor before it becomes immutable as part of the TestPlan. Classes of Extensions that benefit from this callback include:

• ScenarioExtension: Using the @Scenario({"predecessor1", "predecessor2"}) annotation, tests can be ordered based on a list of predecessor tests (by test method name). The AfterDiscoveryCallback would find all the instances of @Scenario (which extends @test) in a test class and, based on the dependency graph described by the predecessors, would replace the set of child MethodTestDescriptors in the parent ClassTestDescriptor with one of the sets that maintain order. The child @Scenario annotated methods would be reordered to honor the requested dependency graph (assuming it's acyclic).
• JUnitParamsExtension: This vintage JUnit Runner is a popular way of parameterizing tests. There are multiple ways declaratively and programmatically providing a list of parameter sets to a method. The size of the set must match number of parameters in the method declaration, but the size of the list determines how many times that method is executed. In this case, the MethodTestDescriptor would be changed so that it was a test container, and new test methods would be added based on the size of the parameter set list.

If desired, test filtering could be a special case of this callback. Care must be taken to make sure that filter conditions are executed first (since there's no point translating or expanding tests that are destined to be removed).

There are many more examples of these three classes of Extensions (reduce, translate or expand) that want to alter the TestDescriptor hierarchy.

Status

The PoC is available at https://github.com/selesy/junit5/tree/feature/discovery-callbacks. Currently, there's a no-op AfterDiscoveryCallback installed as a default Extension - it doesn't adapt the TestDescriptor in any way but it does print TestDescriptor information to System.out. All the existing unit and platform tests successfully execute (only one test was changed since there's now one more default extension). No tests have been added for this code (so far). While I've done a little clean-up and formatting, I didn't submit a pull request for this PoC since there are known issues.

Next Steps

The next steps for this PoC are:

• Eliminate the default DiscoveryReportExtension.
• Add @Scenario and ScenarioExtension to more appropriately demonstrate how this callback operates.
• Add unit and platform tests.
• Add Javadocs.

Problems

This PoC has the following known problems:

• The signature for the AfterDiscoveryCallback's method is void afterDiscovery(Object testDescriptor) since TestDescriptor and several of it's aggregated types are not available in junit-jupiter-api.

Questions

• Is there are more appropriate way, given the current system's architecture, to accomplish the same functionality?
• What coding standards and conventions have I messed up?
• How is the determination made as to which tests become unit tests in the class' module and which become platform tests?
• What would be an appropriate way to make the afterDiscovery() method type-safe (not expecting Object as a parameter)?

Please feel free to comment and critique as needed - I won't break. And just so I've stated it clearly at least once - thank you so much for all the work you're doing to upgrade JUnit to the current state of the Java art.

[marcphilipp]

Sorry there hasn't been any feedback, yet. We will discuss your proposal in the team when planning for M2.

[smoyer64]

No problem ... I've been busy too. I followed a lot of the conversations that were occurring as you pushed to M1 and certainly appreciate what you've built. Let me know if you want me to join the discussion at some point!

[smoyer64]

I've just merged all changes to JUnit5 origin/master as of 4aa8827 into the AfterDiscoveryCallback PoC.

[smoyer64]

Can this be added to M4 since it's related to the published theme?

[nipafx]

TL:DR; I'm not sure about the extension point but I think it does not reduce the need for more specific ones.

I finally took some time and thought this through. I'm still not sure what my opinion on the extension point per se is, but I did come to the conclusion that I find it is no good replacement for extension points for scenario, parameterized, or dynamic tests. Reasons (all "in my opinion"):

• Usability: Operating on the test descriptor is rather abstract. More focused extension points are hopefully able to deliver APIs that are much more concrete and easier to use.
• Orthogonality: Ideally extension points for test generation/execution schemas (like scenario or parameterized tests) are orthogonal to one another. Solving them all via the same extension point makes it questionable whether that is possible. It seems too easy for extensions to step on each other's toes.
• Order: It is likely that the order in which extensions are applied can become very important. This means that moving an extension from the test class to the method might result in a vastly different test plan.

Note that some of these problems could be resolved if all extensions would be called with the same original test plan and would return additions for it. The original could then be extended by all those additions, which greatly reduces the possibility for extensions to interact (for better or worse).

[smoyer64]

@nicolaiparlog

I agree that there is a great danger of extensions stepping on each other and it's not just confined to Ordered tests and Parameterized tests ... there's clearly a difference between these two categories just when multi-threaded testing is considered. And what happens when you create a test "Scenario" that is also parameterized.

This RFC as submitted as a means of fostering discussion and I don't disagree with any of your feedback. I tend to think that eventually we'll find a scenario where having an immutable test plan isn't the best choice but I also think that dynamic tests provide a clean way around the problem ... it's probably better to have dozens of extension points that each do one thing well than a general one like this that includes the dangers you've described. We don't want this extension to be the siren that causes the developer to navigate onto the rocks.

Also note that I've asked that PR #577 be marked as "don't-merge". I'm happy to see that the team is discussing it as that was my original intent. I will note that one "feature" of this extension point is that it fits into the runner very well ... if adopted there will be far more lines of documentation needed than actual code.

[marcphilipp]

This has been superseded by test templates.