logging-log4j2
logging-log4j2 copied to clipboard
apache
Fix tests not restoring system properties
Some tests were changing system properties, but were not reverting their changes. This pull request tries to address this issue.
In some cases there were System.setProperty(Constants.LOG4J_CONTEXT_SELECTOR, Strings.EMPTY); calls without that system property being set explicitly. I have removed them (and replaced them with the correct property names, if any) because I assumed that they were copy and paste errors.
This pull request also changes some usage of System.setProperty(..., Strings.EMPTY) to System.clearProperty(...).
Maybe in the future it would be good to use extensions such as JUnit Pioneer (JUnit 5) which support setting and restoring system properties, since manually doing it is rather error-prone.
The test failures seem to be unrelated to these changes; they are also occurring for master, e.g. here.
Isn't there a mechanism we can hook into to take a snapshot of existing properties before and restoring them after a test method?
Isn't there a mechanism we can hook into to take a snapshot of existing properties before and restoring them after a test method?
Yes, there's an app for that ;-) https://junit-pioneer.org/docs/system-properties/
The issue is that currently some tests are still using JUnit 4. For that there is the System Rules project, but I did not want to add two separate extensions / rules without knowing what the migration plans are.
The issue is that currently some tests are still using JUnit 4. For that there is the System Rules project, but I did not want to add two separate extensions / rules without knowing what the migration plans are.
I seems that should move everything to JUnit 5 first.
Yes, there's an app for that ;-) https://junit-pioneer.org/docs/system-properties/
The reason I did not mention about system-properties of JUnit Pioneer is that it operates on an input list of properties, whereas I want to restore all properties back to their original values. I think the former approach is more error-prone and there is no way to tell if a property was leaked.
The issue is that currently some tests are still using JUnit 4. For that there is the System Rules project, but I did not want to add two separate extensions / rules without knowing what the migration plans are.
As @garydgregory noted, all tests need to be migrated to JUnit 5. Given this, maybe we can leverage system-stubs?
Each test class in core runs in an isolated fork, otherwise we couldn’t test different values of properties which are stored into static final fields (e.g. Constants.java)
Each test class in core runs in an isolated fork, otherwise we couldn’t test different values of properties which are stored into static final fields (e.g. Constants.java)
I do not think we should rely on that Maven behavior for all styles of development, yes, it works great on the Maven command line, but it feels like a bit of a hack, leaving cruft behind. I'm not sure all IDEs are smart enough to read POMs that configure Surefire and FailSafe this way and know to do VM forks within the IDE itself. Ideally, I'd like to run all or any group of tests from an IDE and have them pass. Right now, I can only rely on Maven to do that.
The issue is that currently some tests are still using JUnit 4. For that there is the System Rules project, but I did not want to add two separate extensions / rules without knowing what the migration plans are.
As @garydgregory noted, all tests need to be migrated to JUnit 5. Given this, maybe we can leverage system-stubs?
The system stubs warning about Java16+ is not great and it seems like it's not really doing things as simple as I'd like to see. The JUnit Pioneer is cool but too minimal in feature. What I want is something like an @SaveSystemProperties and @RestoreSystemProperties and I do not think you need to do whatever reflection system stubs does. Since sys props is global, having the annotation also use a global would be OK. One could also imagine saving and restoring named savepoints, like in a database transaction, @SaveSystemProperties("Savepoint1"). Such a thing does not exist, but we can write it, just like we have custom JUnit 4 rules.
This was closed automatically by Github because we renamed the release-2.x branch to 2.x. Feel free to resubmit to the 2.x branch.
Hey @vy & @garydgregory,
JUnit Pioneer maintainer here. ✌️ I know this PR is old and the problem it was trying to solve might not be relevant anymore, but regarding:
The reason I did not mention about
system-propertiesof JUnit Pioneer is that it operates on an input list of properties, whereas I want to restore all properties back to their original values. I think the former approach is more error-prone and there is no way to tell if a property was leaked.
And:
The system stubs warning about Java16+ is not great and it seems like it's not really doing things as simple as I'd like to see. The JUnit Pioneer is cool but too minimal in feature. What I want is something like an @SaveSystemProperties and @RestoreSystemProperties and I do not think you need to do whatever reflection system stubs does.
As of Pioneer 2.1.0, there is @RestoreSystemProperties. From the docs:
@RestoreSystemPropertiescan be used to restore changes to system properties made directly in code. While@ClearSystemPropertyand@SetSystemPropertyset or clear specific properties and values, they don’t allow property values to be calculated or parameterized, thus there are times you may want to directly set properties in your test code.@RestoreSystemPropertiescan be placed on test methods or test classes and will completely restore all system properties to their original state after a test or test class is complete.
