-
Notifications
You must be signed in to change notification settings - Fork 77
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
asserting empty output #5
Comments
Running If we do this, we should find a way to stay clear of solutions that may break in misterios ways. |
I don't have much of an opinion on this other than to say, I've seen a fair amount of usage of Even ignoring how we check for STDIN, should Verbally, this sounds reasonable. I just want to make sure we're fully aware of the ramifications, despite the implementation detail. After thinking it through fully, I still like the proposal. |
I like the new interface of
I think it's rather elegant and intuitive. Implementation wise I think we may run into a few unexpected problems outside of SSH. Docker (and likely other) containers often doesn't have a TTY allocated for STDIN/STDOUT. And I'm not sure, but the new container-based Travis CI may also not allocate a TTY (which causes bats to automatically use TAP output). I'll test Travis tomorrow and report back. |
I ran a test and found that Travis CI does connect a TTY to STDIN. See this quick and dirty test here. You're right. SSH's behaviour is not our concern. I was more worried about containers, but I missed that in my first post (long day...). I did ran into problems when trying to run a script that needed to know whether STDIN is a TTY or not. It was quite a while ago so I can't remember why I implemented it the way I did. I'll try to run some tests in docker on the weekend. Though if the problem only effects special use cases (e.g. SSH), we can always add a warning in the documentation. |
@jasonkarns I ran the docker tests. They confirm that a TTY is only allocated when explicitely requested (
I think we should add a warning to the documentation that if the test suite must be able to run without a TTY, e.g. in a docker container, then I'm trying to find a way to get around this, but so far my solutions are very hacky and, I suspect, not portable at all. Haven't given up yet though. |
I'll throw up a PR at some point. Disappointing that we'll have that weird behavior under containers, but I think the consistency and phrasing are worth it, considering how (seemingly) rare the container issue will be. |
I like the new syntax, but I'm wondering if people would avoid it entirely to make sure their test suite doesn't fail mysteriously in some cases. Can we make piping/redirection explicit instead?
This would work around the TTY problem. Of course the actual option name could be anything, e.g. to follow convention. Why an option? Why not just |
Here's a rough version of how that could look like: assert_output() {
local -i is_mode_partial=0
local -i is_mode_regexp=0
local -i is_mode_any=0
local -i is_stdin=0
# Handle options.
while (( $# > 0 )); do
case "$1" in
-p|--partial) is_mode_partial=1; shift ;;
-e|--regexp) is_mode_regexp=1; shift ;;
-i|--stdin) is_stdin=1; shift ;;
--) break ;;
*) break ;;
esac
done
if (( is_mode_partial )) && (( is_mode_regexp )); then
echo "\`--partial' and \`--regexp' are mutually exclusive" \
| batslib_decorate 'ERROR: assert_output' \
| fail
return $?
fi
# Arguments.
local expected
if (( is_stdin )); then
expected="$(cat -)"
else
if (( $# != 0 )); then
expected="$1"
else
is_mode_any=1
# TODO: mutually exclusive: any && ( partial || regexp )
fi
fi
# Matching.
if (( is_mode_any )); then
if [[ $output == '' ]]; then
echo 'Some output was expected, but there was none found' \
| batslib_decorate 'no output' \
| fail
return $?
fi
return 0
fi
if (( is_mode_regexp )); then
... |
Why wouldn't |
@jasonkarns Actually, you can assert output identical to one of the options by using And as I typed this I realised that we can use # Handle options.
while (( $# > 0 )); do
case "$1" in
-p|--partial) is_mode_partial=1; shift ;;
-e|--regexp) is_mode_regexp=1; shift ;;
-) is_stdin=1; shift ;;
--) shift; break ;;
*) break ;;
esac
done I did a quick test and it works (see the snippet below). This avoids the TTY problem and uses
@juanibiapina You added standard input handling back in. What do you think about the new interface? Use this code snippet to test the new interface. #!/usr/bin/env bash
assert_output() {
local -i is_mode_partial=0
local -i is_mode_regexp=0
local -i is_mode_any=0
local -i is_stdin=0
echo "-- DEBUG: assert_output $@"
# Handle options.
while (( $# > 0 )); do
case "$1" in
-p|--partial) is_mode_partial=1; shift ;;
-e|--regexp) is_mode_regexp=1; shift ;;
-) is_stdin=1; shift ;;
--) shift; break;;
*) break ;;
esac
done
# Arguments.
local expected
if (( is_stdin )); then
expected="$(cat -)"
else
if (( $# != 0 )); then
expected="$1"
else
is_mode_any=1
# TODO: mutually exclusive: any && ( partial || regexp )
fi
fi
# Matching.
echo "expected : $expected"
echo "any : $( (( is_mode_any )) && echo yes || echo no )"
echo "stdin : $( (( is_mode_stdin )) && echo yes || echo no )"
echo "partial : $( (( is_mode_partial )) && echo yes || echo no )"
echo "regexp : $( (( is_mode_regexp )) && echo yes || echo no )"
echo '--'
echo
}
# match any output
assert_output
# read <expected> from argument
assert_output 'expected'
# read <expected> from stdin
assert_output - <<EOF
expected
EOF
# match string identical to one of the options
assert_output -- -e |
I think it makes a lot of sense, thanks for asking. |
👍 |
@jasonkarns Have you started on this? I was thinking of changing the input selection ( |
This has been resolved in my fork as of: bats-core@44c1c08 |
assertion api question, when attempting to assert that output is empty.
I find
assert_output ""
rather distasteful, do you? I would prefer to sayrefute_output
(since I'm asserting that any output at all is a failure) Of course,refute_output
without an arg is triggering STDIN comparison. Should it be checking for[ -t 0 ]
instead of arg counting? That would allow empty comparisons.Thoughts?
The text was updated successfully, but these errors were encountered: