-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix tests not restoring system properties #658
Fix tests not restoring system properties #658
Conversation
Runtime.getRuntime().addShutdownHook(new ShutdownHook()); | ||
System.setProperty("log4j.configurationFile", "log4j2-console.xml"); | ||
try { | ||
// TODO: Does not work correctly; when shutdown hook fails test has already finished successfully |
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.
Added this comment because test might be misleading. Test "failures" would most likely not actually cause test to fail.
@@ -63,7 +63,6 @@ public static void beforeClass() { | |||
|
|||
@BeforeEach | |||
public void before() { | |||
System.setProperty(LOG4J_SKIP_JANSI, "true"); |
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.
Have removed this because it is set in the beforeClass
method above already.
@@ -71,17 +77,17 @@ public void testAdditivity() throws Exception { | |||
|
|||
@Test | |||
public void testIncludeLocationDefaultsToFalse() { | |||
final LoggerConfig rootLoggerConfig = |
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.
Have fixed some inconsistent indentation here (mixed tabs and spaces).
The test failures seem to be unrelated to these changes; they are also occurring for |
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. |
I seems that should move everything to JUnit 5 first. |
The reason I did not mention about |
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) |
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 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 |
This was closed automatically by Github because we renamed the |
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:
And:
As of Pioneer 2.1.0, there is
|
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)
toSystem.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.