Code review comment for lp:~gz/bzr/trivial_discard_log_memento_607413

Are we sure this is safe across supported versions? You mention fixed
upstream, but that always concerns me about compatibility.

John
=:->
On Oct 17, 2011 5:38 PM, "Martin Packman" <email address hidden>
wrote:

> Martin Packman has proposed merging
> lp:~gz/bzr/trivial_discard_log_memento_607413 into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> Bug #607413 in Bazaar: " Exception atexit with selftest and Python 2.7"
> https://bugs.launchpad.net/bzr/+bug/607413
>
> For more details, see:
>
> https://code.launchpad.net/~gz/bzr/trivial_discard_log_memento_607413/+merge/79572
>
> Delete _log_memento after TestCase is done with it so as not to upset the
> funky weakref cleanup mechanism. The original visible fallout got fixed
> upstream in the logging module, and with bug 613247 landed the general case
> of accidentally persisting resources is handled. This branch just makes sure
> that even with TestCase objects we keep alive on purpose (errors, for
> instance), the token is thrown away so logging doesn't keep tracking it.
>
> While I was there I updated the docstrings for the TestCase log setup and
> teardown functions as they no longer deal in tempfiles.
> --
>
> https://code.launchpad.net/~gz/bzr/trivial_discard_log_memento_607413/+merge/79572
> Your team bzr-core is requested to review the proposed merge of
> lp:~gz/bzr/trivial_discard_log_memento_607413 into lp:bzr.
>
> === modified file 'bzrlib/tests/__init__.py'
> --- bzrlib/tests/__init__.py 2011-10-14 01:48:16 +0000
> +++ bzrlib/tests/__init__.py 2011-10-17 15:36:26 +0000
> @@ -1718,10 +1718,7 @@
> return result
>
> def _startLogFile(self):
> - """Send bzr and test log messages to a temporary file.
> -
> - The file is removed as the test is torn down.
> - """
> + """Setup a in-memory target for bzr and testcase log messages"""
> pseudo_log_file = StringIO()
> def _get_log_contents_for_weird_testtools_api():
> return [pseudo_log_file.getvalue().decode(
> @@ -1734,14 +1731,13 @@
> self.addCleanup(self._finishLogFile)
>
> def _finishLogFile(self):
> - """Finished with the log file.
> -
> - Close the file and delete it.
> - """
> + """Flush and dereference the in-memory log for this testcase"""
> if trace._trace_file:
> # flush the log file, to get all content
> trace._trace_file.flush()
> trace.pop_log_file(self._log_memento)
> + # The logging module now tracks references for cleanup so discard
> ours
> + del self._log_memento
>
> def thisFailsStrictLockCheck(self):
> """It is known that this test would fail with -Dstrict_locks.
>
>
>