-
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
[MINOR][TESTS] Replace JVM assert with JUnit Assert in tests #26581
Conversation
Is this all? It seems that we have more instances. |
That's all IntelliJ found in the Java test code. There may be some in non-test code, but some are possibly legitimate. There are, I think, cases where assert() is used for argument checking in non-test code and that's not really what it's for, but a separate thing. Yeah there are some in Scala tests too, I'll fix those. They're not obvious as scalatest uses assert() too, but we can look for use of scala.Predef.assert() |
Test build #114032 has finished for PR 26581 at commit
|
Test build #114027 has finished for PR 26581 at commit
|
(PS to be clear the latest test failed; its result just came first. I'll investigate) |
It seems that both Jenkins/GitHubAction fails due to
|
Test build #114040 has finished for PR 26581 at commit
|
retest this please |
Test build #114056 has finished for PR 26581 at commit
|
90fd917
to
a5b6e9e
Compare
PS @dongjoon-hyun is this failure 'normal'? https://github.com/apache/spark/runs/311900764
Just can't download maven? |
Test build #114167 has finished for PR 26581 at commit
|
Yes. It's a normal |
Test build #114174 has finished for PR 26581 at commit
|
Merged to master |
What changes were proposed in this pull request?
Use JUnit assertions in tests uniformly, not JVM assert() statements.
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. The assertion logic should be identical.
How was this patch tested?
Existing tests.