System tests: assert(): friendlier failure messages #17538
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-ci
bot
added
release-note-none
approved
|
@Luap99 PTAL. I encourage you to recreate the problems you were seeing (if that's easy) and see if the new error message is easier to understand. |
edsantiago
force-pushed
the
system_test_assert
branch
2 times, most recently
from
0f95309 to
9044559
Compare
...safer, too: the big change is using 'mapfile' to split multiline strings; this preserves empty lines, making it easy to see spurious (or missing) blank lines in output. Another change is to indent the expected-output string consistently, for readability. Then, to handle \r (CR) and other control characters, use bash %q to format special chars. But %q makes\ it\ hard\ to read\ lines\ with\ spaces, so strip off those backslashes. This makes assert() much larger and uglier, but this is code that shouldn't be touched often. Finally, because these are big changes to critical code, write a complicated regression test suite for assert(). Signed-off-by: Ed Santiago <[email protected]>
edsantiago
force-pushed
the
system_test_assert
branch
from
9044559 to
c81fbd5
Compare
Luap99
approved these changes
Feb 17, 2023
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, Luap99, vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
...safer, too: the big change is using 'mapfile' to split
multiline strings; this preserves empty lines, making it
easy to see spurious (or missing) blank lines in output.
Another change is to indent the expected-output string
consistently, for readability.
Then, to handle \r (CR) and other control characters, use
bash %q to format special chars. But %q makes\ it\ hard\ to
read\ lines\ with\ spaces, so strip off those backslashes.
This makes assert() much larger and uglier, but this is
code that shouldn't be touched often.
Finally, because these are big changes to critical code,
write a complicated regression test suite for assert().
Closes: #17509
Signed-off-by: Ed Santiago [email protected]