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
Allow empty_run! and reporter to display summary for empty runs #986
Conversation
I'm starting to seriously regret this addition. Damn you, @tenderlove! This PR does a few things differently:
I would think that moving down as-is would be what you really want, no? But also... Why change this for a rails test of questionable merit? Is it valuable that the test exist (or exist as-is)? Is it valuable that the summary line get printed if nothing is run? The If somehow it IS a valuable test, this seems a sufficient change to me: @@ -167,10 +167,10 @@
# might have been removed/replaced during init_plugins:
summary = reporter.reporters.grep(SummaryReporter).first
- return empty_run! options if summary && summary.count == 0
reporter.report
+ return empty_run! options if summary && summary.count == 0
reporter.passed?
end |
I'm confused why we don't change the upstream Rails test? The test name is |
Tbh, I'm also confused so I opened this PR because it seemed the right way to ask what's going on. I'm happy to follow your suggestions, but I wanted to investigate if there was a path to avoid breaking Rails tests, while also keeping the behavior you sought for. Does the original change breaks the contract with This PR also prompted me to try and add a similar thing to Rails, but this might not be the right approach, appreciate any feedback: rails/rails#51005 FWIW, here are the related test failures, I haven't looked that closely at them yet but I'd say a fair bit of them are like the assertion Aaron mentioned. Test runner tests
Test command tests
|
I also think this diff is the thing we want. I would definitely find it surprising that a custom reporter no longer gets to run because the number of tests run happened to be 0. Re: the tests in Rails, all of the failing tests in |
43897d4
to
f3f1fa8
Compare
Due to how test success is checked, the failure to run at least one test was never noticed. A change in behavior in minitest exposed the problem. See minitest/minitest#986.
This makes the minitest initializer create a dummy test so - the test code gets exercised on the first run - minitest will output the number of tests run in version 5.22 (see minitest/minitest#986) It also fixes the setup code which was now finally tested and found failing.
I tried to change the tests upstream but for some tests there is no other way to assert what we want to assert. |
This makes sense. It's what I was missing. Custom reporters should get the message. |
Done! Sorry for all the confusion. I blame @tenderlove 😛 I'll get this out in a bit... still poking at #995 |
done and released |
If we make it possible to print both messages, which fixes the rails test suite that are failing even after 5.22.2.
For example when there are no tests run, we still don't get the default Rails reporter with number of runs, asserts, benchmark, etc.
This results in failing test cases, like:
See rails/rails#50978