Contribute SystemPropertyExtension #5258
Conversation
✅ All tests passed ✅Test Summary
🏷️ Commit: 0cea1ed Learn more about TestLens at testlens.app. |
junit-jupiter-api/src/main/java/org/junit/jupiter/api/util/SystemPropertyExtension.java
Outdated
Show resolved
Hide resolved
mpkorstanje
force-pushed
the
4726_passsystempropext
branch
from
5115788 to
8426118
Compare
junit-jupiter-api/src/main/java/org/junit/jupiter/api/util/SystemPropertyExtension.java
Outdated
Show resolved
Hide resolved
junit-jupiter-api/src/main/java/org/junit/jupiter/api/util/SystemPropertyExtension.java
Outdated
Show resolved
Hide resolved
junit-jupiter-api/src/main/java/org/junit/jupiter/api/util/SystemPropertyExtension.java
Outdated
Show resolved
Hide resolved
jbduncan
left a comment
jbduncan
left a comment
There was a problem hiding this comment.
Just a couple of nits for consideration, but otherwise this draft is looking fantastic so far!
junit-jupiter-api/src/main/java/org/junit/jupiter/api/util/SystemPropertyExtensionUtils.java
Outdated
Show resolved
Hide resolved
junit-jupiter-api/src/main/java/org/junit/jupiter/api/util/SystemPropertyExtensionUtils.java
Outdated
Show resolved
Hide resolved
mpkorstanje
force-pushed
the
4726_passsystempropext
branch
from
62cf959 to
7e2eb7c
Compare
|
|
||
| The system `Properties` object is normally just a hashmap of strings, however, it is technically possible to store non-string values and create {jdk-javadoc-base-url}/java.base/java/util/Properties.html#%3Cinit%3E(java.util.Properties)[nested `Properties` with inherited default values]. | ||
| `@RestoreSystemProperties` restores the original `Properties` object with all of its potential richness _after_ the annotated scope is complete. | ||
| However, for use during the test _within_ the test scope it provides a cloned `Properties` object with these limitations: |
There was a problem hiding this comment.
For team discussion: Are these limitations acceptable?
junit-jupiter-api/src/main/java/org/junit/jupiter/api/util/SystemPropertyExtension.java
Outdated
Show resolved
Hide resolved
|
For discussion: As originally written the This effective clone is limited to the observable entries and defaults of the original properties object. This means that default values that are hidden by an entry are not included. In essence this test will fail: var defaults = new Properties();
defaults.setProperty("a", "a");
var properties = new Properties(defaults);
properties.put("a", "b");
var clone = createEffectiveClone(properties);
assertThat(clone.getProperty("a")).isEqualTo("b");
clone.remove("a");
assertThat(clone.getProperty("a")).isEqualTo("a"); // fails: actually null.This can be avoided by changing the cloning process such that the entries from the original properties object are removed to expose the default values, and then restoring the entries after the defaults have been copied. But this requires mutating the original object. Is that acceptable? |
junit-jupiter-api/src/main/java/org/junit/jupiter/api/util/SystemPropertyExtension.java
Outdated
Show resolved
Hide resolved
I checked the docs for I can't believe I'm saying this, but would The way I see it, there is a small window for tests to flake if this extension mutates the system properties during cloning whilst other tests running in parallel are using them. So whereas this mutation would be acceptable to me because I'm acutely aware of the perils of global state and race conditions and I would avoid calling |
No. Clone doesn't include the defaults. And serialization would duplicate any objects in a corrupted properties object. If they're serializable in the first place...
Good point. |
mpkorstanje
force-pushed
the
4726_passsystempropext
branch
from
b8bb156 to
0cea1ed
Compare
|
Note: Manually set DCO check to pass. Originally #5184 was contributed before the DCO bot was enabled. |
I also think we should avoid mutating it |
| [[writing-tests-built-in-extensions-SystemProperty]] | ||
| ==== `@ClearSystemProperty` and `@SetSystemProperty` | ||
|
|
||
| The `@ClearSystemProperty` and `@SetSystemProperty` annotations can be used to clear and set, respectively, the values of system properties for a test execution. |
There was a problem hiding this comment.
Our usual convention is to link the first occurrence of a class in a "section" to its Javadoc page (see above for {DefaultLocale}).
| `@DefaultLocale` or `@DefaultTimeZone`. | ||
|
|
||
| [[writing-tests-built-in-extensions-SystemProperty]] | ||
| ==== `@ClearSystemProperty` and `@SetSystemProperty` |
There was a problem hiding this comment.
Code snippets in headers look weird in Antora's theme so we should avoid them.
| ==== `@ClearSystemProperty` and `@SetSystemProperty` | |
| == @ClearSystemProperty and @SetSystemProperty |
| A class-level configuration means that the specified system properties are cleared/set before and reset after each individual test in the annotated class. | ||
| ==== | ||
|
|
||
| == `@RestoreSystemProperties` |
There was a problem hiding this comment.
Ditto
| == `@RestoreSystemProperties` | |
| [[RestoreSystemProperties]] | |
| == @RestoreSystemProperties |
| 'Restore', is only needed if system properties are modified in some way _other than_ Clear and Set during a test. | ||
| ==== | ||
|
|
||
| === `@RestoreSystemProperties` Limitations |
There was a problem hiding this comment.
Should be enough since it's below @RestoreSystemProperties, right?
| === `@RestoreSystemProperties` Limitations | |
| [[RestoreSystemProperties-limitations]] | |
| === Limitations |
| == Thread-Safety | ||
|
|
||
| Since system properties are global state, reading and writing them during <<writing-tests-parallel-execution, parallel test execution>> can lead to unpredictable results and flaky tests. | ||
| The system property extension is prepared for that and tests annotated with `@ClearSystemProperty`, `@SetSystemProperty`, or `@RestoreSystemProperties` will never execute in parallel (thanks to https://docs.junit.org/current/api[resource locks]) to guarantee correct test results. |
There was a problem hiding this comment.
| The system property extension is prepared for that and tests annotated with `@ClearSystemProperty`, `@SetSystemProperty`, or `@RestoreSystemProperties` will never execute in parallel (thanks to https://docs.junit.org/current/api[resource locks]) to guarantee correct test results. | |
| The system property extension is prepared for that and tests annotated with `@ClearSystemProperty`, `@SetSystemProperty`, or `@RestoreSystemProperties` will never execute in parallel (thanks to xref:writing-tests/parallel-execution.adoc#synchronization[resource locks]) to guarantee correct test results. |
| List<Extension> extensions = registry.getExtensions(Extension.class); | ||
|
|
||
| assertEquals(NUM_DEFAULT_EXTENSIONS, extensions.size()); | ||
| assertEquals(NUM_DEFAULT_EXTENSIONS + 1, extensions.size()); |
| * Marks tests that read system properties but don't use the system property extension themselves. | ||
| * | ||
| * <p>During | ||
| * <a href="https://docs.junit.org/current/user-guide/#writing-tests-parallel-execution" target="_top">parallel test execution</a>, |
There was a problem hiding this comment.
| * <a href="https://docs.junit.org/current/user-guide/#writing-tests-parallel-execution" target="_top">parallel test execution</a>, | |
| * <a href="https://docs.junit.org/current/writing-tests/parallel-execution.html" target="_top">parallel test execution</a>, |
| * annotated with {@code RestoreSystemProperties}. | ||
| * | ||
| * <p>During | ||
| * <a href="https://docs.junit.org/current/user-guide/#writing-tests-parallel-execution" target="_top">parallel test execution</a>, |
There was a problem hiding this comment.
| * <a href="https://docs.junit.org/current/user-guide/#writing-tests-parallel-execution" target="_top">parallel test execution</a>, | |
| * <a href="https://docs.junit.org/current/writing-tests/parallel-execution.html" target="_top">parallel test execution</a>, |
| * configurations.</p> | ||
| * | ||
| * <p>During | ||
| * <a href="https://docs.junit.org/current/user-guide/#writing-tests-parallel-execution" target="_top">parallel test execution</a>, |
There was a problem hiding this comment.
| * <a href="https://docs.junit.org/current/user-guide/#writing-tests-parallel-execution" target="_top">parallel test execution</a>, | |
| * <a href="https://docs.junit.org/current/writing-tests/parallel-execution.html" target="_top">parallel test execution</a>, |
| * Marks tests that write system properties but don't use the system property extension themselves. | ||
| * | ||
| * <p>During | ||
| * <a href="https://docs.junit.org/current/user-guide/#writing-tests-parallel-execution" target="_top">parallel test execution</a>, |
There was a problem hiding this comment.
| * <a href="https://docs.junit.org/current/user-guide/#writing-tests-parallel-execution" target="_top">parallel test execution</a>, | |
| * <a href="https://docs.junit.org/current/writing-tests/parallel-execution.html" target="_top">parallel test execution</a>, |
Adopts the
SystemPropertyExtensionfrom JUnit Pioneer into JUnit Jupiter.While this is a faithful port, the following changes have been made:
AbstractEntryBasedExtensionhas been merged into theSystemPropertyExtensionfor clarity. Should theEnvironmentVariableExtensionever be contributed we can undo that.PropertiesAssertionshas been trimmed down to the essentials and does not require reflection to workSystemPropertyExtensionUtilsmethods have been in lined or moved toJupiterPropertyUtils.Closes: #4726
I hereby agree to the terms of the JUnit Contributor License Agreement.
Definition of Done
@APIannotations