Revisit extension registration model

[sbrannen]

Status Quo

The extension registration model is currently a work in progress and needs to be revisited to make sure it aligns with the needs of the extension authors and the community at large.

Topics to Review and Revisit

• TestExtension vs. ExtensionPoint
  • What purpose does TestExtension serve since the introduction of ExtensionPoint? Do we need two maker interfaces for extensions?
• ExtensionPoint vs RegisteredExtensionPoint
• ExtensionPointSorter
  • Why limit sorting to implementations of ExtensionPoint? Why not allow any type of TestExtension to be sorted?
• ExtensionRegistry vs. TestExtensionRegistry
  • At the very least, the names are both misleading and confusing.
  • An ExtensionRegistry currently allows one to register only instances of ExtensionPoint. So why isn't it called an ExtensionPointRegistry?
• ExtensionPoint.Position
  • Why is Position specific to ExtensionPoint?
  • An extension author would likely never want an extension to be invoked "in the middle" (i.e., in the DEFAULT position) alongside user code. So what purpose does DEFAULT serve?
  • Position defines only 5 options for extension authors. Why should JUnit be so restrictive? Why not allow any numerical ordering (e.g., the entire range of integers, perhaps other than 0) beyond the hard coded 5 current values?
  • Consider a solution such as @Order that can be reused across multiple use cases within the framework.
• Verify (via appropriate testing) that the current algorithm properly sorts extensions when multiple extensions exist within test class hierarchies and within a given extension execution context.
• The current extension model allows additional extensions to be registered programmatically via the ExtensionRegistry; however, there is no mechanism for overriding extensions that have already been registered (e.g., in superclasses).
  • We need a way to override extensions that have been registered in an enclosing context.

[jlink]

Some team decisions:

• Rename TestExtension to Extension.
• Rename ExtensionRegistry to ExtensionPointRegistry.
• ExtensionRegistrar must implement Extension instead of ExtensionPoint.
• Move Position to ExtensionPointRegistry.

[jlink]

A thing we talked about but did not decide on:

• Reintroduce extension name into extension registration - and RegisteredExtensionPoint to allow for better logging and reporting.

[sbrannen]

Reintroduce extension name into extension registration - and RegisteredExtensionPoint to allow for better logging and reporting.

Do you mean a custom, user-provided name?

Or do you mean a name that we infer from the source of the extension? (and by "source" I mean the class implementing ExtensionRegistrar)

[jlink]

Mostly the latter. But it should not work only for registrars but also for registering annotated methods (BeforeEach etc). See ClassTestDescriptor.

Von meinem iPad gesendet

Am 16.01.2016 um 23:55 schrieb Sam Brannen notifications@github.com:

Reintroduce extension name into extension registration - and RegisteredExtensionPoint to allow for better logging and reporting.

Do you mean a custom, user-provided name?

Or do you mean a name that we infer from the source of the extension? (and by "source" I mean the class implementing ExtensionRegistrar)

—
Reply to this email directly or view it on GitHub.

[sbrannen]

FYI: (from commit 8a11695)

In order to improve error reporting and logging, a new source property has been introduced in RegisteredExtensionPoint.

If an extension point is registered declaratively via @ExtendWith, the extensionPoint and source properties will contain a reference to the same object; however, if an extension point is registered programmatically -- for example, by an ExtensionRegistrar as a lambda expression or by the framework via the TestExtensionRegistry -- the source may be the ExtensionRegistrar that registered the extension point, the underlying method that implements the extension point API, or similar.

The source is currently used for error reporting in ExtensionPointSorter when conflicting extensions are encountered. In addition, the source is logged under the org.junit.gen5.engine.junit5.execution category at the TRACE log level whenever an extension is registered.

[sbrannen]

I did not reintroduce the extension name, since it is not always clear what the name should be. I instead opted to track the source as described above.

If you execute AllFastJUnit5Tests with the org.junit log level set to TRACE (in log4j2-test.xml), that will give you a good idea of what kind of information is now available in the source property.

Here's some sample output:

Registering extension point [org.junit.gen5.engine.junit5.extension.DisabledCondition@15b204a1] from source [org.junit.gen5.engine.junit5.extension.DisabledCondition@15b204a1] with position [DEFAULT].
Registering extension point [org.junit.gen5.engine.junit5.extension.TestInfoParameterResolver@77167fb7] from source [org.junit.gen5.engine.junit5.extension.TestInfoParameterResolver@77167fb7] with position [DEFAULT].
Registering extension point [org.junit.gen5.engine.junit5.extension.TestReporterParameterResolver@1fe20588] from source [org.junit.gen5.engine.junit5.extension.TestReporterParameterResolver@1fe20588] with position [DEFAULT].
Registering extension point [org.junit.gen5.engine.junit5.descriptor.ClassTestDescriptor$$Lambda$159/1704629915@76908cc0] from source [public void org.junit.gen5.commons.util.ClasspathScannerTests.init()] with position [DEFAULT].
Registering extension point [org.junit.gen5.engine.junit5.descriptor.ClassTestDescriptor$$Lambda$159/1704629915@3f4faf53] from source [public void org.junit.gen5.engine.AbstractTestDescriptorTests.initTree()] with position [DEFAULT].
Registering extension point [org.junit.gen5.engine.junit5.descriptor.ClassTestDescriptor$$Lambda$159/1704629915@3feb2dda] from source [public void org.junit.gen5.engine.HierarchicalTestExecutorTests.init()] with position [DEFAULT].
Registering extension point [org.junit.gen5.engine.junit5.descriptor.ClassTestDescriptor$$Lambda$159/1704629915@363a52f] from source [void org.junit.gen5.engine.junit5.AbstractJUnit5TestEngineTests.initListeners()] with position [DEFAULT].
Registering extension point [org.junit.gen5.engine.junit5.extension.DisabledCondition@66c61024] from source [org.junit.gen5.engine.junit5.extension.DisabledCondition@66c61024] with position [DEFAULT].
Registering extension point [org.junit.gen5.engine.junit5.ClassLevelCallbackTests$InnermostAndOutermost$$Lambda$397/893591815@7b420819] from source [org.junit.gen5.engine.junit5.ClassLevelCallbackTests$InnermostAndOutermost@7a1a14a4] with position [INNERMOST].
Registering extension point [org.junit.gen5.engine.junit5.ClassLevelCallbackTests$InnermostAndOutermost$$Lambda$398/643489709@a3d9978] from source [org.junit.gen5.engine.junit5.ClassLevelCallbackTests$InnermostAndOutermost@7a1a14a4] with position [INNERMOST].
Registering extension point [org.junit.gen5.engine.junit5.ClassLevelCallbackTests$InnermostAndOutermost$$Lambda$399/1632914150@4b41dd5c] from source [org.junit.gen5.engine.junit5.ClassLevelCallbackTests$InnermostAndOutermost@7a1a14a4] with position [OUTERMOST].
Registering extension point [org.junit.gen5.engine.junit5.ClassLevelCallbackTests$InnermostAndOutermost$$Lambda$400/999736366@5d066c7d] from source [org.junit.gen5.engine.junit5.ClassLevelCallbackTests$InnermostAndOutermost@7a1a14a4] with position [OUTERMOST].

[jlink]

"Source" sounds more intention revealing to me. Thanks.

Am 17.01.2016 um 23:03 schrieb Sam Brannen notifications@github.com:

I did not reintroduce the extension name, since it is not always clear what the name should be. I instead opted to track the source as described above.

If you execute AllFastJUnit5Tests with the org.junit log level set to TRACE (in log4j2-test.xml), that will give you a good idea of what kind of information is now available in the source property.

Here's some sample output:

