Make entry-based extensions execute around classes

[Michael1993]

Related to #623 so we can move forward with it.

@JoergSiebahn could you share your opinion about this approach? Would this work for you?

Proposed commit message:

${action} (${issues} / ${pull-request}) [max 70 characters]

${body} [max 70 characters per line]

${references}: ${issues}
PR: ${pull-request}

---

PR checklist

The following checklist shall help the PR's author, the reviewers and maintainers to ensure the quality of this project.
It is based on our contributors guidelines, especially the "writing code" section.
It shall help to check for completion of the listed points.
If a point does not apply to the given PR's changes, the corresponding entry can be simply marked as done.

Documentation (general)

• There is documentation (Javadoc and site documentation; added or updated)
• There is implementation information to describe why a non-obvious source code / solution got implemented
• Site documentation has its own .adoc file in the docs folder, e.g. docs/report-entries.adoc
• Site documentation in .adoc file references demo in src/demo/java instead of containing code blocks as text
• Only one sentence per line (especially in .adoc files)
• Javadoc uses formal style, while sites documentation may use informal style

Documentation (new extension)

• The docs/docs-nav.yml navigation has an entry for the new extension
• The package-info.java contains information about the new extension

Code

• Code adheres to code style, naming conventions etc.
• Successful tests cover all changes
• There are checks which validate correct / false usage / configuration of a functionality and there are tests to verify those checks
• Tests use AssertJ or our own PioneerAssert (which are based on AssertJ)

Contributing

• A prepared commit message exists
• The list of contributions inside README.md mentions the new contribution (real name optional)

[beatngu13]

