Cleaner, quieter test rig tests #24073
Conversation
|
Thanks for taking care of this @som-snytt. One less item out of my TODO list! |
|
The longish thread where we were confused. |
|
@hamzaremmal I'm glad to contribute when I can (without making extra work); if there is an actual todo list, let me know! |
YES, I have a long list of things that should be done. Some are urgent and some are not. It doesn't look like a list but it is known internally as my post-it screen. PS: I will review this by Sunday. I'm focusing on studies today. |
| */ | ||
| def expectFailure: CompilationTest = | ||
| new CompilationTest(targets, times, shouldDelete, threadLimit, true, shouldSuppressOutput) | ||
| new CompilationTest(targets, times, shouldDelete, threadLimit, shouldFail = true, shouldSuppressOutput) |
There was a problem hiding this comment.
I'm not a fan of mixing named parameters with unnamed parameters.
There was a problem hiding this comment.
Boolean literals always get a name. Scala 2 has -Wunnamed-boolean-literal.
|
|
||
| import scala.concurrent.duration._ | ||
| import scala.util.control.NonFatal | ||
| import org.junit.{Test as test, AfterClass as tearDown} |
There was a problem hiding this comment.
I find that confusing. Aliases here makes the code harder to follow: each time someone sees @test or tearDown, they’ll need to check the import to realize these are actually JUnit’s @Test and @AfterClass.
There was a problem hiding this comment.
I borrowed that idiom from bishabosha, I think. I just saw it recently and fell in love. However, I am not wedded to it (at present).
Add noise suppression. Refactor to modern facilities.
Why was the ticket worth tackling?
It didn't have a ticket, but there was confusion on a ticket about the noisy output. That is a waste of time, in a context where the build is in flux.
I wanted to ensure that my recent fix to how lines are counted by the test rig didn't break something. (There was just one semantic merge conflict, for a check file.)
How I fixed it
I finally noticed that the local test run correctly summarizes no failures, even though the individual test echoed its failure state.
Then I noticed that the rig already has a flag for noise suppression, so I added that incrementally.
Since the unit test file is not large, it was expedient to refactor it to look more modern, but without other behavior changes.
Why is this PR worth reviewing?
It improves readability of the test rig's unit test, but avoids futzing with any operational semantics such as threading.
Removing the noisy output helps when scrolling through copious C.I. output, where it looks like an error.
What's the worse that could happen?
The build breaks in subtle ways.