Improve escaping in unittest failure message #320
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
daf5137 fixed a bug where the failure message would not be printed correctly if it included values subject to shell variable expansion (e.g. "$FOO") by quoting the limit string for the heredoc the message ("cat <<'EOF'"). However, it still left an issue if that limit string appeared on its own line in a message, which would happen if a compared value included "\nEOF\n". (Plausible if the function under test generated shell script code.) This could be worked around by choosing a more unusual string, but that remains an odd implementation detail. Instead, the shell implementation could use the same path of splitting the output into lines, echoing each line, and backslash-escaping each character. The previous code also did not split the error output into lines correctly if individual failure messages contained newlines (which could happen when comparing values where at least one was a string containing a newline; the code that generates the error message converts with "%s" (str)). This matters if those lines are joined with something other than "\n", which was the case in the Windows implementation (and now is the case with both). The Windows implementation also did not avoid variable expansion by escaping "%" as "%%" if the error message included "%".
bazel-io
pushed a commit
to bazelbuild/bazel
that referenced
this pull request
Oct 1, 2021
Currently, the failure message does not output correctly if it contains sequences subject to shell variable expansion (e.g. `$1`) or the limit string used to denote the end of the message (in this case `\nEOF\n`). Those are quite plausible edge-cases, since analysistest targets might assert about the command-line of actions generated with `ctx.actions.run_shell` or the contents of shell files generated with `ctx.actions.write`. This uses the approach of echoing by line and escaping characters as necessary for both versions. In the sh case, we can baskslash-escape every character. For Windows, `%` is escaped as `%%`. (Other special characters don't seem to cause trouble, though I'm probably missing relevant cases.) The heredoc style `cat<<EOF\n[...]\nEOF\n` can have shell-escaping disabled by quoting the limit-string (`cat<<'EOF'`) or preceding it with a backslash (`cat<<\EOF`). But that still relies on choosing a limit string that doesn't occur in the contents, and there's no way (whether or not shell expansion is done) of escaping the limit string if it does. While one could choose a more obscure limit string, that would add another hidden implementation detail. Alternately, this could add logic to choose a limit string that doesn't occur in the message, but that seems unnecessarily complex. Another approach would be to write the message to a file, then read it with cat/type. However, that requires either splitting the change between Starlark and Java code with separate release cycles (awkward), or doing something to insert the message file in the test's runfiles at the last minute (more complicated). Adds some new test-cases which fail without this change. Removes an unused return value to make the usage of this function a bit clearer. bazelbuild/bazel-skylib#320 for the corresponding change for unittest. RELNOTES: None. PiperOrigin-RevId: 400309738
tetromino
reviewed
Oct 4, 2021
tetromino
approved these changes
Oct 4, 2021
|
Hey @tetromino I've noticed the same thing, that you have to notice when a PR changes something which causes docs to be out-of-date. I bet there's already skew between docs and the docstrings. I think we should just have a I'll send a PR |
alexeagle
added a commit
to alexeagle/bazel-skylib
that referenced
this pull request
Oct 9, 2021
it prints a convenient 'bazel run' command to update them, replacing the shell script Follow-up to bazelbuild#320 (comment)
alexeagle
added a commit
to alexeagle/bazel-skylib
that referenced
this pull request
Oct 9, 2021
it prints a convenient 'bazel run' command to update them, replacing the shell script Follow-up to bazelbuild#320 (comment)
|
Oh, sorry I forgot we already discussed that PR in #297 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
daf5137 fixed a bug where the failure message would not be printed
correctly if it included values subject to shell variable expansion
(e.g. "$FOO") by quoting the limit string for the heredoc with the message
("cat <<'EOF'"). However, it still left an issue if that limit string
appeared on its own line in a message, which would happen if a compared
value included "\nEOF\n". (Plausible if the function under test
generated shell script code.) This could be worked around by choosing
a more unusual string, but that remains an odd implementation detail.
Instead, the shell implementation could use the same path of splitting
the output into lines, echoing each line, and backslash-escaping each
character.
The previous code also did not split the error output into lines
correctly if individual failure messages contained newlines (which
could happen when comparing values where at least one was a string
containing a newline; the code that generates the error message
converts with "%s" (str)). This matters if those lines are joined
with something other than "\n", which was the case in the Windows
implementation (and now is the case with both).
The Windows implementation also did not avoid variable expansion
by escaping "%" as "%%" if the error message included "%".