Skip to content
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

bats/helpers: Improve create_forwarding_script #195

Merged
merged 1 commit into from
Sep 1, 2017
Merged

Conversation

mbland
Copy link
Owner

@mbland mbland commented Sep 1, 2017

Also adds create_forwarding_scripts to create multiple forwarding scripts in one call.

The updates to create_forwrading_script addresses several shortcomings that became apparent while using it to test the new test helper skip_if_missing_background_utilities that I'm adding as part of #181.

First, I needed a way to better restrict the PATH of the forwarded command. Trying to set PATH while invoking create_forwarding_script would lead to no forwarding script being created if PATH excluded the directory containing the forwarded program. The new --path option resolves this.

That specific change also resolved a bug where $@ was passed to stub_program_in_path, rather than just the command name argument, causing the script to contain the arguments after the command name.

It then became apparent that the forwarding script needed to exec the wrapped command. As explained in the implementation comments:

The exec feature of the forwarding script is crucial, otherwise the parent process ID within the executed program will be set to the process ID of the wrapper, not the process that invoked the wrapper.

For example, not calling exec confuses bats-exec-test, which invokes itself via bats_perform_tests. As a result, in the parent process, bats_preprocess_source creates BATS_TEST_SOURCE based on BATS_TMPNAME, which is based on $$; then bats_evaluate_preprocessed_source in the child process tries to execute BATS_TEST_SOURCE based on BATS_PARENT_TMPNAME, which is based on $PPID. Since they're different, Bats raises a 'No such file or directory' error.

The correct thing for Bats to do is prefix its self-execution with BATS_TEST_SOURCE="$BATS_TEST_SOURCE", but it's still good practice to use exec here, and to understand why it's important.

Also adds `create_forwarding_scripts` to create multiple forwarding
scripts in one call.

The updates to `create_forwrading_script` addresses several shortcomings
that became apparent while using it to test the new test helper
`skip_if_missing_background_utilities` that I'm adding as part of #181.

First, I needed a way to better restrict the `PATH` of the forwarded
command. Trying to set `PATH` while invoking `create_forwarding_script`
would lead to no forwarding script being created if `PATH` excluded the
directory containing the forwarded program. The new `--path` option
resolves this.

That specific change also resolved a bug where `$@` was passed to
`stub_program_in_path`, rather than just the command name argument,
causing the script to contain the arguments after the command name.

It then became apparent that the forwarding script needed to `exec` the
wrapped command. As explained in the implementation comments:

  The `exec` feature of the forwarding script is crucial, otherwise the
  parent process ID within the executed program will be set to the
  process ID of the wrapper, not the process that invoked the wrapper.

  For example, not calling `exec` confuses `bats-exec-test`, which
  invokes itself via `bats_perform_tests`. As a result, in the parent
  process, `bats_preprocess_source` creates BATS_TEST_SOURCE based on
  BATS_TMPNAME, which is based on $$; then
  `bats_evaluate_preprocessed_source` in the child process tries to
  execute BATS_TEST_SOURCE based on BATS_PARENT_TMPNAME, which is based
  on $PPID. Since they're different, Bats raises a 'No such file or
  directory' error.

  The correct thing for Bats to do is prefix its self-execution with
  `BATS_TEST_SOURCE="$BATS_TEST_SOURCE"`, but it's still good practice
  to use `exec` here, and to understand why it's important.
@mbland mbland added this to the v1.7.0 milestone Sep 1, 2017
@mbland mbland self-assigned this Sep 1, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 94.648% when pulling e8c629c on forwarding-script into e6d646d on master.

@mbland mbland merged commit 736eb12 into master Sep 1, 2017
@mbland mbland deleted the forwarding-script branch September 1, 2017 02:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants