You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/01/03 19:41:34 UTC
[GitHub] [ozone] adoroszlai opened a new pull request #2959: HDDS-6123. Disable system exit in integration tests
adoroszlai opened a new pull request #2959:
URL: https://github.com/apache/ozone/pull/2959
## What changes were proposed in this pull request?
Integration tests should not trigger real `System.exit` call, because it prevents further tests in the surefire fork (`Crashed tests`). In these cases we only get the test output, not test results.
https://issues.apache.org/jira/browse/HDDS-6123
## How was this patch tested?
Regular CI:
https://github.com/adoroszlai/hadoop-ozone/actions/runs/1593529883
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a change in pull request #2959: HDDS-6123. Disable system exit in integration tests
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #2959:
URL: https://github.com/apache/ozone/pull/2959#discussion_r778027019
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/server/TestSecureContainerServer.java
##########
@@ -125,6 +127,7 @@
@BeforeClass
public static void setup() throws Exception {
DefaultMetricsSystem.setMiniClusterMode(true);
+ ExitUtils.disableSystemExit();
Review comment:
The changes in this PR make sense to me, expecially the change inside MiniOzoneCluster, so that if you use a mini-ozone cluster it automatically calls `disableSystemExit()` and individual tests don't need to worry about it.
However the change like this one gives me a little bit of concern. How easy would it be for someone to add a new test class that calls some code with a `System.exit()` they have no idea about? Have we any other way to make this problem invisible to tests somehow? Perhaps there is no other feasible way to fix this, but thought I would ask anyway.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai merged pull request #2959: HDDS-6123. Disable system exit in integration tests
Posted by GitBox <gi...@apache.org>.
adoroszlai merged pull request #2959:
URL: https://github.com/apache/ozone/pull/2959
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #2959: HDDS-6123. Disable system exit in integration tests
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2959:
URL: https://github.com/apache/ozone/pull/2959#discussion_r778173713
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/server/TestSecureContainerServer.java
##########
@@ -125,6 +127,7 @@
@BeforeClass
public static void setup() throws Exception {
DefaultMetricsSystem.setMiniClusterMode(true);
+ ExitUtils.disableSystemExit();
Review comment:
> How easy would it be for someone to add a new test class that calls some code with a System.exit() they have no idea about? Have we any other way to make this problem invisible to tests somehow?
FindBugs / SpotBugs should catch most of such (`DM_EXIT` rule).
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a change in pull request #2959: HDDS-6123. Disable system exit in integration tests
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #2959:
URL: https://github.com/apache/ozone/pull/2959#discussion_r778207720
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/server/TestSecureContainerServer.java
##########
@@ -125,6 +127,7 @@
@BeforeClass
public static void setup() throws Exception {
DefaultMetricsSystem.setMiniClusterMode(true);
+ ExitUtils.disableSystemExit();
Review comment:
OK - makes sense. I guess `DM_EXIT` would catch any new instances of System.exit(), but I'm not sure it would catch a case where a new test invokes existing code which already has a System.exit() in place. However there probably isn't much we can do about that. So I am happy with the changes here.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #2959: HDDS-6123. Disable system exit in integration tests
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2959:
URL: https://github.com/apache/ozone/pull/2959#discussion_r778173713
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/server/TestSecureContainerServer.java
##########
@@ -125,6 +127,7 @@
@BeforeClass
public static void setup() throws Exception {
DefaultMetricsSystem.setMiniClusterMode(true);
+ ExitUtils.disableSystemExit();
Review comment:
> How easy would it be for someone to add a new test class that calls some code with a System.exit() they have no idea about? Have we any other way to make this problem invisible to tests somehow?
FindBugs / SpotBugs should catch most of such direct `System.exit()` calls (`DM_EXIT` rule).
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #2959: HDDS-6123. Disable system exit in integration tests
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #2959:
URL: https://github.com/apache/ozone/pull/2959#issuecomment-1005189594
Thanks @sodonnel for the review.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org