Skip to content
This repository has been archived by the owner on Apr 29, 2021. It is now read-only.

Write a warning in big fat letters about [[ somewhere in the readme #49

Open
andsens opened this issue Mar 24, 2014 · 13 comments
Open

Write a warning in big fat letters about [[ somewhere in the readme #49

andsens opened this issue Mar 24, 2014 · 13 comments
Milestone

Comments

@andsens
Copy link

andsens commented Mar 24, 2014

Try this in bash:

user@machine:~$ set -e
user@machine:~$ [ 1 -eq 1 ]
user@machine:~$ [ 1 -eq 0 ] # shell exits
user@machine:~$ set -e
user@machine:~$ [[ 1 == 1 ]]
user@machine:~$ [[ 1 == 0 ]] # expected to exit, but doesnt
user@machine:~$ echo $?      # however...
1
user@machine:~$

I managed to convert all my unit tests from shUnit2 to bats before I noticed that something was off. I was using the double bracket notation everywhere because it's just nicer to work with. Little did I know that the set -e shell option totally ignored those double brackets.
Best of all, the docs don't mention that fact explicitly. It's because the [[ is part of the conditional constructs, which of course do not trigger that shell option.

@mislav
Copy link
Collaborator

mislav commented Mar 24, 2014

Woah, nasty. Didn't know that.

How should we warn the people? We don't have tutorials on writing actual tests (this is supposed to be left as an exercise to the reader)

@andsens
Copy link
Author

andsens commented Mar 24, 2014

Hm, maybe just in the first example:

@test "addition using bc" {
  result="$(echo 2+2 | bc)"
  [ "$result" -eq 4 ]
  # Do not use double brackets ( [[ ... ]] ).
  # They are treated as conditionals by bash and will not cause any test to fail, ever.
}

Or you could write it below them as a caveat emptor in cursive.

EDIT: emptor... not empty

@sstephenson
Copy link
Owner

set -e behavior with bare [[ ... ]] expressions varies depending on the version of Bash, but I agree we should discourage its use. At the very least you should append || false to the end of a bare [[ ... ]] expression.

I've started a branch that prints a warning when you use a bare [[ ... ]] expression: https://github.com/sstephenson/bats/compare/double-brackets

@duggan
Copy link
Contributor

duggan commented Jul 10, 2014

Some additional information:

Bash 4.1-alpha "fixed" this in 2009 changelog:

j. The [[ and (( commands are now subject to the setting of `set -e' and the
ERR trap.

OSX seems to ship with 3.2 (released in 2007), but anything like Ubuntu will have shipped with a more recent version of Bash (4.1) since at least 10.04.

I'm guessing a lot of tests will be written on OSX and executed in CI environments with Ubuntu. Personally, I'm all for keeping my shell up to date - is it worth introducing a dependency on a more recent version of Bash, or at least a strong recommendation on 4.1?

This can be handled with a keg-only dependency in homebrew... I'm not sure it's a real problem in other package managed environments, since they'll likely already have more recent versions of Bash. The README currently suggests "install from source" which perhaps should be amended.

It's worth noting that this will only cause unexpected behaviour with multiple test statements.

A simple test like:

@test "my foo" {
   [[ 1 = 0 ]]
}

Will fail as you'd expect it to.

@aw-was-here
Copy link

It seems heavy handed to require bash 4.x for something that could be handled with a warning. "Hey! We detected that you are using [[ with bash < 4! You might not want to do that..."

@duggan
Copy link
Contributor

duggan commented Jul 10, 2014

"Require" is probably too strong a word - I mean try to encourage having 4.1+ where possible (ie, in package managers). Incorporating the warning where it's not available makes sense.

@aw-was-here
Copy link

"dependency" = "requirement" in my head. Thank you for the clarification. :D

Totally anecdotal, but as I rewrite the shell scripts that Hadoop ships (and looking at our options for shell unit testing), there was some push back on even requiring bash v3...

@duggan
Copy link
Contributor

duggan commented Jul 10, 2014

Well, I'm generally for writing portable shell wherever possible, but since Bash is already a requirement here, I think that ship has sailed :)

@abesto
Copy link

abesto commented Nov 26, 2015

Data point: I was just bitten by this. +1 for a light-weight solution like adding a comment to the first example.

@sgeb
Copy link

sgeb commented Feb 1, 2016

A comment in the first example of README.md would be helpful. IMHO that's good enough, no need to have bats detect the use of double-brackets and issue a warning.

I've also been bitten by this, used double-brackets to test the output against a regex, like so:

run some cmd
[ "$status" -eq 0 ]
[[ "$output" =~ ^some\ +(random )?\ *regex$ ]]

The test command (i.e. [) doesn't support matching against regular expressions, so I resorted to using grep with a Here String instead:

run some cmd
[ "$status" -eq 0 ]
grep -E '^some +(random )? *regex$' <<< "$output"

Hope this helps others who stumble upon this issue.

Summary:

  • use [ instead of [[ where possible;
  • use grep with a Here String grep -E '...' <<< "$output" instead of double-bracket regex matching [[ "$output" =~ ... ]];
  • append || false if you still prefer to use [[, like so: [[ "$output" =~ blah ]] || false – though I find that quite error prone.

kimh pushed a commit to CircleCI-Archived/image-builder that referenced this issue May 9, 2016
The use of double bracket will bring suprising outcome.
sstephenson/bats#49
@sometheycallme
Copy link

Hope this helps others who stumble upon this issue.

👍

@sometheycallme
Copy link

sometheycallme commented Jul 14, 2016

Ran into some challenges running CircleCI test attempting to check output of commands against a running container.

Seems like Circle gets confused with sdout and stderr

To work with this, I had to craft the following (my clumsy test) test in BATS, and then seek specific output. Needed to sleep 5 to avoid race condition.

HTH

@test "ansible-controller: webserver responds to curl and playbook executes remotely (outside container)" {
 # check to see if ansible webserver accepts json data (commands) and runs fixtures playbook remotely
 run docker run -d --name=webtest -p 8080:8080 --env-file helpful_files/env_vars -v /home/ubuntu/ansible-security/fixtures/etc/ansible:/opt/staging/cleanerbot_master/ansible-security ansible-controller
   ip=$(docker inspect --format '{{ .NetworkSettings.IPAddress }}' webtest)
   port=8080
   sleep 5
  # the curl command sends ansible playbook feedback of play execution to user
  # put the output of the curl command in a variable
   testoutput=$(curl -v -X POST -d '{"branch_name": "master", "git_handle": "cleanerbot", "flags": [{"flag": "-i", "argument": "inventory"}], "playbook": "ansible-security/play_test.yml"}' http://${ip}:${port}/play)
  # echo the variable into a file
   echo $testoutput > testfile
  # run grep for the variable in the file
  run grep PLAY testfile
  #  check for the output
 [[ ${output} =~ PLAY ]]
}

@jonross
Copy link

jonross commented Oct 27, 2016

Just tripped over this myself. Very painful. I found that [[ ]] work in some cases but mis-report the offending code. Minimal example, given:

@test "t1" {
    run sh -c false
    [ $status = 1 ]
}

@test "t2" {
    run sh -c false
    [[ $status == 1 ]]
}

t1 and t2 both pass. Change the condition to [ $status = 0 ] and you get [ $status = 0 ] failed, as expected. Change t2 to [[ $status == 0 ]] and BATS instead reports run sh -c false failed. Given this was my first exposure to BATS I assumed my script was the problem and lost two hours before seeing this issue. Please add this warning to the README.

@ztombol ztombol mentioned this issue Dec 13, 2016
18 tasks
aojea added a commit to aojea/bats that referenced this issue Sep 22, 2017
Bats has know issues with old versions of bash < 4.1 as explained
in issues sstephenson#49 and sstephenson#140.
These issues create confusion because bats runs the tests without
errors, but the results of the tests are wrong.
This patchs adds the capability to check for the bash version and
exit if it's not supported.
mbland added a commit to bats-core/bats-core that referenced this issue Oct 7, 2017
Closes #18.

Corresponds to sstephenson/bats#49. For a negligible performance cost,
assertions using `[[ ... ]]` will now behave as expected even on Bash
versions prior to 4.1-alpha. Most notably, they will work with Bash
3.2.57(1)-release that ships with macOS.

The nice thing is, the way `bats_print_failed_command` already works,
the `|| false` suffix doesn't appear in the failure output. Also, since
the cost is negligible, there's no need to make the suffix insertion
dependent on a version check.

Tested using both 3.2.57(1)-release and Bash 4.4.12(1)-release.
mbland added a commit to bats-core/bats-core that referenced this issue Oct 14, 2017
Closes #18.

Corresponds to sstephenson/bats#49. For a negligible performance cost,
assertions using `[[ ... ]]` will now behave as expected even on Bash
versions prior to 4.1-alpha. Most notably, they will work with Bash
3.2.57(1)-release that ships with macOS.

The nice thing is, the way `bats_print_failed_command` already works,
the `|| false` suffix doesn't appear in the failure output. Also, since
the cost is negligible, there's no need to make the suffix insertion
dependent on a version check.

Tested using both 3.2.57(1)-release and Bash 4.4.12(1)-release.
27Bslash6 added a commit to greenpeace/planet4-docker that referenced this issue Dec 13, 2018
greenpeace-circleci pushed a commit to greenpeace/planet4-docker that referenced this issue Dec 14, 2018
* release/v0.10.125:
  Update domain name
  Tidying
  Extended regex grep call
  Overcome grep exit code 141 error on premature exit. See: https://stackoverflow.com/questions/19120263/why-exit-code-141-with-grep-q
  Strange that test failed in CI but not locally
  Replace all double bracket tests with single bracket. See sstephenson/bats#49
  Describe IMAGE_TAG in use
  Add --allow-root flag (sigh)
  More desperate attempts to debug random `wp core download` failures
alxersov added a commit to alxersov/ctlg that referenced this issue Sep 30, 2019
## Summary

* `restore` command when searches for snapshots uses exact date match.
* Fixed integration tests for older `bash` versions (sstephenson/bats#49).
* Fixed documentation markup.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants