Introduce system property extension

[beatngu13]

Introduces a Jupiter extension to clear (@ClearSystemProperty) or set (@SetSystemProperty) system properties for tests. Both annotations work on the test method and class level, are repeatable as well as combinable. Resolves #129.

@ClearSystemProperty(key = "some property")
@Test
void clearSystemProperty() {
	assertThat(System.getProperty("some property")).isNull();
}

@SetSystemProperty(key = "some property", value = "new value")
@Test
void setSystemProperty() {
	assertThat(System.getProperty("some property")).isEqualTo("new value");
}

Resolves #129

---

I hereby agree to the terms of the JUnit Pioneer Contributor License Agreement.

[nipafx]

Great PR, thanks Daniel! :)

There's the comment below and a few more things to do:

• add a file docs/system-properties.adoc in which you describe the feature, i.e. its use case and how to use it
• add a corresponding line to docs/docs-nav.yml
• add a corresponding line to src/main/java/org/junitpioneer/jupiter/package-info.java (never mind that it's incomplete - that's fixed somewhere else)

If you don't want to do the docs, just let me know. No problem, I will do it then.

[beatngu13]

Updated the docs, please let me know if there is anything else missing or you would like to change something.

I've also changed the copyright note, which was outdated. Do you know why Spotless complains about this change?

https://travis-ci.org/junit-pioneer/junit-pioneer/jobs/606463317#L291

[nipafx]

Looks great! Thanks for updating this after such a long time. Just a few corner cases to discuss/fix.

[beatngu13]

Thanks for your review, very helpful comments! Resolved all but:

• Introduce system property extension #133 (comment)
• Introduce system property extension #133 (comment)

Have to think about them … 🤔

Done.

[nipafx]

Hey @beatngu13, looking very good. I got sidetracked into refactoring storing/restoring the backup properties ~> 45b54ad. I wasn't quite happy how the code (almost) repeated itself when storing and when restoring, but I'm not sure, whether my refactoring improved matters much (or at all).

Have a look and let me know what you think - we can revert it if you want. Otherwise we're done here. 🙌 Time to come up with a commit message.

[Bukama]

Hey @beatngu13, looking very good. I got sidetracked into refactoring storing/restoring the backup properties ~> 45b54ad. I wasn't quite happy how the code (almost) repeated itself when storing and when restoring, but I'm not sure, whether my refactoring improved matters much (or at all).

Have a look and let me know what you think - we can revert it if you want. Otherwise we're done here. 🙌 Time to come up with a commit message.

As written in chat yesterday I like the refactoring as it's reduce duplicate code and improves readabilty of some things (like the check for properties that were @Set and @Cleared). And, you introduced a check for duplicate annotations - even if we have to wait what will come out of Junit5 Issue 2131.

[beatngu13]

Hey @nicolaiparlog @Bukama,

I wanted to join the Twitch stream, but our daughter was born earlier than we expected … 😉

However, I had a quick look at the changes and they are looking really good! I also didn't like the redundancy I had in my code, so thanks for taking the time and fixing it. Also the fact that we now check for duplicate annotations is really helpful.

Will go over the changes in detail and create a proper commit message ASAP.

[verhas]

Generally: if there is a need for an inline comment then it is a code smell. In this case, the code uses a Map instead of a Set because the collector to the Map throws an exception, while the collector to the Set does not (which is btw. absolutely logical).

The nice solution is to use a special set that throws an exception when an element already in the set is added:

private static class UniqueThrowingSet extends HashSet {
        @Override
        public boolean add(Object o) {
            if (contains(o)) {
                throw new IllegalStateException("Duplicate element cannot be added");
            }
            return super.add(o);
        }
    }

I am not advocating this solution against the current solution, because this seems to be a bit overkill for the actual problem.

[beatngu13]

I wouldn't say an inline comment is a code smell per se, sometimes it is perfectly reasonable. However, I agree that the current solution looks a bit "hackish".

Another thing we could do is to use a custom collector:

private static <T> Collector<T, Set<T>, Set<T>> toSetStrict() {
	return Collector.of(
		HashSet::new,
		(set, element) -> {
			if (set.contains(element)) {
				throw new IllegalStateException();
			}
			set.add(element);
		},
		(left, right) -> {
			left.addAll(right);
			return left;
		});
}

Then use it like:

findRepeatableAnnotations(context, ClearSystemProperty.class).stream()
        .map(ClearSystemProperty::key)
        .collect(toSetStrict());

[beatngu13]

I wouldn't say an inline comment is a code smell per se, sometimes it is perfectly reasonable. However, I agree that the current solution looks a bit "hackish".

Another thing we could do is to use a custom collector:

private static <T> Collector<T, Set<T>, Set<T>> toSetStrict() {
	return Collector.of(
		HashSet::new,
		(set, element) -> {
			if (set.contains(element)) {
				throw new IllegalStateException();
			}
			set.add(element);
		},
		(left, right) -> {
			left.addAll(right);
			return left;
		});
}

Then use it like:

findRepeatableAnnotations(context, ClearSystemProperty.class).stream()
        .map(ClearSystemProperty::key)
        .collect(toSetStrict());

[beatngu13]

I personally find the method name a bit ambiguous. Maybe something like preventClearAndSetSameSystemProperties?

[beatngu13]

Wouldn't Set be more appropriate?

• The field is backed by a HashSet.
• For the properties to set, we are using a Map with similar characteristics (no duplicates).
• While we don't rely on it internally, the no-duplicates property probably better captures the intent—it doesn't make sense to clear a property twice or more.

What do you think?

[beatngu13]

Proposed commit message:

Implement @ClearSystemProperty and @SetSystemProperty

Sometimes it is helpful to clear and/or set system properties for a
test execution. This PR introduces two annotations,
`@ClearSystemProperty` and `@SetSystemProperty`, that allow to do
this in a save manner with a proper restore mechanism. Both
annotations work on the test method and class level, are repeatable
as well as combinable.

Fixes: #129

PR: #133

[nipafx]

Thanks a lot, @beatngu13 for the work and perseverance. 👍

[nipafx]

Wow, PR went from 2018 to 2020.