Support declarative extension registration on fields and parameters #2680

sbrannen · 2021-08-03T12:23:53Z

Overview

This is a PR for #864.

The implementation is essentially feature complete and tested (following the 80/20 rule); however, there are some points that should be discussed before merging this PR into the main branch.

Topics for Discussion

When discussing any of these topics, I recommend that you review the current behavior by analyzing the tests in ExtensionRegistrationViaParametersAndFieldsTests before spending time studying the actual implementation details.

Registration Order: I have done my best to register extensions in the order in which the fields or parameters are used in the lifecycle of a test class and test method. This can be inspected in the registrationOrder() test method. Implementation details can be found in ClassBasedTestDescriptor and TestMethodTestDescriptor.
@Order Support: ExtensionUtils.registerExtensionsFromFields() already supported @Order for extensions registered via @RegisterExtension, and @Order is now also applied to extensions registered via fields using @ExtendWith. But... @ExtendWith extensions are registered before @RegisterExtension extensions.
@ExtendWith target: @ExtendWith can currently be declared on a TYPE or METHOD. It cannot be declared directly on a PARAMETER or FIELD.
Private fields: @RegisterExtension fields are not allowed to be private, but in this PR fields meta-annotated with @ExtendWith are allowed to be private.

I will share my thoughts on these topics as comments and invite feedback.

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 including corner cases, errors, and exception handling
Public API has Javadoc and @API annotations
Change is documented in the User Guide and Release Notes
All continuous integration builds pass

sbrannen · 2021-08-03T12:35:41Z

Private fields: @RegisterExtension fields are not allowed to be private, but in this PR fields meta-annotated with @ExtendWith are allowed to be private.

From one perspective, this is inconsistent treatment of fields; however, I wonder if we haven't been too strict with regard to the current non-private policy for @RegisterExtension fields.

If JUnit or a third party provides an extension that is implicitly registered by a custom composed annotation (which is meta-annotated with @ExtendWith), then it is really up to the user of that composed annotation to decide if the annotated field is private.

As long as the extension can work with a private field, there should be no unnecessary constraint on the visibility of the field.

In other words, why should JUnit not allow the user to decide if a field is private?

I recommend that we allow @RegisterExtension and @ExtendWith fields to be private.

sbrannen · 2021-08-03T12:38:46Z

@ExtendWith target: @ExtendWith can currently be declared on a TYPE or METHOD. It cannot be declared directly on a PARAMETER or FIELD.

Although the original intention of #864 was to allow extension authors to provide custom composed annotations that are meta-annotated with @ExtendWith, after having spent more time with the issue I think there are likely valid use cases for allowing @ExtendWith to be declared directly on a field or parameter.

In other words, I don't believe we should force users or library implementers to create custom composed annotations when all they need is @ExtendWith on a field or parameter.

sbrannen · 2021-08-03T12:49:51Z

@Order Support: ExtensionUtils.registerExtensionsFromFields() already supported @Order for extensions registered via @RegisterExtension, and @Order is now also applied to extensions registered via fields using @ExtendWith. But... @ExtendWith extensions are registered before @RegisterExtension extensions.

The above describes the status quo in this PR, but we can certainly change the semantics.

My rationale for implementing the feature with these semantics was based on the fact that extensions registered via @ExtendWith on a class or interface are always registered before any extensions registered via @RegisterExtension. This is the current behavior in JUnit Jupiter, and that default behavior will not change.

So, the status quo in this PR is consistent in that regard.

However, we have an opportunity (for the first time) to make it possible for extensions registered via @ExtendWith to be ordered by the user relative to extensions registered via @RegisterExtension.

Achieving that is straight forward: I'd just need to collapse the two iterations in ExtensionUtils.registerExtensionsFromFields() into a single iteration over the fields.

After having spent more time with this issue, I am in favor of allowing @RegisterExtension and @ExtendWith fields to be ordered relative to each other, and I imagine the community would likely expect all such fields to be sorted together when using @Order.

sbrannen · 2021-08-03T12:51:34Z

Registration Order: I have done my best to register extensions in the order in which the fields or parameters are used in the lifecycle of a test class and test method. This can be inspected in the registrationOrder() test method. Implementation details can be found in ClassBasedTestDescriptor and TestMethodTestDescriptor.

Aside from the discussion about @Order support, I feel pretty comfortable with the current extension registration order, but feedback is certainly welcome!

codecov · 2021-08-03T15:10:44Z

Codecov Report

Merging #2680 (77cafeb) into main (c8f45de) will increase coverage by 0.03%.
The diff coverage is 93.65%.

@@            Coverage Diff             @@
##             main    #2680      +/-   ##
==========================================
+ Coverage   91.46%   91.49%   +0.03%     
==========================================
  Files         427      427              
  Lines       12067    12100      +33     
  Branches      941      946       +5     
==========================================
+ Hits        11037    11071      +34     
  Misses        790      790              
+ Partials      240      239       -1

Impacted Files	Coverage Δ
...ter/engine/extension/MutableExtensionRegistry.java	`100.00% <ø> (+1.75%)`	⬆️
...iter/engine/execution/DefaultParameterContext.java	`88.23% <66.66%> (+9.97%)`	⬆️
...g/junit/platform/commons/util/AnnotationUtils.java	`92.91% <70.00%> (-1.96%)`	⬇️
...er/engine/descriptor/ClassBasedTestDescriptor.java	`96.92% <100.00%> (+0.11%)`	⬆️
...unit/jupiter/engine/descriptor/ExtensionUtils.java	`100.00% <100.00%> (ø)`
...er/engine/descriptor/TestMethodTestDescriptor.java	`98.37% <100.00%> (+0.04%)`	⬆️

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 c8f45de...77cafeb. Read the comment docs.

sbrannen · 2021-08-03T15:53:15Z

However, we have an opportunity (for the first time) to make it possible for extensions registered via @ExtendWith to be ordered by the user relative to extensions registered via @RegisterExtension.

Related issues:

sbrannen · 2021-08-06T12:22:46Z

Team Decision:

Allow @RegisterExtension and @ExtendWith fields to be private.
Allow @ExtendWith to be declared directly on fields and parameters.
Allow @RegisterExtension and @ExtendWith fields to be sorted via @Order relative to each other.
Throw an exception if a field is annotated with both @RegisterExtension and @ExtendWith.

Prior to this commit, an exception was thrown if a @RegisterExtension field was declared `private`. This commit removes this restriction. See #864, #2680

@ExtendWith

Prior to this commit, @ExtendWith could only be declared on types (i.e., classes, interfaces, annotations) and methods. @ExtendWith can now be declared directly on fields and parameters as well. See #864, #2680

@ExtendWith

Prior to this commit, @ExtendWith could only be declared on types (i.e., classes, interfaces, annotations) and methods. @ExtendWith can now be declared directly on fields and parameters as well. See #864, #2680

@ExtendWith

…each other Prior to this commit, @ExtendWith fields and @RegisterExtension fields were sorted using @order, but extensions registered via @ExtendWith fields were always registered first (before extensions registered via @RegisterExtension fields). This commit ensures that @RegisterExtension fields and @ExtendWith fields are sorted via @order relative to each other. See #864, #2680

@ExtendWith

…each other Prior to this commit, @ExtendWith fields and @RegisterExtension fields were sorted using @order, but extensions registered via @ExtendWith fields were always registered first (before extensions registered via @RegisterExtension fields). This commit ensures that @RegisterExtension fields and @ExtendWith fields are sorted via @order relative to each other. See #864, #2680

@ExtendWith

…each other Prior to this commit, @ExtendWith fields and @RegisterExtension fields were sorted using @order, but extensions registered via @ExtendWith fields were always registered first (before extensions registered via @RegisterExtension fields). This commit ensures that @RegisterExtension fields and @ExtendWith fields are sorted via @order relative to each other. See #864, #2680

@ExtendWith

Prior to this commit, @ExtendWith and @RegisterExtension could be declared on a field to register duplicate extensions of the same time, but that scenario likely does not make sense. This commit ensures that @RegisterExtension and @ExtendWith cannot be used to register an extension of the same type for a given field, by throwing a PreconditionViolationException if the user attempts to do so. See #2680

Prior to this commit, an exception was thrown if a @RegisterExtension field was declared `private`. This commit removes this restriction. See #864, #2680

Issue: #864

@ExtendWith

This commit introduces extensions registered explicitly via @RegisterExtension to verify the respective ordering with extensions registered implicitly via @ExtendWith on fields. Issue: #864

Issue: #864

Prior to this commit, an exception was thrown if a @RegisterExtension field was declared private. This commit removes this restriction. Closes #2688 See #864, #2680

@ExtendWith

Prior to this commit, @ExtendWith could only be declared on types (i.e., classes, interfaces, annotations) and methods. @ExtendWith can now be declared directly on fields and parameters as well. See #864, #2680

@ExtendWith

…each other Prior to this commit, @ExtendWith fields and @RegisterExtension fields were sorted using @order, but extensions registered via @ExtendWith fields were always registered first (before extensions registered via @RegisterExtension fields). This commit ensures that @RegisterExtension fields and @ExtendWith fields are sorted via @order relative to each other. See #864, #2680

@ExtendWith

Prior to this commit, @ExtendWith and @RegisterExtension could be declared on a field to register duplicate extensions of the same time, but that scenario likely does not make sense. This commit ensures that @RegisterExtension and @ExtendWith cannot be used to register an extension of the same type for a given field, by throwing a PreconditionViolationException if the user attempts to do so. See #2680

Issue: #864

Prior to this commit, an exception was thrown if a @RegisterExtension field was declared private. This commit removes this restriction. Closes #2688 See #864, #2680

@ExtendWith

Prior to this commit, @ExtendWith could only be declared on types (i.e., classes, interfaces, annotations) and methods. @ExtendWith can now be declared directly on fields and parameters as well. See #864, #2680

@ExtendWith

…each other Prior to this commit, @ExtendWith fields and @RegisterExtension fields were sorted using @order, but extensions registered via @ExtendWith fields were always registered first (before extensions registered via @RegisterExtension fields). This commit ensures that @RegisterExtension fields and @ExtendWith fields are sorted via @order relative to each other. See #864, #2680

@ExtendWith

Prior to this commit, @ExtendWith and @RegisterExtension could be declared on a field to register duplicate extensions of the same time, but that scenario likely does not make sense. This commit ensures that @RegisterExtension and @ExtendWith cannot be used to register an extension of the same type for a given field, by throwing a PreconditionViolationException if the user attempts to do so. See #2680

@ExtendWith

Prior to this commit, @ExtendWith could only be used to register extensions declaratively on test interfaces, test classes, and test methods. However, there are certain use cases where it is more convenient for the user if extensions can be registered declaratively on fields and parameters. This commit introduces support for registering extensions declaratively via @ExtendWith on the following elements. - static fields - non-static fields - parameters in test class constructors, test methods, and lifecycle methods (i.e., @BeforeAll, @afterall, @beforeeach, and @AfterEach methods) Fields annotated or meta-annotated with @ExtendWith can have any visibility (including private) and can be sorted relative to @RegisterExtension fields via the @order annotation. See the RandomNumberExtension example in the User Guide. Closes #864, #2680

sbrannen · 2021-08-17T09:03:09Z

Closed via commit 8b5387c

Prior to this commit, an exception was thrown if a @RegisterExtension field was declared private. This commit removes this restriction. Closes junit-team#2688 See junit-team#864, junit-team#2680

@ExtendWith

Prior to this commit, @ExtendWith could only be declared on types (i.e., classes, interfaces, annotations) and methods. @ExtendWith can now be declared directly on fields and parameters as well. See junit-team#864, junit-team#2680

@ExtendWith

…each other Prior to this commit, @ExtendWith fields and @RegisterExtension fields were sorted using @order, but extensions registered via @ExtendWith fields were always registered first (before extensions registered via @RegisterExtension fields). This commit ensures that @RegisterExtension fields and @ExtendWith fields are sorted via @order relative to each other. See junit-team#864, junit-team#2680

@ExtendWith

Prior to this commit, @ExtendWith and @RegisterExtension could be declared on a field to register duplicate extensions of the same time, but that scenario likely does not make sense. This commit ensures that @RegisterExtension and @ExtendWith cannot be used to register an extension of the same type for a given field, by throwing a PreconditionViolationException if the user attempts to do so. See junit-team#2680

sbrannen added theme: extensions theme: programming model component: Jupiter type: new feature labels Aug 3, 2021

sbrannen linked an issue Aug 3, 2021 that may be closed by this pull request

Support declarative extension registration on fields and parameters #864

Closed

sbrannen mentioned this pull request Aug 3, 2021

Support declarative extension registration on fields and parameters #864

Closed

This was referenced Aug 3, 2021

Support ordering between @ExtendWith and @RegisterExtension #2651

Closed

Support @ExtendWith on fields #416

Closed

sbrannen added the theme: ordering label Aug 3, 2021

sbrannen added a commit that referenced this pull request Aug 6, 2021

Allow @RegisterExtension fields to be private

ee18ea7

Prior to this commit, an exception was thrown if a @RegisterExtension field was declared `private`. This commit removes this restriction. See #864, #2680

sbrannen self-assigned this Aug 6, 2021

sbrannen marked this pull request as ready for review August 6, 2021 12:45

sbrannen force-pushed the issues/864-declarative-extension-registration-params-fields-round-2 branch from 98d0ca9 to 935ce6f Compare August 6, 2021 13:18

sbrannen force-pushed the issues/864-declarative-extension-registration-params-fields-round-2 branch from d78650b to d89b9da Compare August 6, 2021 13:51

sbrannen force-pushed the issues/864-declarative-extension-registration-params-fields-round-2 branch from 5a97a3d to 1c42810 Compare August 9, 2021 13:55

sbrannen force-pushed the issues/864-declarative-extension-registration-params-fields-round-2 branch from 4a9d1fd to afe5fe2 Compare August 12, 2021 13:47

sbrannen added a commit that referenced this pull request Aug 12, 2021

Allow @RegisterExtension fields to be private

cc3e9c7

Prior to this commit, an exception was thrown if a @RegisterExtension field was declared `private`. This commit removes this restriction. See #864, #2680

This was referenced Aug 13, 2021

Allow @TempDir fields to be private #2687

Closed

Allow @RegisterExtension fields to be private #2688

Closed

marcphilipp approved these changes Aug 15, 2021

View reviewed changes

sbrannen added 14 commits August 17, 2021 10:10

Support extension registration via constructor & method params

999bbb3

Issue: #864

Test extension registration order for constructor & method params

1a15ed7

Issue: #864

Support extension registration via @ExtendWith on fields

5e2e6f9

Issue: #864

Test extension registration via @ExtendWith on fields

a5b7e07

Issue: #864

Test extension registration for double @nested constructor parameter

e093de1

Issue: #864

Update Javadoc for ExtensionUtils.registerExtensionsFromFields()

86f3fbc

Issue: #864

Test implicit vs. explicit extension registration order for fields

5a0e820

This commit introduces extensions registered explicitly via @RegisterExtension to verify the respective ordering with extensions registered implicitly via @ExtendWith on fields. Issue: #864

Polishing

456898f

Issue: #864

Remove code duplication in ExtensionUtils

a73012f

Issue: #864

Allow @RegisterExtension fields to be private

cfe6fe1

Prior to this commit, an exception was thrown if a @RegisterExtension field was declared private. This commit removes this restriction. Closes #2688 See #864, #2680

Allow @ExtendWith to be declared directly on fields and parameters

db6a943

Prior to this commit, @ExtendWith could only be declared on types (i.e., classes, interfaces, annotations) and methods. @ExtendWith can now be declared directly on fields and parameters as well. See #864, #2680

Document support for @ExtendWith on parameters and fields

77cafeb

Issue: #864

sbrannen force-pushed the issues/864-declarative-extension-registration-params-fields-round-2 branch from a740853 to 77cafeb Compare August 17, 2021 08:30

sbrannen added a commit that referenced this pull request Aug 17, 2021

Allow @RegisterExtension fields to be private

3480fc8

Prior to this commit, an exception was thrown if a @RegisterExtension field was declared private. This commit removes this restriction. Closes #2688 See #864, #2680

sbrannen closed this Aug 17, 2021

sbrannen deleted the issues/864-declarative-extension-registration-params-fields-round-2 branch September 18, 2021 14:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support declarative extension registration on fields and parameters #2680

Support declarative extension registration on fields and parameters #2680

sbrannen commented Aug 3, 2021 •

edited

Loading

Uh oh!

sbrannen commented Aug 3, 2021

Uh oh!

sbrannen commented Aug 3, 2021

Uh oh!

sbrannen commented Aug 3, 2021

Uh oh!

sbrannen commented Aug 3, 2021

Uh oh!

codecov bot commented Aug 3, 2021 •

edited

Loading

Uh oh!

sbrannen commented Aug 3, 2021

Uh oh!

sbrannen commented Aug 6, 2021 •

edited

Loading

Uh oh!

sbrannen commented Aug 17, 2021

Uh oh!

Support declarative extension registration on fields and parameters #2680

Support declarative extension registration on fields and parameters #2680

Conversation

sbrannen commented Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Topics for Discussion

Definition of Done

Uh oh!

sbrannen commented Aug 3, 2021

Uh oh!

sbrannen commented Aug 3, 2021

Uh oh!

sbrannen commented Aug 3, 2021

Uh oh!

sbrannen commented Aug 3, 2021

Uh oh!

codecov bot commented Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sbrannen commented Aug 3, 2021

Uh oh!

sbrannen commented Aug 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbrannen commented Aug 17, 2021

Uh oh!

sbrannen commented Aug 3, 2021 •

edited

Loading

codecov bot commented Aug 3, 2021 •

edited

Loading

sbrannen commented Aug 6, 2021 •

edited

Loading