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

IFS broken after first run #89

Closed
Sylvain303 opened this issue Jan 28, 2015 · 8 comments

Comments

Projects
None yet
4 participants
@Sylvain303
Copy link
Contributor

commented Jan 28, 2015

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2015

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

This comment has been minimized.

Copy link
Collaborator

commented Jan 29, 2015

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

@Sylvain303

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2015

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2015

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 added a commit to Sylvain303/bats that referenced this issue Jan 29, 2015

saving $IFS in run() not altered for code using it
IFS was modified by run() becoming '\n' and so relying to its bash default
was failing tests.

Also some wrong tests corrected because was relying on this behavior to pass.

Fix sstephenson#89
@Sylvain303

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2015

Do you need more test to merge the request?

@mislav mislav closed this in #90 Feb 25, 2015

mislav added a commit that referenced this issue Feb 26, 2015

md5 added a commit to appropriate/nginx-proxy that referenced this issue Oct 11, 2015

alzadude added a commit to alzadude/ansible-firefox-addon that referenced this issue Jan 25, 2016

@alzadude

This comment has been minimized.

Copy link

commented May 15, 2016

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

This comment has been minimized.

Copy link

commented May 16, 2016

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

@alzadude

This comment has been minimized.

Copy link

commented May 16, 2016

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

jinuxstyle added a commit to jinuxstyle/bats that referenced this issue Jul 1, 2016

saving $IFS in run() not altered for code using it
IFS was modified by run() becoming '\n' and so relying to its bash default
was failing tests.

Also some wrong tests corrected because was relying on this behavior to pass.

Fix sstephenson#89

@ztombol ztombol referenced this issue Dec 13, 2016

Open

Road to v1.0 #196

0 of 18 tasks complete

@jasonkarns jasonkarns referenced this issue Oct 2, 2017

Closed

1.0 bug fixes #20

5 of 6 tasks complete

eli-schwartz added a commit to archlinux/dbscripts that referenced this issue Mar 16, 2018

test: common.bash:__getCheckSum: Don't rely on IFS
I managed to stumble across a bug in BATS where the run() function
screwed with the global IFS.  The bug has been fixed in git, but isn't
in a release yet.

sstephenson/bats#89

Anyway, this bug breaks __getCheckSum().  Fortunately, we have avoided
tripping it so far because luck has it that we never call
__getCheckSum() after run() in the same test.

So, there's nothing actually broken here, but it makes me nervous.  So
go ahead and modify __getCheckSum to not rely on IFS.

And, while we're at it: declare the result variable and set it as
separate commands.  Doing both in the same command masks the exit code
of the subshell expansion.  We don't explicitly check the exit code,
but BATS runs the test suite with `set -e`, so splitting it does mean
that BATS will now detect errors from sha1sum.  We don't really expect
that to happen, but if BATS will give us error checking on it for
free, why not?

mtrmac added a commit to mtrmac/skopeo that referenced this issue Jun 11, 2019

Work around bats corrupting IFS
CentOS, as of bats-0.4.0-1.20141016git3b33a5a.el7.noarch, misses the fix
for sstephenson/bats#89 , causing (read) not
to split words at white space.

Set IFS to the default value explicitly to work around this.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac added a commit to mtrmac/skopeo that referenced this issue Jun 17, 2019

Work around bats corrupting IFS
CentOS, as of bats-0.4.0-1.20141016git3b33a5a.el7.noarch, misses the fix
for sstephenson/bats#89 , causing (read) not
to split words at white space.

Set IFS to the default value explicitly to work around this.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.