-
Notifications
You must be signed in to change notification settings - Fork 428
Adding bats_pipe
Helper Function (originally suggested adding --output-processor
Flag to run
Helper).
#663
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
Adding bats_pipe
Helper Function (originally suggested adding --output-processor
Flag to run
Helper).
#663
Conversation
Thanks for your contribution. I think a better approach would be a helper function that encapsulates the piping. This would be composible with Here is a short example of how I would envision this:
The |
Thanks for the quick review! |
I think allowing for arbitrary number of pipes is easiest to solve via recursion. For a first try we can do with a single pipe that can be nested manually:
|
--output-processor
Flag to run
Helper.bats_pipe
Helper Function (formerly --output-processor
Flag to run
Helper).
bats_pipe
Helper Function (formerly --output-processor
Flag to run
Helper).bats_pipe
Helper Function (originally suggested adding --output-processor
Flag to run
Helper).
I've updated this PR (code, documentation, and PR description). Now implemented using separate This implementation does support any number of piped commands, e.g.: |
After thinking a bit more about this, I noticed that there might be an even easier solution: eval. Passing the parameters including pipes straight into eval should work. The only problems I see are
|
IMO, |
Huh?
Maybe I haven't been bitten by
I see the following advantages:
|
This is how GitHub wanted to format a reply. I do not know. 🤷 |
To comment here on I think that a testing library should help authors write good tests, prevent them from writing bad tests (and be clear why), and help them diagnose when a test fails. If I were to write a test and I saw I would have to dig through the test library (bats), not even my tests, to figure out what I did wrong. This sort of thing costs developer time and is a good way to get them to not write tests (or use any given library). I'd much favor keeping the explicit checks that are in place to give human-readable feedback, e.g. If this explicit checking is done, as well as the With all that said, I can try and implement this Thanks! |
Fair enough. Maybe we can replace the eval part of this message with bats_pipe and be done. Most of the relevant context should come from
The pipestatus should be pretty lightwheight, something like:
Thanks for going the extra mile. :) |
81dc7e9
to
1c32654
Compare
1c32654
to
5876f94
Compare
I did not fix those shellcheck messages that stem from unreachable code due to the different implemetations for bats_pipe. |
5876f94
to
d8ece7e
Compare
Hey, I am very sorry about this loooong delay. 😅 I've been on paternity leave and spending time with my family (first kid). I went ahead and flushed out the eval implementation (we pretty much were thinking the same thing). I also went ahead and removed the other implementations. Let's go ahead and get this PR finally merged in, I've taken long enough (again, really sorry). FWIW, while I was gone, I thought more about eval and realized that it makes the best sense to use. There could be a lot of functionality that a test author could put into bats, and giving consistency with the rest of bash is a great priority -- and not reinventing it ourselves (via usage of eval) is the best way to go. Feel free to give any other feedback on the current version of the PR and I'll be sure to update on Monday (or asap otherwise). Sorry again and thank you for your patience and friendliness! 😄 |
d5a7d1f
to
2aaee72
Compare
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.
No worries, I was absent myself.
My suggestions are mainly cosmetic/minor changes. Good job so far, especially on the documentation and extensive tests.
local -a escaped_args=() | ||
local pipe_count=0 | ||
local previous_pipe_index=-1 | ||
for (( index = 0; index < $#; index++ )); do |
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.
for (( index = 0; index < $#; index++ )); do | |
local -i index=0 | |
for (( index = 0; index < $#; index++ )); do |
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.
Done. Also updated other integer type declarations.
lib/bats-core/test_functions.bash
Outdated
printf "Usage error: Cannot have leading \`\\|\`.\n" >&2 | ||
return 1 | ||
fi | ||
if [ "$(( previous_pipe_index + 1 ))" -ge "$index" ]; then |
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.
if [ "$(( previous_pipe_index + 1 ))" -ge "$index" ]; then | |
if (( previous_pipe_index + 1 >= index )); then |
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.
Done.
lib/bats-core/test_functions.bash
Outdated
escaped_args+=("$escaped_arg") | ||
done | ||
|
||
if [ "$previous_pipe_index" -gt 0 ] && [ "$previous_pipe_index" -eq "$(( $# - 1 ))" ]; then |
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.
if [ "$previous_pipe_index" -gt 0 ] && [ "$previous_pipe_index" -eq "$(( $# - 1 ))" ]; then | |
if (( previous_pipe_index > 0 && previous_pipe_index == $# - 1 )); then |
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.
Done.
lib/bats-core/test_functions.bash
Outdated
return 1 | ||
fi | ||
|
||
if [ "$pipe_count" -eq 0 ]; then |
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.
if [ "$pipe_count" -eq 0 ]; then | |
if (( pipe_count == 0 )); then |
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.
Done.
lib/bats-core/test_functions.bash
Outdated
return 1 | ||
fi | ||
|
||
if [ "$pipestatus_position" -gt "$pipe_count" ]; then |
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.
if [ "$pipestatus_position" -gt "$pipe_count" ]; then | |
# there will be pipe_count + 1 entries in PIPE_STATUS | |
# valid indices are [0; pipe_count] and [-pipe_count-1; -1] | |
if (( pipestatus_position > pipe_count || pipestatus_position < -pipe_count - 1 )); then |
lib/bats-core/test_functions.bash
Outdated
if (( pipestatus_position < 0 )); then | ||
# if we are performing default "last failure" behavior, | ||
# iterate backwards through pipe_status to find the last error. | ||
result_status=0 | ||
for index in "${!__bats_pipe_eval_pipe_status[@]}"; do | ||
# OSX bash doesn't support negative indexing. | ||
local backward_iter_index="$((${#__bats_pipe_eval_pipe_status[@]} - index - 1))" | ||
local status_at_backward_iter_index="${__bats_pipe_eval_pipe_status[$backward_iter_index]}" | ||
if (( status_at_backward_iter_index != 0 )); then | ||
result_status="$status_at_backward_iter_index" | ||
break; | ||
fi | ||
done | ||
else | ||
result_status="${__bats_pipe_eval_pipe_status[$pipestatus_position]}" | ||
fi |
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.
if (( pipestatus_position < 0 )); then | |
# if we are performing default "last failure" behavior, | |
# iterate backwards through pipe_status to find the last error. | |
result_status=0 | |
for index in "${!__bats_pipe_eval_pipe_status[@]}"; do | |
# OSX bash doesn't support negative indexing. | |
local backward_iter_index="$((${#__bats_pipe_eval_pipe_status[@]} - index - 1))" | |
local status_at_backward_iter_index="${__bats_pipe_eval_pipe_status[$backward_iter_index]}" | |
if (( status_at_backward_iter_index != 0 )); then | |
result_status="$status_at_backward_iter_index" | |
break; | |
fi | |
done | |
else | |
result_status="${__bats_pipe_eval_pipe_status[$pipestatus_position]}" | |
fi | |
if [[ -n $pipestatus_position ]]; then | |
if (( pipestatus_position < 0 )); then | |
pipestatus_position=$((pipestatus_position + ${#__bats_pipe_eval_pipe_status[@]} )) | |
fi | |
result_status="${__bats_pipe_eval_pipe_status[$pipestatus_position]}" | |
else | |
result_status=0 | |
# use last non zero value | |
for val in ${__bats_pipe_eval_pipe_status[@]}; do | |
if (( val != 0)); then | |
result_status=$val | |
fi | |
done | |
fi |
lib/bats-core/test_functions.bash
Outdated
# Supplying -N (e.g. -0) will instead always use the exit code of the command | ||
# at that position in the chain. | ||
|
||
local pipestatus_position=-1 |
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 suggest adding the possibility to use negative values like --1
to always use the last status, --2
next to last status, and so on
local pipestatus_position=-1 | |
local pipestatus_position= |
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 suppose negative indexing (as used in linux's bash, python, etc) would be an interesting option. But the suggested syntax is quite peculiar. I don't think I've seen negative values used outside args to other parameters (e.g. --num -5
, -X=-100
, etc) or as a positional argument (e.g. some_command -a -f -- -2 -50 -100
). Using negative values as optional arguments confuses the meaning of -
and becomes more confusing when considering unix style arguments (where --
is for long parameters).
WDYT about adding in a separate argument that allows for negative values? Something like --returned-status=N
(where N
is a positive or negative value). The -N
syntax would just be a shorthand (for positive values).
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've gone ahead and added in --returned-status
(which supports positive and negative values). I've also added in tests for this. Let me know what you think.
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.
Please also add this to the online documentation in the .md files.
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.
Updated documentation.
test/bats_pipe.bats
Outdated
######### | ||
|
||
@test "run bats_pipe with no commands" { | ||
run bats_pipe |
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.
Some general hints for all tests here:
- use the
-1
switch to do inline testing - avoid
echo $output
, there is--print-output-on-failure
for that and if run fails due to an exit code check, it should also print$output
(in future releases)
- also, stderr and stdout is not differentiated in bats output, so no need to redirect
- (maybe) place the line count check last, to get more localized error messages first
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.
use the -1 switch to do inline testing
No problem with changing [ "$status" -eq 1 ]
to instead use run -1 blah
.
... but I'm a little curious around intended flow and interplay with extension libs. My usage of [ "$status" -eq 1 ]
to to mimic the functionality of bats-assert's assert_failure
command (or assert_success
).
I happen to use bats-assert for my projects as I find it as a very nice addition to bats. Obviously, I'm not using it here as it would present a project circular dependency. However, there is this overlap of functionality with run -N
. I'm curious about intention over that. If it's as simple as "authors could use whichever" and "run -N
is less verbose than [ "$status" -eq N ]
", then that's cool.
FWIW, I like [ "$status" -eq N ]
or assert_failure N
as it's easier to read what the test is doing (run -N
is a bit more opaque when reading).
Sorry for asking and dragging out conversation. 😅
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.
avoid echo $output
Done.
test/bats_pipe.bats
Outdated
[ "${lines[0]}" = 'Will return status 4.' ] | ||
} | ||
|
||
@test "run bats_pipe for Nth error status too large" { |
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.
If we added support for negative indices above, please add tests for the range check and actual return code position here.
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.
Added tests for --returned-status
as discussed in other comment thread.
Allows for easily processing output from the command being run, and properly propagate status.
…mmand, `bats_pipe`.
…ace (maybe copied from `run`).
* Working out a few issues and clarifying values. * Adding comments to clarify flow. * Leaving previous functions for the time being for comparison.
* Fixing disable code.
* Adding comment to detail unquoted var with disable.
…debug. * Adding test cases for parameters with spaces. * Switching out hexdump for the (more widely available) od. * Updating assertions to match od output.
* Cleaning up / consolidating / simplifying bats_pipe function.
* Moving if conditions to use c-style parenthesis construct. * Using explicit parenthesis to not rely upon operator precedence. * Changing eval array expansion to use `[@]` instead of `[*]`. * Likely not a big impact in this exact use case, but this is the typical syntax for desired behavior -- less likely to have undesired surprises.
…r than negative value.
* Supports positive and negative values. * Negative values count from the end in reverse, similar to (linux's) bash and python. * Can be used as either `--return-status N` or `--return-status=N`. * Adding tests to validate `--return-status` functionality.
9e93bfa
to
5752471
Compare
This PR allows for easily processing output from the command being run, and properly propagate status.
When looking up how to use pipes with
run
, a number of "work arounds" are mentioned, although it seems that none fully handle the core intention of processing output.One mentioned work around (e.g.
run grep "item" <(command_to_run)
) would seem to suggest that the testing focus is on the functionality ofgrep
and not the command. If that is any such tests purpose, that's great. However, if one wanted to focus on the functionality ofcommand_to_run
, withgrep
as an aside, there is no "easy" way to do this.A different alternative shows the usage of
bash -c
to handle piping (run bash -c 'command_to_run | grep "item"'
). However, this swallows the exit code of the command of interest,command_to_run
. Proper preservation would require pulling it fromPIPESTATUS
(e.g.run bash -c 'command_to_run | grep "item" ; exit "${PIPESTATUS[0]}"'
). This would properly set the globalstatus
variable to the exit code fromcommand_to_run
. One could wrap this functionality in a function, but its still quite bloated. Arguably, requiring each test to manually handle this is brittle and complicates the test -- I have not actually seen this suggested in any of the threads that I have been searching through (even though those sample tests are still asserting onstatus
, which seems brittle/misleading/lack of coverage).As per discussion below, this PR adds a new helper
bats_pipe
, which is able to parse\|
to allow piping of multiple commands.A simple test to illustrate this is as follows:
This functionality is especially helpful when testing an executable/script/etc which outputs binary data into stdout. This can be shown as follows:
New tests have been added to cover a wide variety of usage for this new helper function, see
tests/bats_pipe.bats
.Original text of this PR (which adding `--output-processor` flag to `run`). No longer applicable, but kept for context.
In an effort to encapsulate this into the `run` command directly, this PR adds a new flag, `--output-processor` which allows specifying an "output processing command" to transform output as the author sees fit. It handles the command of interest separately from this extra output processing. I implemented this in such a way to allow the test author to handle/process output however was best for them, and not be too prescriptive/presumptive for how this functionality best be done exactly.A simple test to illustrate this is as follows:
This functionality is especially helpful when testing an executable/script/etc which outputs binary data into stdout. This can be shown as follows:
These tests are included in this PR (added to 'tests/run.bats').
It is important to note that, if
--output-processor
is not supplied, command execution inrun
is completely unchanged (aside from the check foroutput_processor
).This expansion of functionality of
run
is also very helpful when using bats extension libraries, like bats-assert, which makerun
very helpful and does a great job of making straight forward, readable, and powerful tests.Files changed in the PR were based mostly on the changes in f69c26d. If there's something else that needs to be updated, I'm happy to.
If you have any questions, suggestions, or other feedback, please do let me know. I would also like to thank you all for creating/maintaining this repo. I've made great use of it over a number of years, and am very glad to have another great tool to easily write tests. Thanks!
Related resources: sstephenson/bats#10, https://bats-core.readthedocs.io/en/stable/writing-tests.html, https://bats-core.readthedocs.io/en/stable/gotchas.html#my-piped-command-does-not-work-under-run