[merge] run_bzr_subprocess supplies --no-plugins

[Robert Collins]

On Tue, 2006-10-10 at 18:18 +1000, John Arbash Meinel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> The attached patch makes run_bzr_subprocess default to supplying
> - --no-plugins.
> 
> This fixes Alexander's problem, where a site-wide installed plugin can
> accidentally cause test failures (such as when bzrtools thinks it is out
> of version sync).
> 
> It also should help make benchmark runs more reproducable, because
> plugins won't be loaded.

In general I like this. I think its worth documenting this in HACKING,
in the TESTING section because it seems like a probably 'wtf' moment for
a test writer.


> @@ -931,11 +931,17 @@
>              The values must be strings. The change will only occur in the
>              child, so you don't need to fix the environment after running.
>          :param universal_newlines: Convert CRLF => LF
> +        :param allow_plugins: By default the subprocess is run with
> +            --no-plugins to ensure test reproducibility. Also, it is possible
> +            for system-wide plugins to create stipple on stderr, which can
> +            cause unnecessary test failures.

Rather than stipple, perhaps 'unexpected output'.

>  
> +    def _popen(self, *args, **kwargs):
> +        """Place a call to Popen.
> +        This allows tests to inspect what is going on, without having to
> +        actually spawn Popen.
> +        """
> +        return Popen(*args, **kwargs)
> +

pep8 here. Also it allows tests to 'override this method to intercept
the calls made to Popen for inspection'. - in and of itself it does not
offer inspection facilities.



>  class TestRunBzrSubprocess(TestCaseWithTransport):
>  
> +    def _popen(self, *args, **kwargs):
> +        """Log the command that is run, so that we can ensure it is correct"""
> +        self._popen_args = args
> +        self._popen_kwargs = kwargs
> +        return super(TestRunBzrSubprocess, self)._popen(*args, **kwargs)

This does not 'log' in the sense used elsewhere in bzr, rather it
records the values passed to _popen. Also, you might consider raising an
exception which you can catch after saving the args : you are not
interested in the program getting run after all.

> +
> +    def test_empty_run_bzr_subprocess(self):
> +        self.run_bzr_subprocess()
> +        command = self._popen_args[0]
> +        self.assertEqual(sys.executable, command[0])
> +        self.assertEqual(self.get_bzr_path(), command[1])
> +        self.assertEqual(['--no-plugins'], command[2:])

I think the test name can be improved - its not an empty test, its a
test that by default --no-plugins is passed in. So lets give it a name
to reflext that.

> +    def test_allow_plugins(self):
> +        self.run_bzr_subprocess(allow_plugins=True)
> +        command = self._popen_args[0]
> +        self.assertEqual([], command[2:])
> +
>      def test_run_bzr_subprocess(self):
>          """The run_bzr_helper_external comand behaves nicely."""
>          result = self.run_bzr_subprocess('--version')
> 
> === modified file bzrlib/tests/test_selftest.py
> --- bzrlib/tests/test_selftest.py
> +++ bzrlib/tests/test_selftest.py
> @@ -1051,3 +1051,5 @@
>          self.apply_redirected(out, err, None, bzrlib.tests.selftest, 
>              test_suite_factory=factory)
>          self.assertEqual([True], factory_called)
> +
> +

What are these lines for ?

-Rob
-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20061013/28603184/attachment.pgp