-
Notifications
You must be signed in to change notification settings - Fork 117
[feat] Print failure statistics table as well as the run options for failures #1176
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
[feat] Print failure statistics table as well as the run options for failures #1176
Conversation
|
Hello @ZQyou, Thank you for updating! Cheers! There are no PEP8 issues in this Pull Request!Do see the ReFrame Coding Style Guide Comment last updated at 2020-03-18 16:20:06 UTC |
|
Can I test this patch? |
Codecov Report
@@ Coverage Diff @@
## master #1176 +/- ##
==========================================
- Coverage 91.92% 91.66% -0.27%
==========================================
Files 84 84
Lines 12209 11850 -359
==========================================
- Hits 11223 10862 -361
- Misses 986 988 +2
Continue to review full report at Codecov.
|
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.
Hi @ZQyou, thanks for your PR. I have some general comments first.
- I think that the description in the summary table is redundant; it just explains what each phase is.
- I would call this part as "Failure Statistics" and move it to a separate function from the standard summary report. A command line option could control whether the statistics are printed or not.
- Instead of printing the command line options to rerun each test individually, I think it'd be better to provide different groupings in the statistics, e.g., (failures by programming environment, by system partition, by phase etc) and provide the failed tests as
|-separated list that could be passed to the-ncommand line option. Something like the following:
Total number of tests: 6
Total number of failures: 5
Phase # Failing tests
----------- ----- ------------------------------------------------------------
setup 2 BadSetupCheck|BadSetupCheckEarly
sanity 1 SanityFailureCheck
performance 1 PerformanceFailureCheck
cleanup 1 CleanupFailTest
You could produce similar summaries per programming environment and per system partition as well.
* Add failure_stats function to print failure statistics * Add "Rerun as:" in failure report for run options
|
@vkarak I have made changes according to your suggestions. In my tests with release 2.21 and this branch, I found:
I think one of these issues has been fixed in pre-release but need more work? Here is the last part of a log file from working directory: |
|
@ZQyou Thanks for the updating the PR. I've read your comments, but I didn't have the time to get back to you. I'll review your PR and get back to you asap. |
I haven't seen that with some runs using this branch. Perhaps, it's a more general issue and it could reserve a separate issue if you have a reproducer.
That could also be a problem. If you have a reproducer it would help.
I haven't seen something related being merged since 2.21, so the bug, if it is one, may still be around... |
|
As a final step, we will also need a unit test in |
* Print test cases in the table * Add unittest for option `--failure-stats
vkarak
left a comment
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.
Thanks @ZQyou for updating the PR. Looks good now. I have just a couple of minor comments, which I could also address if you don't have time.
vkarak
left a comment
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.
Unit tests are failing. I will do the small fix necessary.
vkarak
left a comment
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.
lgtm now. Thanks @ZQyou for the PR.
|
ok to test |
Fixes #1161.