-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
More informative output for failed CLI tests in CI #15544
Conversation
- Comparing exit code first looks confusing in CI, because we see that there is an error but not what it is. - Similarly, we wnat to see the errors before we see the difference in output (which is usually just the stdout becoming empty)
…nting the error - No need to let the exception escape and print the whole stack trace. This is an expected situation.
@@ -275,12 +275,18 @@ EOF | |||
sed -i.bak -e 's/^\(Dynamic exception type:\).*/\1/' "$stderr_path" | |||
rm "$stderr_path.bak" | |||
|
|||
if [[ $exitCode -ne "$exit_code_expected" ]] | |||
if [[ "$(cat "$stderr_path")" != "${stderr_expected}" ]] |
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.
Note that while the diff may at first look like there are more changes here, I'm really only changing the order of the if
s.
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.
Nice. Looks good to me.
check=True, | ||
) | ||
except subprocess.CalledProcessError as exception: | ||
sys.exit(exception) |
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 didn't know you could exit with an object instead of an integer code. Neat.
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 actually didn't know that myself. I forgot a string conversion here at first, then realized it still works :)
But I did know that you can pass in a string and it makes it exit with code 1 and print the message to stderr, which is a nice shortcut.
Currently
cmdlineTests.sh
compares the exit codes first, then stdout, and only last stderr. This makes CI output quite confusing, e.g. fromt_osx_cli
(1677888):Incorrect exit code. Expected 0 but got 1.
actually refers to compiler's exit code but it's not very clear here. And then we don't see what the error is.The PR fixes this by comparing stderr first, which will always show the errors if there are any. Exit code is compared last, because it's the least informative.
The PR also makes
parallel_cli_tests.py
gracefully handle test failures. It now displays a short message instead of the whole stack trace.