-
Notifications
You must be signed in to change notification settings - Fork 443
Cleanup #88
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
Conversation
Closes #51. Eliminates the need for a long-lived directory, and makes the suite run about 0.2s to 0.3s quicker to boot (MacBook Pro, 2.9 GHz Intel Core i5): Fastest of three before: 0m5.677s Slowest of three after: 0m5.476s
Has a negligible impact on performance, but eliminates a separate process with no loss in clarity.
To make @sublimino and @jasonkarns happy. :-) The one exception: the `"$BATS_TEST_SKIPPED" != '1'` expression. If updated to `"$BATS_TEST_SKIPPED" -ne 1`, the "tap passing and skipping tests" and "skipped tests" test cases fail.
After running bats with `bash -x bats`, I realized that the `BATS_READLINK` search was happening with every call to `resolve_link()`. This ensures it's set only once, which shaves O(0.01s) off the time of the test suite. Not a huge savings, but the resulting code is just as clear and works as it's intended.
Not only is `[[ ]]` technically safer, but it shaved O(0.07s) off of the suite's running time.
This updates most, but not all, such statements so that easier to see logical branching. The `teardown ... || status="$?"` remains because converting it to an if statement breaks the semantics of the `status="$?"` expression and several tests fail. I'll address the `||` expressions in `run()` in the next commit.
Saves a few lines, and shaves off a slight amount of the suite's running time while achieving the same effect.
Eliminates an external program, and shaves off a slight amount of the suite's run time.
Sidesteps the problems encountered during #32, and with running `bin/bats` on Windows environments that don't support symlinks.
Wow, that's odd. Appveyor's failing with:
Will have to dig into this later. |
An attempt to correct this failure: bash.exe: warning: could not find /tmp, please create! /c/projects/bats-core/libexec/bats-exec-test: line 356: /tmp/bats.2692.src: No such file or directory from these builds: https://ci.appveyor.com/project/bats-core/bats-core/build/v0.4.0.175 https://ci.appveyor.com/project/bats-core/bats-core/build/v0.4.0.176
This reverts commit e6d7bb3. Now we're getting this error: bash -c 'mkdir /tmp' mkdir: cannot create directory '/tmp': File exists from: https://ci.appveyor.com/project/bats-core/bats-core/build/v0.4.0.177 https://ci.appveyor.com/project/bats-core/bats-core/build/v0.4.0.178
We'll see if this is sufficient to fix the current build breakage: https://ci.appveyor.com/project/bats-core/bats-core/build/v0.4.0.179 https://ci.appveyor.com/project/bats-core/bats-core/build/v0.4.0.180
This reverts commit 59f61fc. That didn't fix the previous breakage: https://ci.appveyor.com/project/bats-core/bats-core/build/v0.4.0.181 https://ci.appveyor.com/project/bats-core/bats-core/build/v0.4.0.182
Hoping this'll help debug the current build failures: https://ci.appveyor.com/project/bats-core/bats-core/build/v0.4.0.183 https://ci.appveyor.com/project/bats-core/bats-core/build/v0.4.0.184
Attempts to eliminate this error: bash.exe: warning: could not find /tmp, please create! /c/projects/bats-core/libexec/bats-exec-test: line 369: /tmp/bats.1856.src: No such file or directory Causing Appveyor builds to fail: https://ci.appveyor.com/project/bats-core/bats-core/build/v0.4.0.185 https://ci.appveyor.com/project/bats-core/bats-core/build/v0.4.0.186
This reverts commit c3cb034. The previous change failed to fix the Appveyor build: https://ci.appveyor.com/project/bats-core/bats-core/build/v0.4.0.187 https://ci.appveyor.com/project/bats-core/bats-core/build/v0.4.0.188
It occurred to me that the Appveyor tests were failing because TMPDIR wasn't set, but TMP was, and the `teardown()` hook was deleting /tmp after the first test: https://ci.appveyor.com/project/bats-core/bats-core/build/v0.4.0.189 D'oh!
OK, all is well now. Per the commit message from 0929539: It occurred to me that the Appveyor tests were failing because TMPDIR wasn't set, but TMP was, and the https://ci.appveyor.com/project/bats-core/bats-core/build/v0.4.0.189 D'oh! |
Not much of a noticeable performance gain, but more idiomatic.
Going to merge this now; happy to address comments and concerns after the fact. |
Closes #90. Closes #83. Closes #56. Closes #55. Closes #53. Resolves outstanding issues from #32. Per all these earlier issues and PRs: - The original bin/bats symlink was causing problems on Windows. - The attempted fix from #32 created problems for install.sh. - Per #90, changing bin/bats to a one-line script in #88 broke some Bats installations that rely on symlink schemes (such as when installed via https://github.com/basherpm/basher). For an idea why I wanted to abolish using any symlinks if I could for Windows's sake, 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 Using a method inspired by go-core.bash from https://github.com/mbland/go-script-bash, I was able to reliably compute the root of the Bats installation without using `readlink` by making use of `PWD` and `cd`. What's more, in the course of doing this, I realized libexec/bats didn't need `readlink` either, so I removed it, eliminating external dependencies and subshells. I also removed some extraneous variables and small bits of logic. On top of making install.sh, bin/bats, and libexec/bats more self-contained, resilient, and compact, the existing test suite (before the new installer and symlink tests) 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.326s user 0m2.793s sys 0m1.508s 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.563s user 0m2.782s sys 0m1.614s
Closes #90. Closes #83. Closes #56. Closes #55. Closes #53. Resolves outstanding issues from #32. Per all these earlier issues and PRs: - The original bin/bats symlink was causing problems on Windows. - The attempted fix from #32 created problems for install.sh. - Per #90, changing bin/bats to a one-line script in #88 broke some Bats installations that rely on symlink schemes (such as when installed via https://github.com/basherpm/basher). For an idea why I wanted to abolish using any symlinks if I could for Windows's sake, 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 Using a method inspired by go-core.bash from https://github.com/mbland/go-script-bash, I was able to reliably compute the root of the Bats installation without using `readlink` by making use of `PWD` and `cd`. What's more, in the course of doing this, I realized libexec/bats didn't need `readlink` either, so I removed it, eliminating external dependencies and subshells. I also removed some extraneous variables and small bits of logic. On top of making install.sh, bin/bats, and libexec/bats more self-contained, resilient, and compact, the existing test suite (before the new installer and symlink tests) 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.326s user 0m2.793s sys 0m1.508s 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.563s user 0m2.782s sys 0m1.614s Also tweaks the Dockerfile to update the symlink to point to bin/bats, not libexec/bats.
Closes #90. Closes #83. Closes #56. Closes #55. Closes #53. Resolves outstanding issues from #32. Per all these earlier issues and PRs: - The original bin/bats symlink was causing problems on Windows. - The attempted fix from #32 created problems for install.sh. - Per #90, changing bin/bats to a one-line script in #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.
While running my mbland/go-script-bash tests with v1.0.0 (which I should've tried _before_ v1.0.0), I noticed some failed due to the last line of output missing. This happened because the `while` loop used to replace `sed` in #88 would break the loop when the last line didn't end with a newline. As it turns out, the fix was pretty easy, thanks to the hints from: - https://stackoverflow.com/a/14547230 - https://unix.stackexchange.com/a/418067 However, there was a slight performance hit for the existing tests when the `[[ -n "$line" ]]` conditional appeared as part of the `while` expression. Instead, this change emits the last line in an `if` statement after the `while` loop. I got my mbland/go-script-bash tests to pass by updating the code to always emit a newline, which was the right thing to do regardless. However, it seemed still worth handling this condition in Bats itself, to maintain behavioral parity with v0.4.0.
https://github.com/bats-core/bats-core/releases/tag/v1.0.0, while honoring the same interface as v0.4.0, introduced a few changes that required some cleanup of our Bats utilities: - bin/bats is no longer a symlink to libexec/bats, and the latter can no longer be invoked directly; hence _GO_BATS_PATH is now set to bin/bats. - TAP output for skipped tests changed in bats-core/bats-core#19. - run_bats_test_suite_in_isolation depended upon _GO_ROOTDIR and _GO_BATS_PATH, which led to bats not being able to find its libexec scripts. The bats path is now computed using `command -v bats`, which makes the library itself more portable. - Some assertion-test-helpers output didn't end with a newline. This prevented the `while` loop used to replace `sed` in bats-core/bats-core#88 from reading the final line of output from the test. While this was fixed in bats-core/bats-core#99, updating the code to always emit newlines was still the right thing to do. There was a bug in Bats v1.0.0 in setting `BATS_CWD` that broke the "assertion-test-helpers: failing assertion must disable shell options" test case. Specifically, stack trace paths that should've looked like: tests/assertion-test-helpers.bash instead looked like: go-script-bash/tests/assertion-test-helpers.bash This was fixed by bats-core/bats-core#98.
Various cleanup commits, best reviewed one-at-a-time. Can split some of these out into separate PRs if desired. All together shaves about 0.5s off the suite's running time on my MacBook Pro with a 2.9 GHz Intel Core i5.