[Merge] lp:~vila/bzr/430749-no-extensions-warning into lp:bzr

[Vincent Ladeuil]

That seems worth redirecting to the ML.
>>>>> "jam" == John A Meinel <john at arbash-meinel.com> writes:

    jam> Martin Pool wrote:
    >> 2009/9/18 fullermd <fullermd at over-yonder.net>:
    >>>> I chose to modify the tests that were expecting a clean stderr (or
    >>>> some expected stderr) because they were only a few of them.
    >>> This makes me a little nervous, in that it adds a new undocumented
    >>> rule about what you need to do if you care about stderr when writing a
    >>> test (and since pretty much anybody writing a test probably HAS
    >>> extensions, they'll never notice it either).
    >> 
    >> It does seem a bit hacky.
    >> 

    jam> Though arguably this is why I asked Vincent to run the test suite on a
    jam> buildbot with extensions disabled... :)

And I think that was and still is an important goal to achieve:
support the python fallbacks. This is a very important goal.

Now, to answer Matthew an Martin concerns: keep in mind that only
an handful of tests required that flag. I should have explained
why a bit better.

The warning is emitted only *once* when bzr completes. In a test
context that means running an *external* process and processing
the full stderr output. That's quite unusual and may even be seen
as abusive. But let's look at the tests that required fixing:

1) bzrlib/tests/blackbox/test_diff.py	TestExternalDiff.test_external_diff

Some comment taken from the test:

        # We have to use run_bzr_subprocess, because we need to
        # test writing directly to stdout, (there was a bug in
        # subprocess.py that we had to workaround).
        # However, if 'diff' may not be available

and

        os.environ['BZR_PROGRESS_BAR'] = 'none'
...

        self.assertEqual('', err)

So here, I could have *removed* that assertion instead of
disabling the warning.

I saw no point in reducing the scope of the test though, even if
its goal is mostly stdout related, it wants an empty stderr.

2) bzrlib/tests/blackbox/test_locale.py TestLocale.test_log_C

The test requires a subprocess, that's hardly arguable we really
want to test a global behavior including an empty stderr.

3) bzrlib/tests/blackbox/test_serve.py TestBzrServe.setUp()

Related to an ongoing discussions, I'd like to mention that all
tests were concerned here and a *single* setUp() was enough to
address the problem *because* those related tests, sharing some
common goal, were already in the same class[1].

And here the common goal is to ensure that 'bzr serve' leave
stderr empty (or with a specific content) under various launching
conditions.

4) bzrlib/tests/test_selftest.py TestActuallyStartBzrSubprocess.test_start_and_stop_bzr_subprocess_send_signal

The test name says it all :)

We want to make sure stderr stays empty.

Summary:
--------

All the tests that needed to disable that warning explicitly
requires an empty stderr. 

I see nothing hackish there.

I don't think the case is common enough that test writers have to
worry about it, future will tell us, but I'm ready to bet that
tests failing for that reason will be quite rare.

      Vincent

[1]: Writing this mail, I realized Robert added a new test
     (test_bzr_connect_to_bzr_ssh), which is unrelated, here
     since I wrote my patch. So this both ruins and validates my
     argument because *that* added test is not concerned by the
     setUp() but Robert added it *before* there was a
     setUp()... Following my own logic, I think I'll split the
     class into 3 different ones: the test_bzr_connect_to_bzr_ssh
     in its own class (that one is ugly enough to not share
     anything with any other test anyway, it's full of XXX :),
     the other tests split between the --inet and --port
     variants. The later two already have their setUp() almost
     defined as start_server_inet() and start_server_port(). I
     even wonder why they are not parametrized tests already...