Skip to content

Commit

Permalink
Fix annotation lookup in AbstractEntryBasedExtension (#473 / #476)
Browse files Browse the repository at this point in the history
When using `findClosestEnclosingRepeatableAnnotations(...)`, outer
elements such as classes are also considered during search, if the
current element isn't annotated accordingly. This may cause an
`ExtensionConfigurationException` when an entry is both cleared (e.g.
on the class level) and set (e.g. on the method level).

This search is superfluous because Jupiter calls the extension at
this extension points already, so the annotation on outer elements
are already processed.

The solution is to just not look for annotation on enclosing elements.

Closes: #473
PR: #476
  • Loading branch information
beatngu13 committed May 4, 2021
1 parent 0eb9cda commit 3ecf133
Show file tree
Hide file tree
Showing 5 changed files with 57 additions and 9 deletions.
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ The least we can do is to thank them and list some of their accomplishments here
#### 2021

* [Cory Thomas](https://github.com/dump247) contributed the `minSuccess` attribute in [retrying tests](https://junit-pioneer.org/docs/retrying-test/) (#408 / #430)
* [Daniel Kraus](https://github.com/beatngu13) fixed bugs in the environment variable and system property extensions (#432 / #433 and #448 / #449)
* [Daniel Kraus](https://github.com/beatngu13) fixed bugs in the environment variable and system property extensions (#432 / #433, #448 / #449, and #473 / #476)
* [Slawomir Jaranowski](https://github.com/slawekjaranowski) Migrate to new Shipkit plugins (#410 / #419)
* [Stefano Cordio](https://github.com/scordio) contributed [the Cartesian Enum source](https://junit-pioneer.org/docs/cartesian-product/#cartesianenumsource) (#379 / #409 and #414 / #453)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import java.util.concurrent.atomic.AtomicBoolean;

import org.junit.jupiter.api.extension.ExtensionContext;
import org.junit.platform.commons.support.AnnotationSupport;
import org.junitpioneer.internal.PioneerAnnotationUtils;
import org.junitpioneer.internal.PioneerUtils;

Expand All @@ -36,16 +37,26 @@ protected boolean isAnnotationPresent(ExtensionContext context) {

@Override
protected Set<String> entriesToClear(ExtensionContext context) {
return PioneerAnnotationUtils
.findClosestEnclosingRepeatableAnnotations(context, ClearEnvironmentVariable.class)
return AnnotationSupport
// This extension implements `BeforeAllCallback` and `BeforeEachCallback` and so if an outer class
// (i.e. a class that the test class is @Nested within) uses this extension, this method will be
// called on those extension points and discover the variables to set/clear. That means we don't need
// to search for enclosing annotations here.
.findRepeatableAnnotations(context.getElement(), ClearEnvironmentVariable.class)
.stream()
.map(ClearEnvironmentVariable::key)
.collect(PioneerUtils.distinctToSet());
}

@Override
protected Map<String, String> entriesToSet(ExtensionContext context) {
return PioneerAnnotationUtils
.findClosestEnclosingRepeatableAnnotations(context, SetEnvironmentVariable.class)
return AnnotationSupport
// This extension implements `BeforeAllCallback` and `BeforeEachCallback` and so if an outer class
// (i.e. a class that the test class is @Nested within) uses this extension, this method will be
// called on those extension points and discover the variables to set/clear. That means we don't need
// to search for enclosing annotations here.
.findRepeatableAnnotations(context.getElement(), SetEnvironmentVariable.class)
.stream()
.collect(toMap(SetEnvironmentVariable::key, SetEnvironmentVariable::value));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import java.util.Set;

import org.junit.jupiter.api.extension.ExtensionContext;
import org.junit.platform.commons.support.AnnotationSupport;
import org.junitpioneer.internal.PioneerAnnotationUtils;
import org.junitpioneer.internal.PioneerUtils;

Expand All @@ -29,16 +30,26 @@ protected boolean isAnnotationPresent(ExtensionContext context) {

@Override
protected Set<String> entriesToClear(ExtensionContext context) {
return PioneerAnnotationUtils
.findClosestEnclosingRepeatableAnnotations(context, ClearSystemProperty.class)
return AnnotationSupport
// This extension implements `BeforeAllCallback` and `BeforeEachCallback` and so if an outer class
// (i.e. a class that the test class is @Nested within) uses this extension, this method will be
// called on those extension points and discover the properties to set/clear. That means we don't need
// to search for enclosing annotations here.
.findRepeatableAnnotations(context.getElement(), ClearSystemProperty.class)
.stream()
.map(ClearSystemProperty::key)
.collect(PioneerUtils.distinctToSet());
}

@Override
protected Map<String, String> entriesToSet(ExtensionContext context) {
return PioneerAnnotationUtils
.findClosestEnclosingRepeatableAnnotations(context, SetSystemProperty.class)
return AnnotationSupport
// This extension implements `BeforeAllCallback` and `BeforeEachCallback` and so if an outer class
// (i.e. a class that the test class is @Nested within) uses this extension, this method will be
// called on those extension points and discover the properties to set/clear. That means we don't need
// to search for enclosing annotations here.
.findRepeatableAnnotations(context.getElement(), SetSystemProperty.class)
.stream()
.collect(toMap(SetSystemProperty::key, SetSystemProperty::value));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,19 @@ void methodLevelShouldOverwriteClassLevel() {
assertThat(systemEnvironmentVariable("clear envvar F")).isNull();
}

@Test
@DisplayName("method level should not clash (in terms of duplicate entries) with class level")
@SetEnvironmentVariable(key = "set envvar A", value = "new A")
void methodLevelShouldNotClashWithClassLevel() {
assertThat(systemEnvironmentVariable("set envvar A")).isEqualTo("new A");
assertThat(systemEnvironmentVariable("set envvar B")).isEqualTo("old B");
assertThat(systemEnvironmentVariable("set envvar C")).isEqualTo("old C");
assertThat(systemEnvironmentVariable("clear envvar D")).isEqualTo("new D");

assertThat(systemEnvironmentVariable("clear envvar E")).isNull();
assertThat(systemEnvironmentVariable("clear envvar F")).isNull();
}

}

@DisplayName("with nested classes")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,19 @@ void methodLevelShouldOverwriteClassLevel() {
assertThat(System.getProperty("clear prop F")).isNull();
}

@Test
@DisplayName("method level should not clash (in terms of duplicate entries) with class level")
@SetSystemProperty(key = "set prop A", value = "new A")
void methodLevelShouldNotClashWithClassLevel() {
assertThat(System.getProperty("set prop A")).isEqualTo("new A");
assertThat(System.getProperty("set prop B")).isEqualTo("old B");
assertThat(System.getProperty("set prop C")).isEqualTo("old C");
assertThat(System.getProperty("clear prop D")).isEqualTo("new D");

assertThat(System.getProperty("clear prop E")).isNull();
assertThat(System.getProperty("clear prop F")).isNull();
}

}

@DisplayName("with nested classes")
Expand Down

0 comments on commit 3ecf133

Please sign in to comment.