Skip to content

Conversation

mbland
Copy link
Contributor

@mbland mbland commented Jun 1, 2018

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.

mbland added 10 commits June 1, 2018 15:21
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.
@mbland mbland requested review from jasonkarns, sublimino, btamayo and a team June 1, 2018 21:05
@ghost ghost assigned mbland Jun 1, 2018
@ghost ghost added the review label Jun 1, 2018
@mbland
Copy link
Contributor Author

mbland commented Jun 1, 2018

Wow, that's odd. Appveyor's failing with:

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

Will have to dig into this later.

mbland added 8 commits June 1, 2018 17:13
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
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
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!
@mbland
Copy link
Contributor Author

mbland commented Jun 1, 2018

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 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!

Not much of a noticeable performance gain, but more idiomatic.
@mbland
Copy link
Contributor Author

mbland commented Jun 3, 2018

Going to merge this now; happy to address comments and concerns after the fact.

@mbland mbland merged commit ac8a176 into master Jun 3, 2018
@ghost ghost removed the review label Jun 3, 2018
@mbland mbland deleted the cleanup branch June 3, 2018 19:15
@mbland mbland mentioned this pull request Jun 3, 2018
2 tasks
mbland added a commit that referenced this pull request Jun 7, 2018
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
mbland added a commit that referenced this pull request Jun 7, 2018
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.
mbland added a commit that referenced this pull request Jun 7, 2018
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.
@mbland mbland mentioned this pull request Jun 7, 2018
2 tasks
mbland added a commit that referenced this pull request Jun 10, 2018
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.
mbland added a commit to mbland/go-script-bash that referenced this pull request Jun 10, 2018
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant