search

mbland / go-script-bash

Framework for writing modular, discoverable, testable Bash scripts
ISC License
95 stars 16 forks source link

bats/background-process: Close file descriptor 3 #227

Closed mbland closed 6 years ago

mbland commented 6 years ago

Closes #226. From the comment within run_in_background:

Bats duplicates standard output as file descriptor 3 so that output from its framework functions isn't captured along with any output from the code under test. If the code under test contains a sleep or other blocking operation, this file descriptor will be held open until the process becomes unblocked, preventing Bats from exiting. Hence, we explicitly close file descriptor 3.

Any other code running under Bats that opens a background process should close this file descriptor as well. See: https://github.com/sstephenson/bats/issues/80

Much thanks to @marascio for discovering and researching the problem, and proposing the actual fix.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 95.025% when pulling dbf7048c8c5c988c6e8e7b2e3f2aad23f3b4a5eb on close-bats-fd-3 into c82223e7df926fc54543ae796a3ce22bc8b8b97f on master.

mbland commented 6 years ago

Very strange...Coveralls is starting to count the kcov files towards coverage. Will look into this in a little bit: https://coveralls.io/builds/14508796

mbland commented 6 years ago

The coverage dropped because https://github.com/SimonKagstrom/kcov/commit/0dd22bd5cbae00545611551e26a24ab52b8def7a added the ability for kcov to automatically discover all scripts residing in the same directory of the script under test. In this case, it unfortunately discovered all the scripts in ./git and tests/kcov, as well as various *.md and *.yml files that happened to have "bash" in the first line regardless of the presence of a #!.

I'll file a separate PR to resolve this issue before merging this one. I may file a PR against https://github.com/SimonKagstrom/kcov as well, to ensure only files starting with #! are discovered.

mbland commented 6 years ago

Resolved the coverage issue for now by requiring kcov v35; see #244 for details.