HDFS-17828. SocketTimeoutException not recognized in TestWebHdfsTimeouts with Java 24 #7955
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
PTAL @slfan1989 |
slfan1989
approved these changes
Sep 15, 2025
LGTM |
steveloughran
requested changes
Sep 15, 2025
| throw new AssertionError(E_NULL_THROWABLE_STRING, t); | ||
| } | ||
| if (pattern != null && !pattern.matcher(msg).matches()) { | ||
| String prefix = org.apache.commons.lang3.StringUtils.isEmpty(message) |
There was a problem hiding this comment.
our own StringUtils.hasLength(message) does this. silly name but at least unique.
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/GenericTestUtils.java
Show resolved
Hide resolved
| Throwable t, | ||
| String message) { | ||
| assertNotNull(t, E_NULL_THROWABLE); | ||
| String msg = t.toString(); |
There was a problem hiding this comment.
rename to caughtString; as msg means "message" to me, and it isn't as the code doesn't use getMessage(). (which I'm happy with not using)
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/GenericTestUtils.java
Show resolved
Hide resolved
That is not relevant as the method already expects a Pattern, not a String.
Added the other tests. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Can you take another look @steveloughran ? If you prefer, I can make your suggested changes in both the old and new method, but I think they should be be as similar as possible. |
steveloughran
requested changes
Sep 24, 2025
There was a problem hiding this comment.
commented; looks like you aren't that familar with our intercept() method yet -it's how we assert operations raise exceptions
- returns the caught ex for further asserts
- if the exception wasn't raised, and the lambda expression returned something, the string value of that is included in the text
based on ScalaTest.intercept()'s design
|
|
||
| @Test | ||
| public void testAssertExceptionContainsNullEx() throws Throwable { | ||
| boolean assertTriggered = false; |
There was a problem hiding this comment.
use LambdaTestUtils.intercept here.
intercept(AssertionError.class, E_NULL_THROWABLE, () -> assertExceptionContains("", null));
same below.
| } catch (AssertionError e) { | ||
| assertTriggered = true; | ||
| String s = e.toString(); | ||
| if (!s.contains(E_UNEXPECTED_EXCEPTION) |
There was a problem hiding this comment.
again, use intercept, but here
AssertionError e = intercept(AssertionError.class, "", () ->
assertExceptionContains("Expected", new Exception("(actual)")));
// checks from L32 onwards
String s = e.toString();
if (!s.contains(E_UNEXPECTED_EXCEPTION) ...
|
Done @steveloughran |
|
💔 -1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
Fixes a test issue where TestWebHdfsTimeouts exepects "connect timed out", but Java 24 returns "Connect timed out"
in the exception message.
How was this patch tested?
Ran the test with Java 24
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?