Show details when a test in regression.sh fails #128
Conversation
|
@ymattw please see AloisMahdal#1 for preview on how it looks. The PR includes same commit as this one, but then another one, which has only purpose of breaking the tests in order to demonstrate how failing test would look like in the pipeline. What's interesting is that while injecting the fail I stumbled upon what looks like a bug: I tried to add extra "test call" which uses Back to the main PR: There could be probably more done to make the output a bit more readable, but I see that for ydiff this will probably be challenging since it is basically binary output with all the ANSI control chars and what not. If yoyuare OK with it I could come up better solution to that. (As extra commit or separate PR.) (Eg. one strategy I used in past for similar scenario was to post-process the raw output such that it eg. includes escape chars instead of any non-ASCII, effectively making it plain-text again, and then use eg. colordiff to produce a diff which is the thing that describes the test failure. That gets rid of two problems: 1. the intended color codes get in the way of understanding what is "correct/incorrect" in the test output, 2. when a failure is precisely because of missing or ANSI chars, it's possible to show that clearly. The post-processing step is not trivial, but I suppose it could be done with few lines of Python embedded in regression.sh.) |
tests/regression.sh
Outdated
| @@ -37,16 +46,20 @@ function cmp_output() | |||
| printf "$cmd" | |||
|
|
|||
| if [[ $TRAVIS_OS_NAME == windows ]]; then | |||
| cmp_tool="diff --strip-trailing-cr -q" | |||
| cmp_tool="diff --strip-trailing-crq" | |||
There was a problem hiding this comment.
Please keep -q separated, otherwise it confuses readers.
There was a problem hiding this comment.
This is a typo, I meant to remove -q. diff with -q only says the files differ which is already implied in this context.
So all I really did here I just broke the tests for Windows. :)
tests/regression.sh
Outdated
| fi | ||
|
|
||
| if eval $cmd 2>/dev/null | eval $cmp_tool $expected_out - > /dev/null; then | ||
| if eval $cmd 2>cmd.err | tee actual.out | eval $cmp_tool $expected_out - >cmp_tool.out; then |
There was a problem hiding this comment.
I'd recommend using mktemp -d to create a temporary dir (by default in /tmp) and put all temporary files there, cleanup can be omitted.
| @@ -26,6 +26,15 @@ function fail() | |||
| fi | |||
| } | |||
|
|
|||
| function show_file() | |||
There was a problem hiding this comment.
I chose to discard the cmp results because they are not really readable - they contain ANSI escape sequences and it's hard to tell what parts differ. Maybe just print out their locations to help further debug, rather than cat them all?
There was a problem hiding this comment.
I don't think printing locations would be useful; at least in the pipeline Fedora uses the files won't be collected.
I do agree that the result of cmp in particular is not very helpful, though. What would be a better option?
It seems tricky especially in case of ydiff, where ANSI codes are subject of the test, plus the output is already diff (sometimes diff of diff). But as I mentioned in my comment to #125, I do have ideas to crack this. I just did not want to "ambush" you with too big changes in one PR :)
How about I try to come up with better way of presenting the result? It would be a bit more changes to this shell script but it might be worth it. My idea is to normalize the outputs a little bit (basically just replace ANSI with escape codes, making it true utf8 plain text while preserving the structure); and then show simply the diff -u of the normalized versions of expected vs. actual output. At that point the diff itself would be descriptive enough so that it would suffice to print just the diff and the stderr. (Perhaps with some brief message to help the reader understand the output.)
There was a problem hiding this comment.
Sure, if you found a good solution.
Keep in mind that when there is a regression, it's usually failing a lot of tests, so the result diff might be hard to read and often you will want to debug one case at a time - that's why I prefer printing out the paths of the temporary files for further looking. You are welcome to proceed your way if you do find a good balance.
Another thing is that such regressions are usually caught by unit tests first.
Don't just thow away the comparison; instead show what precisely was expected, what was produced, any stderr, and detail of the difference.
AloisMahdal
force-pushed
the
test_fail_details
branch
from
5cbdc5a to
ad0adb9
Compare
Don't just thow away the comparison; instead show what precisely was expected, what was produced, any stderr, and detail of the difference.
When tryiong to enable tests in Fedora packaging, I learned that failing tests don't report any details.
This change makes sure that if a test fails, the failure is followd by: