cnauroth commented on code in PR #7567:
URL: https://github.com/apache/hadoop/pull/7567#discussion_r2059255067
##########
hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/TestExternalCall.java:
##########
@@ -65,8 +66,12 @@ private static Configuration getConf() {
@BeforeEach
public void setup() {
- securityManager = System.getSecurityManager();
- System.setSecurityManager(new NoExitSecurityManager());
+ try {
+ securityManager = System.getSecurityManager();
+ System.setSecurityManager(new NoExitSecurityManager());
+ } catch (UnsupportedOperationException e) {
+ assumeTrue(false, "Test is skipped because SecurityManager cannot be set
(JEP 411)");
Review Comment:
Thank you for fixing this, @stoty . It seems like this will result in loss
of test coverage on JDK 24. A couple of potential ideas:
1. I think only `testCleanupTestViaToolRunner` actually depends on modifying
security manager to catch a `System.exit` call. We could push the `assumeTrue`
and all of the security manager modification down into just that method, and
the other test methods could continue running on JDK 24.
2. We could potentially modify `DistCp` so that instead of directly calling
`System.exit`, it holds a `Consumer<Integer> exitHandler`. By default, this is
set to `System::exit`, but we also have a `VisibleForTesting` constructor that
accepts an override. Tests can override it to assert on the exit code instead
of a real `System.exit`. This technique would totally remove the security
manager dependency so that all tests can run across all JDK versions.
Let me know your thoughts. Thanks!
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