Registering extension point [org.junit.gen5.engine.junit5.extension.DisabledCondition@15b204a1] from source [org.junit.gen5.engine.junit5.extension.DisabledCondition@15b204a1] with position [DEFAULT].
Registering extension point [org.junit.gen5.engine.junit5.extension.TestInfoParameterResolver@77167fb7] from source [org.junit.gen5.engine.junit5.extension.TestInfoParameterResolver@77167fb7] with position [DEFAULT].
Registering extension point [org.junit.gen5.engine.junit5.extension.TestReporterParameterResolver@1fe20588] from source [org.junit.gen5.engine.junit5.extension.TestReporterParameterResolver@1fe20588] with position [DEFAULT].
Registering extension point [org.junit.gen5.engine.junit5.descriptor.ClassTestDescriptor$$Lambda$159/1704629915@76908cc0] from source [public void org.junit.gen5.commons.util.ClasspathScannerTests.init()] with position [DEFAULT].
Registering extension point [org.junit.gen5.engine.junit5.descriptor.ClassTestDescriptor$$Lambda$159/1704629915@3f4faf53] from source [public void org.junit.gen5.engine.AbstractTestDescriptorTests.initTree()] with position [DEFAULT].
Registering extension point [org.junit.gen5.engine.junit5.descriptor.ClassTestDescriptor$$Lambda$159/1704629915@3feb2dda] from source [public void org.junit.gen5.engine.HierarchicalTestExecutorTests.init()] with position [DEFAULT].
Registering extension point [org.junit.gen5.engine.junit5.descriptor.ClassTestDescriptor$$Lambda$159/1704629915@363a52f] from source [void org.junit.gen5.engine.junit5.AbstractJUnit5TestEngineTests.initListeners()] with position [DEFAULT].
Registering extension point [org.junit.gen5.engine.junit5.extension.DisabledCondition@66c61024] from source [org.junit.gen5.engine.junit5.extension.DisabledCondition@66c61024] with position [DEFAULT].
Registering extension point [org.junit.gen5.engine.junit5.ClassLevelCallbackTests$InnermostAndOutermost$$Lambda$397/893591815@7b420819] from source [org.junit.gen5.engine.junit5.ClassLevelCallbackTests$InnermostAndOutermost@7a1a14a4] with position [INNERMOST].
Registering extension point [org.junit.gen5.engine.junit5.ClassLevelCallbackTests$InnermostAndOutermost$$Lambda$398/643489709@a3d9978] from source [org.junit.gen5.engine.junit5.ClassLevelCallbackTests$InnermostAndOutermost@7a1a14a4] with position [INNERMOST].
Registering extension point [org.junit.gen5.engine.junit5.ClassLevelCallbackTests$InnermostAndOutermost$$Lambda$399/1632914150@4b41dd5c] from source [org.junit.gen5.engine.junit5.ClassLevelCallbackTests$InnermostAndOutermost@7a1a14a4] with position [OUTERMOST].
Registering extension point [org.junit.gen5.engine.junit5.ClassLevelCallbackTests$InnermostAndOutermost$$Lambda$400/999736366@5d066c7d] from source [org.junit.gen5.engine.junit5.ClassLevelCallbackTests$InnermostAndOutermost@7a1a14a4] with position [OUTERMOST].

—
Reply to this email directly or view it on GitHub.

[sbrannen]

We forgot one:

• Rename TestExtensionRegistry to ExtensionRegistry.

[jlink]

Here's my take on the current s tate of extension registration. I also think it needs some overhaul.

• Repeated registration of same extension should have an influence on ordering of extension points. Having such a mechanism in place would also allow to "switch off" an extension. I'd wait with implementing switching off, though, until a real need comes up.
• The exact lifecycle of an extension should be an implementation detail that is not guaranteed. This will make life for us easier if we decide later to change it, e.g. to instantiation per test in concurrent mode. Since we have ExtensionContext.getStore() there is no need for an extension to have member variables. We could even have a rule to forbid member state (see Introduce validation rules for tests #121).
• The current way of specifying extension ordering can surely be improved and the enum values of Position are not intuitive in some cases, e.g. ExceptionHandlerExtensionPoint. However, going to a number is a big step back IMO since it takes away meaningful names. I think we should go the other direction and move towards more semantic ordering.

[sbrannen]

Closing this issue since many of the issues have already been addressed or will be addressed in #506.