Skip to content

Conversation

@mbland
Copy link
Owner

@mbland mbland commented Jan 25, 2017

Closes #132. Adds test cases missing from #131.

As demonstrated by the "plugin subcommand finds correct command in own plugin" test case, not setting _GO_SCRIPTS_DIR correctly for a plugin could've caused serious bugs:

  • showstopper (best case): no other command script of that name exists outside the plugin
  • obscure (medium case): finding the wrong script could cause confusing error messages when the interface between the caller and callee don't match
  • subtle (worst case): finding the wrong version of a script could exhibit confusing behaviors that might only become apparent after-the-fact

In the process of writing these tests, I made some other improvements along the way which are well-documented in the commit messages. At a high level:

  • Command scripts are invoked correctly even when /plugins/ appears in _GO_ROOTDIR, in _GO_SCRIPTS_DIR, in the command script path relative to _GO_SCRIPTS_DIR, or in one or more combined. While such cases would be pathological, it was easy to handle them.
  • ./go commands no longer insists there be no duplicate script names, in light of _GO_SEARCH_PATHS and the role it plays in the plugin model.
  • fail_if line_* now prints the full output on error, just as if the corresponding assert_line_* function failed.
  • Added the new test_break_after_num_iterations function to lib/bats/helpers, and hit yet another obscure Bash bug in the process (Bash <4.4 doesn't set the BASH_SOURCE entry for the main script.

After triggering a `fail_if line_matches` failure, I realized it would
be helpful to emit the output just as when `assert_line_matches` fails.
Needed to debug an infinite loop, decided this was a good general
utility.
Turns out `test_break_after_num_iterations` would break on Bash 3.2.57
(and, as I later verified, 4.2.25 and 4.3) thanks to a Bash bug whereby
the `BASH_SOURCE` entry for the main script would remain unset. The
workaround is to use `$0` whenever a `BASH_SOURCE` entry is empty.

Possible fix from https://tiswww.case.edu/php/chet/bash/CHANGES:

  This document details the changes between this version, bash-4.4-rc1,
  and the previous version, bash-4.4-beta.

  h.  Fixed values added to BASH_SOURCE and BASH_LINENO for functions
      inherited from the environment.
While trying to create a test case for #131 and #132 to demonstrate that
setting the `_GO_SCRIPTS_DIR` for a plugin incorrectly may cause it to
find the wrong command scripts, the test exited with an error at some
point because of the constraint that no duplicate scripts are allowed
by `_@go.find_commands` (via `_@go.merge_scripts_into_list`).

Since plugins may contain command names that match those in other
plugins or the top-level `_GO_SCRIPTS_DIR`, and since `_GO_SEARCH_PATHS`
has the correct semantics with regard to which command should take
precedence for any given script, I realized raising an error was no
longer appropriate. Now the first command found via searching
`_GO_SEARCH_PATHS` takes precedence over duplicates in later paths.
Command scripts are invoked correctly even when `/plugins/` appears in
`_GO_ROOTDIR`, in `_GO_SCRIPTS_DIR`, in the command script path relative
to `_GO_SCRIPTS_DIR`, or in one or more combined.
Closes #132. Adds test cases missing from #131.

As demonstrated by the "plugin subcommand finds correct command in own
plugin" test case, not setting `_GO_SCRIPTS_DIR` correctly for a plugin
could've caused serious bugs:

- showstopper (best case): no other command script of that name exists
  outside the plugin
- obscure (medium case): finding the wrong script could cause confusing
  error messages when the interface between the caller and callee don't
  match
- subtle (worst case): finding the wrong version of a script could
  exhibit confusing behaviors that might only become apparent
  after-the-fact
@mbland mbland added this to the v1.4.0 milestone Jan 25, 2017
@mbland mbland self-assigned this Jan 25, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 95.063% when pulling 37b3af4 on plugin-scope-tests into 790d07b on master.

@mbland mbland merged commit 60f2ec6 into master Jan 26, 2017
@mbland mbland deleted the plugin-scope-tests branch January 26, 2017 01:08
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