New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make entry-based extensions execute around classes #641
Conversation
Hmmm, I thought we want to avoid this (#623 (comment)):
And have settled on (#623 (comment)):
? |
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. |
Also I don't know what this means:
How do you want to achieve that? |
"configuration option", read it again, slowly. 😬 It is very similar to the first draft, to which @nipafx responded as above.
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:
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). |
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 |
* | ||
* http://www.eclipse.org/legal/epl-v20.html | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Javadoc :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Javadoc and normal documentation is pending, I just wanted to check if this approach is OK, first.
Thanks for asking. For me the second draft that applies class level annotations in
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 @SetSystemProperty(key = "", value = "", mode = { ApplyMode.TEST, ApplyMode.CLASS }) |
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 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 |
Going back to your earlier comment @beatngu13 - should we actually use EDIT: Hm, well, on second thought, using it directly wouldn't make sense and using its values might confuse people... |
…into issue/623-config-entry-extensions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also did three commits, feel free to revert if you dislike them and/or I missed something.
src/test/java/org/junitpioneer/jupiter/SystemPropertyExtensionTests.java
Show resolved
Hide resolved
src/test/java/org/junitpioneer/jupiter/SystemPropertyExtensionTests.java
Show resolved
Hide resolved
This conversation left us with the open question of whether this PR can be merged before 2.0. Con:
Pro:
|
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. |
Thanks a lot for the fix. 1.9.0 worked out of the box. |
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:
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)
.adoc
file in thedocs
folder, e.g.docs/report-entries.adoc
.adoc
file references demo insrc/demo/java
instead of containing code blocks as text.adoc
files)Documentation (new extension)
docs/docs-nav.yml
navigation has an entry for the new extensionpackage-info.java
contains information about the new extensionCode
Contributing
README.md
mentions the new contribution (real name optional)