-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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
[SPARK-48386][TESTS] Replace JVM assert with JUnit Assert in tests #46698
Conversation
I think it is necessary to create a jira |
Done. |
This reverts commit a776372.
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.
+1, LGTM (pending tests)
@@ -127,8 +128,8 @@ public void testBasicLoggerWithException() { | |||
Pair.of(Level.DEBUG, debugFn), | |||
Pair.of(Level.TRACE, traceFn)).forEach(pair -> { | |||
try { | |||
assert (captureLogOutput(pair.getRight()).matches( | |||
expectedPatternForBasicMsgWithException(pair.getLeft()))); | |||
Assertions.assertTrue(captureLogOutput(pair.getRight()).matches( |
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.
Why don't we import assertTrue
?
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.
Updated.
@@ -78,9 +80,9 @@ public Predicate[] pushPredicates(Predicate[] predicates) { | |||
|
|||
Predicate[] unsupported = Arrays.stream(predicates).filter(f -> { | |||
if (f.name().equals(">")) { | |||
assert(f.children()[0] instanceof FieldReference); | |||
Assertions.assertInstanceOf(FieldReference.class, f.children()[0]); |
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.
ditto. Shall we import assertInstanceOf
?
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.
Updated.
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.
+1, LGTM.
Thank you for updating, @panbingkun . |
Merged into master for Spark 4.0. Thanks @panbingkun and @dongjoon-hyun ~ |
What changes were proposed in this pull request?
The pr aims to replace
JVM assert
withJUnit Assert
in tests.Why are the changes needed?
assert() statements do not produce as useful errors when they fail, and, if they were somehow disabled, would fail to test anything.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?
No.