Code review comment for lp:~gagern/bzr/bug560030-include-bash-completion-plugin

On 18.05.2010 22:48, John A Meinel wrote:
> I do wonder about using "os.access('/bin/bash', X_OK)" rather than just:
> subprocess.Popen(['bash', '-c', 'echo hello'], shell=True)
> And in theory, that means that you could run the tests on all platforms, rather than requiring a specific path to be available.
> That can be updated/added later, though.

lp:~gagern/bzr/bash_completion-ExecutableFeature has the change from
hardcoded paths to generic PATH environment variable.
I'm avoiding Popen(shell=True) to avoid executing yet another shell
process just to do the path resolution.

> most of the tests pass on Windows except for a couple. However, they're really quite slow:

Ouch! Wouldn't have expected this.

One bad solution would be skipping these tests on windows, or depending
on some environment setting, or some such hack. Feels bad, as it reduces
test coverage for performance reasons only.

In the short run, keeping the number of test cases actually executing
bash is probably the best solution. In the long run, it might be
feasible to execute all of these tests from a single bash instance, but
ensuring proper isolation under these circumstances could be tricky.

> ..._completion.tests.test_bashcomp.TestBashCompletionInvoking.test_revspec_tag_all FAIL 3407ms

That one deserves a closer look. Filed bug #582538 for this.

> I'm certainly hesitant to be adding tests that take multiple seconds to run. Though it may just be a win32 bash thing. Can someone else run "bzr selftest -s bp.bash" and let me know?

What alternatives do you have in mind? Not testing the handwritten bash
code seems like a poor alternative.

> The README clearly doesn't apply as-is anymore.

Will adjust that for both the in-tree and the standalone version at a
later point in time. Will submit a new merge proposal for that.

Thanks for the review, looking forward for the merge.