-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Cleaner, quieter test rig tests #24073
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
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!
@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.
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.
I'm not a fan of mixing named parameters with unnamed parameters.
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.
Boolean literals always get a name. Scala 2 has -Wunnamed-boolean-literal.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
Uh oh!
There was an error while loading. Please reload this page.
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.