need a test helper to reliably capture warnings and mutters

[Vincent Ladeuil]

In https://code.launchpad.net/~vila/bzr/723074-http-debug-mask-credentials/+merge/63697, martin commented:

This looks a lot like something that ought to be split out
into a more generic test helper. I think we accumulate too much stuff
redundantly within individual test cases, and also we rely a bit too
much on overrideAttr to test things. Someone could easily break this
behaviour, not plain unqualified mutter() and then not be caught by

grep -nH -e overrideAttr\(trace :

./test_http.py:1748: self.overrideAttr(trace, 'mutter', mutter)
./test_branch.py:673: self.overrideAttr(trace, 'warning', warning)
./test_config.py:666: self.overrideAttr(trace, 'warning', warning)
./test_config.py:1002: self.overrideAttr(trace, 'warning', warning)
./test_config.py:1135: self.overrideAttr(trace, 'warning', warning)
./test_transform.py:3577: self.overrideAttr(trace, 'warning', warning)
./test_transform.py:3619: self.overrideAttr(trace, 'warning', warning)
./test_plugins.py:811: self.overrideAttr(trace, 'warning', captured_warning)
./test_trace.py:317: self.overrideAttr(trace, '_bzr_log_filename')
./test_trace.py:364: self.overrideAttr(trace, "_bzr_log_filename", None)

[Vincent Ladeuil (vila)]

I agree about the proliferation of redundant stuff and a helper sounds like a good way to address that.

Regarding the use of overrideAttr for these cases, I think it's fine to use it as it means being minimally intrusive and focused on checking an expected behaviour (I quickly looked at the grep hits above and that's the case).

Regarding unqualified mutter(), well, without restarting the import argument, in the mp mentioned above, I *had* to use trace.mutter() (instead of mutter()) to fix the test failures.

Now, since trace is based on logging there may be other ways to achieve the same effect without requiring s/mutter/trace.mutter/ and for tests we may accept to rely on the trace implementation respecting that.

[Martin Pool (mbp)]

I don't think overrideAttr is bad for any particular test. I think it
has some bad emergent behaviour because it does make it easy to do
monkey patching: we don't add a cleaner interface than monkey patching
to set up or observe the thing we care about; also it tends to be done
inline in particular tests. In some cases (sorry, I forget which) the
first of those has seemed to make the tests overly tightly coupled to
the implementation.

I think generally it is at least worth thinking about whether the
overrideAttr ought to be extracted out; and if it is extracted whether
it should go somewhere more general than the particular test file.