You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2021/12/25 01:10:36 UTC
[GitHub] [logging-log4j2] Marcono1234 opened a new pull request #658: Fix tests not restoring system properties
Marcono1234 opened a new pull request #658:
URL: https://github.com/apache/logging-log4j2/pull/658
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.
Maybe in the future it would be good to use extensions such as [JUnit Pioneer (JUnit 5)](https://junit-pioneer.org/docs/system-properties/) which support setting and restoring system properties, since manually doing it is rather error-prone.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] Marcono1234 commented on a change in pull request #658: Fix tests not restoring system properties
Posted by GitBox <gi...@apache.org>.
Marcono1234 commented on a change in pull request #658:
URL: https://github.com/apache/logging-log4j2/pull/658#discussion_r775094711
##########
File path: log4j-core/src/test/java/org/apache/logging/log4j/core/Log4j1222Test.java
##########
@@ -34,8 +34,13 @@
@Test
public void homepageRendersSuccessfully()
{
- System.setProperty("log4j.configurationFile", "log4j2-console.xml");
- 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
Review comment:
Added this comment because test might be misleading. Test "failures" would most likely not actually cause test to fail.
##########
File path: log4j-core/src/test/java/org/apache/logging/log4j/core/appender/ConsoleAppenderTest.java
##########
@@ -63,7 +63,6 @@ public static void beforeClass() {
@BeforeEach
public void before() {
- System.setProperty(LOG4J_SKIP_JANSI, "true");
Review comment:
Have removed this because it is set in the `beforeClass` method above already.
##########
File path: log4j-core/src/test/java/org/apache/logging/log4j/core/async/AsyncLoggerConfigTest.java
##########
@@ -71,17 +77,17 @@ public void testAdditivity() throws Exception {
@Test
public void testIncludeLocationDefaultsToFalse() {
- final LoggerConfig rootLoggerConfig =
Review comment:
Have fixed some inconsistent indentation here (mixed tabs and spaces).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] Marcono1234 commented on pull request #658: Fix tests not restoring system properties
Posted by GitBox <gi...@apache.org>.
Marcono1234 commented on pull request #658:
URL: https://github.com/apache/logging-log4j2/pull/658#issuecomment-1001090717
The issue is that currently some tests are still using JUnit 4. For that there is the [System Rules](https://github.com/stefanbirkner/system-rules) project, but I did not want to add two separate extensions / rules without knowing what the migration plans are.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory edited a comment on pull request #658: Fix tests not restoring system properties
Posted by GitBox <gi...@apache.org>.
garydgregory edited a comment on pull request #658:
URL: https://github.com/apache/logging-log4j2/pull/658#issuecomment-1001191452
> > The issue is that currently some tests are still using JUnit 4. For that there is the [System Rules](https://github.com/stefanbirkner/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](https://github.com/webcompere/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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on pull request #658: Fix tests not restoring system properties
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #658:
URL: https://github.com/apache/logging-log4j2/pull/658#issuecomment-1001072849
> 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/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on pull request #658: Fix tests not restoring system properties
Posted by GitBox <gi...@apache.org>.
vy commented on pull request #658:
URL: https://github.com/apache/logging-log4j2/pull/658#issuecomment-1001159235
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on pull request #658: Fix tests not restoring system properties
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #658:
URL: https://github.com/apache/logging-log4j2/pull/658#issuecomment-1001101249
> The issue is that currently some tests are still using JUnit 4. For that there is the [System Rules](https://github.com/stefanbirkner/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on pull request #658: Fix tests not restoring system properties
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #658:
URL: https://github.com/apache/logging-log4j2/pull/658#issuecomment-1001190503
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on pull request #658: Fix tests not restoring system properties
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #658:
URL: https://github.com/apache/logging-log4j2/pull/658#issuecomment-1001178492
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on pull request #658: Fix tests not restoring system properties
Posted by GitBox <gi...@apache.org>.
vy commented on pull request #658:
URL: https://github.com/apache/logging-log4j2/pull/658#issuecomment-1001070661
Isn't there a mechanism we can hook into to take a snapshot of existing properties before and restoring them after a test method?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on pull request #658: Fix tests not restoring system properties
Posted by GitBox <gi...@apache.org>.
vy commented on pull request #658:
URL: https://github.com/apache/logging-log4j2/pull/658#issuecomment-1001159870
> The issue is that currently some tests are still using JUnit 4. For that there is the [System Rules](https://github.com/stefanbirkner/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](https://github.com/webcompere/system-stubs)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on pull request #658: Fix tests not restoring system properties
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #658:
URL: https://github.com/apache/logging-log4j2/pull/658#issuecomment-1001191452
> > The issue is that currently some tests are still using JUnit 4. For that there is the [System Rules](https://github.com/stefanbirkner/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](https://github.com/webcompere/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 a global, have 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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] Marcono1234 commented on pull request #658: Fix tests not restoring system properties
Posted by GitBox <gi...@apache.org>.
Marcono1234 commented on pull request #658:
URL: https://github.com/apache/logging-log4j2/pull/658#issuecomment-1000960051
The test failures seem to be unrelated to these changes; they are also occurring for `master`, e.g. [here](https://github.com/apache/logging-log4j2/runs/4628017826).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org