Skip to content
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

Open
jasonkarns opened this issue Feb 15, 2016 · 14 comments
Open

asserting empty output #5

jasonkarns opened this issue Feb 15, 2016 · 14 comments

Comments

@jasonkarns
Copy link
Contributor

assertion api question, when attempting to assert that output is empty.

I find assert_output "" rather distasteful, do you? I would prefer to say refute_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?

@ztombol ztombol mentioned this issue Feb 16, 2016
@ztombol
Copy link
Owner

ztombol commented Feb 16, 2016

Running refute_output to assert that no output is made would be very elegant. But we need a solid implementation that detects if the output is comming from STDIN or an argument. I had to do this once, and found in a StackOverflow thread that it's virtually impossible do foolproof in Bash.

If we do this, we should find a way to stay clear of solutions that may break in misterios ways.

@ztombol ztombol mentioned this issue Feb 16, 2016
@jasonkarns
Copy link
Contributor Author

I don't have much of an opinion on this other than to say, I've seen a fair amount of usage of [ -t 0 ] within bats test suites (mostly rbenv et. al.). Add to that, the fact that -t is POSIX, the weirdness that SSH seems to be displaying sounds counter to POSIX so I'm less inclined to bend to patch a non compliant behavior.

Even ignoring how we check for STDIN, should refute_output with no args be the same as assert_output "" ? The complement would then be that: refute_output "" is equivalent to assert_output.

Verbally, this sounds reasonable. assert_output with no args reads to me like "assert there is some output". But, OTOH, I can see confusion arising since it would mean that
assert_output "" and assert_output are exact opposites. (And likewise, refute_output != refute_output "")

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.

@ztombol
Copy link
Owner

ztombol commented Feb 17, 2016

I like the new interface of assert_output and refute_output.

proposed old alternative
any output assert_output refute_output ''
no output refute_output assert_output ''

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.

@ztombol
Copy link
Owner

ztombol commented Feb 18, 2016

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.

@ztombol
Copy link
Owner

ztombol commented Feb 22, 2016

@jasonkarns I ran the docker tests. They confirm that a TTY is only allocated when explicitely requested (-t). This is what the documentation says actually.

cat - could hang or report error (stdin is closed) when a TTY is not available.

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 assert_output ''/refute_output '' should be used to test for empty output.

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.

@jasonkarns
Copy link
Contributor Author

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.

@ztombol
Copy link
Owner

ztombol commented Feb 24, 2016

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?

-i, --stdin - read expected output from the standard input
  • assert_output: there's output
  • assert_output <expected>: "$output" matches
  • assert_output -i <<HEREDOC and ... | assert_output -i: read from STDIN

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 -? Some unix utilities that work with files, e.g. cat, use - to specify that input is coming from the standard input. And a file whose name is actually - can be specified as ./-. This would not work in our case.

@ztombol
Copy link
Owner

ztombol commented Feb 24, 2016

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
    ...

@jasonkarns
Copy link
Contributor Author

This would not work in our case.

Why wouldn't - work in our case? It would prevent asserting that $output is exactly -, but with the current implementation, we couldn't assert that $output is exactly -i or --stdin, either. Given that the limitation exists regardless which flag we choose, why not go with the conventional one? Unless I'm missing another reason why it wouldn't work.

@ztombol
Copy link
Owner

ztombol commented Feb 26, 2016

@jasonkarns Actually, you can assert output identical to one of the options by using -- to stop parsing options (which was broken until just now). For example to assert that the output is -i, one would use assert_output -- -i.

And as I typed this I realised that we can use - if we handle it as an option, rather than as an argument (as utilities like cat do). 😄

# 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 - to read from STDIN just like other utilities.

  • assert_output -- assert any output
  • assert_output <expected> -- output matches <expected>
  • assert_output -- <expected> -- output matches <expected> that's identical to an option
  • assert_output - <<HEREDOC ... -- output matches <expected> read from STDIN

@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

@juanibiapina
Copy link
Contributor

I think it makes a lot of sense, thanks for asking.

@jasonkarns
Copy link
Contributor Author

👍

@ztombol
Copy link
Owner

ztombol commented Mar 12, 2016

@jasonkarns Have you started on this? I was thinking of changing the input selection (-i to -). No pressure! Just want to avoid causing conflicts. 😄

@jasonkarns
Copy link
Contributor Author

This has been resolved in my fork as of: bats-core@44c1c08

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants