Require STDIN to be specified explicitly with -#14
Require STDIN to be specified explicitly with -#14jasonkarns wants to merge 7 commits intoztombol:masterfrom
-#14Conversation
ztombol
left a comment
ztombol
left a comment
There was a problem hiding this comment.
Thanks for the additional cleanup work too!
| run echo 'a' | ||
| run refute_output <<INPUT | ||
| @test 'refute_output() - : reads <unexpected> from STDIN' { | ||
| run echo '-' |
There was a problem hiding this comment.
Why did you change the run command? As far as I can tell, it worked with run echo 'a' as well.
There was a problem hiding this comment.
As a stronger assertion that - as input on stdin doesn't cause problems. (Since the - is also the flag to the assertion)
ztombol
left a comment
ztombol
left a comment
There was a problem hiding this comment.
Few minor changes and ready to merge.
| @@ -185,12 +185,14 @@ assert_failure() { | |||
| assert_output() { | |||
There was a problem hiding this comment.
Could you update the function comment too? For example:
# Options:
# -p, --partial - partial matching
# -e, --regexp - extended regular expression matching
+# - - read expected output from the standard input
# Arguments:
-# $1 - [=STDIN] expected output
+# $1 - expected output
# Returns:
# 0 - expected matches the actual output
# 1 - otherwise| @@ -278,12 +282,14 @@ assert_output() { | |||
| refute_output() { | |||
There was a problem hiding this comment.
Could you update the function comment too? For example:
# Options:
# -p, --partial - partial matching
# -e, --regexp - extended regular expression matching
+# - - read unexpected output from the standard input
# Arguments:
-# $1 - [=STDIN] unexpected output
+# $1 - unexpected output
# Returns:
# 0 - unexpected matches the actual output
# 1 - otherwise
src/assert.bash
Outdated
| case "$1" in | ||
| -p|--partial) is_mode_partial=1; shift ;; | ||
| -e|--regexp) is_mode_regexp=1; shift ;; | ||
| -) use_stdin=1; shift ;; |
There was a problem hiding this comment.
I'm thinking a long option, e.g. --stdin, would be nice. Safe for refute_output. What do you think?
|
Updated comments and added --stdin longform option to both |
ztombol
left a comment
ztombol
left a comment
There was a problem hiding this comment.
Needs a few more simple changes and ready to go!
README.md
Outdated
|
|
||
| The expected output can be specified with a heredoc or standard input as well. | ||
| The expected output can be specified with a heredoc or standard input as well, | ||
| by providing `-` as an option. |
There was a problem hiding this comment.
Could you mention the long option in the README as well. Like we did for other options:
Partial matching can be enabled with the
--partialoption (-pfor short). When used, the assertion fails if the expected substring is not found in$output.
README.md
Outdated
|
|
||
| -The unexpected output can be specified with a heredoc or standard input as well. | ||
| The unexpected output can be specified with a heredoc or standard input as well, | ||
| by providing `-` as an option. |
There was a problem hiding this comment.
Could you mention the long option in the README as well. Like we did for other options:
Partial matching can be enabled with the
--partialoption (-pfor short). When used, the assertion fails if the expected substring is not found in$output.
| } | ||
|
|
||
| @test 'assert_output(): reads <expected> from STDIN' { | ||
| @test 'assert_output() - : reads <expected> from STDIN' { |
There was a problem hiding this comment.
Please factor out common parts of tests to avoid duplicating code. Like it's done here.
There was a problem hiding this comment.
I actually have a strong dislike of that extraction. Generally I favor non-dry tests because tests should be explicit and self-contained. In this case (and with test_r_regexp), it's quite difficult to understand what's going on with the extracted function. Instead of the test functioning as example usage, we now have variables (unnamed vars at that) being used in place of the very thing that makes the test unique.
If anything should be extracted, I'd extract the duplicated assertions from all tests:
[ "$status" -eq 0 ]
[ "${#lines[@]}" -eq 0 ]
The implementation of those statements are not germane to the test, so abstracting them away can improve readability. However, extracting away the actual invocation is a bad idea, IMO. The unique invocation is literally the raison d'être for the test(s).
There was a problem hiding this comment.
Opened a PR to this PR to show usage of just the assertion helper rather than attempting to dry up the entire test(s).
test/50-assert-15-assert_output.bats
Outdated
| run assert_output - <<STDIN | ||
| a | ||
| STDIN | ||
| echo "$output" |
test/50-assert-15-assert_output.bats
Outdated
| run assert_output --stdin <<STDIN | ||
| a | ||
| STDIN | ||
| echo "$output" |
There was a problem hiding this comment.
Can you remove this debug print left over from a previous PR?
|
Merged this branch in my fork: bats-core@658b73a |
2 participants
First step for #5