search

sstephenson / bats

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

saving $IFS in run() not altered for code using it - fix #89 #90

Closed Sylvain303 closed 9 years ago

Sylvain303 commented 9 years ago

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 #89

mislav commented 9 years ago

Good catch! This looks good.

sstephenson commented 9 years ago

Sorry for the silence. I've held off on merging this patch because I'm worried it'll break tests for people who might rely on the old, erroneous IFS behavior. Even the Bats tests were not immune.

Can we revert until we're ready to make another release where this backwards-incompatible change is documented, with an explanation and a guide for how to ensure your assertions are properly quoted?

mislav commented 9 years ago

Sure! Any reason why we couldn't just add documentation on top of this merge in master, before tagging a new release, without having to revert first?

sstephenson commented 9 years ago

I expect to see support issues from people whose Chef test suites clone Bats master :/

mislav commented 9 years ago

Sure, but the documentation added to our README won't prevent their test suites to get broken by Chef re-runs. I'll open a PR to revert this now but I don't think we'll ever be able to protect those that are riding the bleeding edge from breaking changes.

Sylvain303 commented 9 years ago

Do you want I produce more thing? documentation or sed / perl snippet quoting recursively test?

In fact, my first test written with bats is failing due to $IFS alteration. I don't know exactly if its the expected bash behavior to set the $IFS on this line of code.

IFS=$'\n' lines=($output)

it's somewhat a double affectation. I'm quite surprised it wasn't noticed before.

Did you try to run Chef testsuite with my patched bats?

md5 commented 9 years ago

We got bit by the IFS=$'\n' issue using bats 0.4.0 in the test suite for jwilder/nginx-proxy (see https://github.com/jwilder/nginx-proxy/issues/246). It would be nice to see the fix in a future version of Bats. If not, I'm not sure what the right workaround should be. I just ended up setting IFS back to the default before each for loop that was expecting the normal IFS.

md5 commented 9 years ago

I'll also add that those using ppa:duggan/bats on Debian or Ubuntu already have this patch included in their "0.4.0". That being said, it appears that the proposed package for Ubuntu vivid and the one in wily don't have the patch (cf. https://code.launchpad.net/ubuntu/+source/bats). Neither does the Homebrew packaging of 0.4.0, which is what I was using.