sstephenson / bats

Bash Automated Testing System
MIT License
7.12k stars 517 forks source link

IFS broken after first run #89

Closed Sylvain303 closed 9 years ago

Sylvain303 commented 9 years ago

Here follows a bats that fails. First run passes,not the second.

loop_func() {
  local search="none one two tree"
  local d

  for d in $search ; do
    echo $d
  done
}

@test "loop_func" {
  run loop_func
  [[ "${lines[3]}" == 'tree' ]]
  run loop_func
  [[ "${lines[2]}" == 'two' ]]
}

outputs:

bats loop.bats 
 ✗ loop_func
   (in test file loop.bats, line 14)
     `[[ "${lines[2]}" == 'two' ]]' failed

1 test, 1 failure
Sylvain303 commented 9 years ago

a patch that works here, may be other IFS should be saved too.

bash --version GNU bash, version 4.3.30(1)-release (x86_64-pc-linux-gnu)

git diff
diff --git a/libexec/bats-exec-test b/libexec/bats-exec-test
index 5a07665..8f3bd51 100755
--- a/libexec/bats-exec-test
+++ b/libexec/bats-exec-test
@@ -48,7 +48,7 @@ load() {
 }

 run() {
-  local e E T
+  local e E T oldIFS
   [[ ! "$-" =~ e ]] || e=1
   [[ ! "$-" =~ E ]] || E=1
   [[ ! "$-" =~ T ]] || T=1
@@ -57,10 +57,12 @@ run() {
   set +T
   output="$("$@" 2>&1)"
   status="$?"
+  oldIFS=$IFS
   IFS=$'\n' lines=($output)
   [ -z "$e" ] || set -e
   [ -z "$E" ] || set -E
   [ -z "$T" ] || set -T
+  IFS=$oldIFS
 }

 setup() {
mislav commented 9 years ago

Patch looks good. You should submit a pull request with it! Thanks

Sylvain303 commented 9 years ago

Ok, I'm preparing it.

Curliously this patch brakes one test, in some strange way:

bats/test$ bats bats.bats 
 ✓ no arguments prints usage instructions
 ✓ -v and --version print version number
 ✓ -h and --help print help
 ✓ invalid filename prints an error
 ✓ empty test file runs zero tests
 ✗ one passing test
   (in test file bats.bats, line 40)
     `[ ${lines[1]} = "ok 1 a passing test" ]' failed with status 2
   /tmp/bats.23082.src: line 40: [: too many arguments
[…]
36 tests, 1 failure

and this test seems dumy simple…

cat fixtures/bats/passing.bats 
@test "a passing test" {
  true
}

but:

bats fixtures/bats/passing.bats
 ✓ a passing test

1 test, 0 failures

hum and in the test/ dir too, this run fails too

bats . gives 43 tests, 10 failures with the patch

but still

43 tests, 8 failures, without my patch

bash --version GNU bash, version 4.3.11(1)-release (x86_64-pc-linux-gnu)

I'm not yet sure the expected behavior of the command bats . I suppose it should run all test in the given folder.

Sylvain303 commented 9 years ago

Found!

The test in bats.bats line 40 was false, relaying on the IFS wrongly set, before the patch.

Last test is wrong [ ${lines[1]} = "ok 1 a passing test" ] : ${lines[1]} is multi characters and must be surrounded by double quotes. I correct both bats.bats and suite.bats failing on the same wrong assertion.

@test "one passing test" {
  run bats "$FIXTURE_ROOT/passing.bats"
  [ $status -eq 0 ]
  [ ${lines[0]} = "1..1" ]
  [ ${lines[1]} = "ok 1 a passing test" ] # <-- wrong
}
Sylvain303 commented 9 years ago

Do you need more test to merge the request?

alza-bitz commented 8 years ago

Hi there,

I see a fix was merged and then the issue was closed, but subsequently the fix was reverted (but the issue was not reopened?)

Any chance this could be addressed? It would avoid needing to do workarounds in tests. I see some comments in the corresponding PR (#90) talking about a requirement to document the breaking changes?

Thanks

ztombol commented 8 years ago

@alzadude This issue has been bugging me too. I think the sooner this is addressed the less painful it'll be. It's already on my list of issues I'm planning to propose for the next release.

alza-bitz commented 8 years ago

@ztombol that's great news, I've just subscribed to notifications on issue #150 :)