Hmmm, I thought we want to avoid this (#623 (comment)):

If at all possible, I'd like to avoid an option because it makes the user choose.

And have settled on (#623 (comment)):

After today's maintainer meeting: we would first try to keep the current/new behavior, but make the extension additionally visible in @BeforeAll.

?

[Michael1993]

I don't think it makes the user choose? By default it's always test-scoped with an optional configuration option to make it class scoped.

[Michael1993]

Also I don't know what this means:

but make the extension additionally visible in @BeforeAll.

How do you want to achieve that?

[beatngu13]

I don't think it makes the user choose? By default it's always test-scoped with an optional configuration option to make it class scoped.

"configuration option", read it again, slowly. 😬 It is very similar to the first draft, to which @nipafx responded as above.

Also I don't know what this means:

but make the extension additionally visible in @BeforeAll.

How do you want to achieve that?

The second draft already works locally:

Diff (as I said, locally and early stage 😉):

--- a/src/main/java/org/junitpioneer/jupiter/AbstractEntryBasedExtension.java
+++ b/src/main/java/org/junitpioneer/jupiter/AbstractEntryBasedExtension.java
@@ -27,7 +27,9 @@ import java.util.Set;
 import java.util.function.Function;
 import java.util.stream.Stream;
 
+import org.junit.jupiter.api.extension.AfterAllCallback;
 import org.junit.jupiter.api.extension.AfterEachCallback;
+import org.junit.jupiter.api.extension.BeforeAllCallback;
 import org.junit.jupiter.api.extension.BeforeEachCallback;
 import org.junit.jupiter.api.extension.ExtensionConfigurationException;
 import org.junit.jupiter.api.extension.ExtensionContext;
@@ -46,7 +48,12 @@ import org.junitpioneer.internal.PioneerUtils;
  * @param <S> The set annotation type.
  */
 abstract class AbstractEntryBasedExtension<K, V, C extends Annotation, S extends Annotation>
-               implements BeforeEachCallback, AfterEachCallback {
+               implements BeforeEachCallback, AfterEachCallback, BeforeAllCallback, AfterAllCallback {
+
+       @Override
+       public void beforeAll(ExtensionContext context) {
+               beforeEach(context);
+       }
 
        @Override
        public void beforeEach(ExtensionContext context) {
@@ -138,7 +145,12 @@ abstract class AbstractEntryBasedExtension<K, V, C extends Annotation, S extends
        }
 
        @Override
-       public void afterEach(ExtensionContext context) throws Exception {
+       public void afterAll(ExtensionContext context) {
+               afterEach(context);
+       }
+
+       @Override
+       public void afterEach(ExtensionContext context) {
                // apply from innermost to outermost
                PioneerUtils.findAllContexts(context).forEach(this::restoreOriginalEntries);
        }

Some existing tests are failing due to backup/reset issues. This is the hard part, and I haven't had time so far to wrap my head around.

But personally I also like the configuration option, mainly because of two reasons:

1. It reminds me of TestInstance.Lifecycle.
2. The implementation is much, much easier.

So, I'm happy to take a step back and again talk about the design/approach (preferably not here in the PR, but in the issue and/or during a maintainer meeting).

[Michael1993]

"configuration option", read it again, slowly. 😬 It is very similar to the first draft, to which @nipafx responded as above.

I thought there is a difference between "making" the user choose and "letting" the user choose. I think this approach just adds more options to our users to configure the extensions. It's also very simple and straightforward (except if someone tries to set a method annotation to mode = CLASS).

[Bukama]

Javadoc :)

[Michael1993]

Javadoc and normal documentation is pending, I just wanted to check if this approach is OK, first.

[JoergSiebahn]

@JoergSiebahn could you share your opinion about this approach? Would this work for you?

Thanks for asking. For me the second draft that applies class level annotations in beforeAll and beforeEach feels more reasonable but also has a drawback:

• If I'm not missing something it is compatible to release 1.7.0 and (nearly compatible) to versions before 1.7.0. („nearly“ = see downside below)
• My personal expectation is that a class level annotation is applied in beforeAll.
• To have independent test cases, applying it in beforeEach again, is arguable.
• The downside is that a wanted or needed dependency between test methods is hard to achieve as test2 in the second draft shows. Imho, this downside exists in 1.7.0 as well. That may be mitigated by adding an optional config option for the class level annotation like CLASS_LEVEL_ONLY. I'm not sure how understandable such a setting is for the users.

However, I'm fine with the current approach based on the first draft. It's not much that has to be changed in client code after upgrade.

[Michael1993]

@JoergSiebahn could you share your opinion about this approach? Would this work for you?

Thanks for asking. For me the second draft that applies class level annotations in beforeAll and beforeEach feels more reasonable but also has a drawback:

• If I'm not missing something it is compatible to release 1.7.0 and (nearly compatible) to versions before 1.7.0. („nearly“ = see downside below)
• My personal expectation is that a class level annotation is applied in beforeAll.
• To have independent test cases, applying it in beforeEach again, is arguable.
• The downside is that a wanted or needed dependency between test methods is hard to achieve as test2 in the second draft shows. Imho, this downside exists in 1.7.0 as well. That may be mitigated by adding an optional config option for the class level annotation like CLASS_LEVEL_ONLY. I'm not sure how understandable such a setting is for the users.

However, I'm fine with the current approach based on the first draft. It's not much that has to be changed in client code after upgrade.

Would using an array for mode work for you? I.e:

@SetSystemProperty(key = "", value = "", mode = { ApplyMode.TEST, ApplyMode.CLASS })

[JoergSiebahn]

Would using an array for mode work for you?

Based on the proposed changes, an array would give another option. To me it seems like it would cover an edge case where the property is required for the class and must be reset for a test because it is changed in another test. I'm not sure if the additional complexity is worth it. Assuming the scenario is an edge case, you could achieve the same with annotating the class and the test method. Maybe I'm missing some other scenarios.

When using TEST and CLASS as a default for class level annotations, you need different defaults based on the location (class or method). That may be hard to understand without looking in the code.

So these are just some thoughts from my point of view. In the end any solution discussed so far will solve our case.

[Michael1993]

Would using an array for mode work for you?

Based on the proposed changes, an array would give another option. To me it seems like it would cover an edge case where the property is required for the class and must be reset for a test because it is changed in another test. I'm not sure if the additional complexity is worth it. Assuming the scenario is an edge case, you could achieve the same with annotating the class and the test method. Maybe I'm missing some other scenarios.

When using TEST and CLASS as a default for class level annotations, you need different defaults based on the location (class or method). That may be hard to understand without looking in the code.

So these are just some thoughts from my point of view. In the end any solution discussed so far will solve our case.

Well, actually it doesn't make sense in general. Since the annotations are repeatable, you could just add one for TEST and one for CLASS.

[Michael1993]

Going back to your earlier comment @beatngu13 - should we actually use TestInstance.Lifecycle (or, alternatively, its enum values)?

EDIT: Hm, well, on second thought, using it directly wouldn't make sense and using its values might confuse people...

[Bukama]

I'm a bit curious if you really need the mode, as the unique id should be unique anyway, isn't it?
If you provide a good explaining commit message, you can merge :)

[beatngu13]

I also did three commits, feel free to revert if you dislike them and/or I missed something.

[nipafx]

This conversation left us with the open question of whether this PR can be merged before 2.0.

Con:

• it changes behavior and might force people to change their code

Pro:

• pushing it to 2.0 not only delays it into the future, it also requires people to upgrade to Java 11, which can be a big ask

[Bukama]

After the documentation is updated (see @nipafx request above) I'm open to merge into merge for 1.x release to make it (also) available for Java 8 users. Yes behavior is changed, but for me bugfixes may change behavior which requires users to change there code. But we should not force user to upgrade to Java 11.

[JoergSiebahn]

Thanks a lot for the fix. 1.9.0 worked out of the box.