saving $IFS in run() not altered for code using it - fix #89 #90
Conversation
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
| @@ -37,7 +37,7 @@ fixtures bats | |||
| run bats "$FIXTURE_ROOT/passing.bats" | |||
| [ $status -eq 0 ] | |||
| [ ${lines[0]} = "1..1" ] | |||
There was a problem hiding this comment.
Can you quote ${lines[0]} on this line as well?
There was a problem hiding this comment.
You want me to quote it too? I only quoted string compare with a space inside it.
bats/test$ grep -RHn '[^"] = "[^" ]\+'
bats.bats:33: [ $output = "1..0" ]
bats.bats:39: [ ${lines[0]} = "1..1" ]
suite.bats:9: [ $output = "1..0" ]
suite.bats:15: [ ${lines[0]} = "1..1" ]
suite.bats:32: [ ${lines[0]} = "1..3" ]
suite.bats:41: [ ${lines[0]} = "1..3" ]
suite.bats:48: [ ${lines[0]} = "1..3" ]
but nothing with a space on the right side of the compare
bats/test$ grep -RHn '[^"] = "[^" ]\+ ' # <= there's a space here.
There was a problem hiding this comment.
All strings (unless you're absolutely sure it will be a single number or word) should be quoted, especially in conditional expressions.
There was a problem hiding this comment.
No problem. Just that it' was not "part" of the modification I made. I'm modifying it. no more pingpong about it. 👍
|
Good catch! This looks good. |
test are all successful.
saving $IFS in run() not altered for code using it
|
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? |
|
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? |
|
I expect to see support issues from people whose Chef test suites clone Bats master :/ |
|
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. |
|
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. it's somewhat a double affectation. Did you try to run Chef testsuite with my patched bats? |
|
We got bit by the |
|
I'll also add that those using |
test are all successful.
Closes sstephenson#90. Closes sstephenson#83. Closes sstephenson#56. Closes sstephenson#55. Closes sstephenson#53. Resolves outstanding issues from sstephenson#32. Per all these earlier issues and PRs: - The original bin/bats symlink was causing problems on Windows. - The attempted fix from sstephenson#32 created problems for install.sh. - Per sstephenson#90, changing bin/bats to a one-line script in sstephenson#88 broke some Bats installations that rely on symlink schemes (such as when installed via https://github.com/basherpm/basher). For an idea of why I wanted to keep bin/bats as a script due to how symlinks complicate things on Windows, see the following results I discovered during a search for "git symlink windows": - https://github.com/git-for-windows/git/wiki/Symbolic-Links - https://blogs.windows.com/buildingapps/2016/12/02/symlinks-windows-10/ - libgit2/libgit2#4107 - https://stackoverflow.com/q/5917249 In the course of applying these changes, I realized libexec/bats computed some extraneous variables, so I removed them, eliminating a few external processes and subshells. I also cleaned up other small bits of logic. On top of making install.sh, bin/bats, and libexec/bats more resilient and compact, the existing test suite (before adding the new test/installer.bats file) sped up significantly, running 0.6s-0.7s faster on my MacBook Pro (2.9 GHz Intel Core i5, 8GB RAM): Bash 3.2.57(1)-release before: 58 tests, 0 failures real 0m4.924s user 0m3.045s sys 0m1.798s Bash 3.2.57(1)-release after: 58 tests, 0 failures real 0m4.341s user 0m2.808s sys 0m1.540s Bash 4.4.23(1)-release before: 58 tests, 0 failures real 0m5.228s user 0m3.046s sys 0m1.952s Bash 4.4.23(1)-release after: 58 tests, 0 failures real 0m4.582s user 0m2.791s sys 0m1.643s Also tweaks the Dockerfile to update the symlink to point to bin/bats, not libexec/bats.
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