[PATCH] ignore log to stdout in benchmarks

[John Arbash Meinel]

Carl Friedrich Bolz wrote:
> Robert Collins wrote:
>> Is there any reason this should not be done to bzrlib.tests.TestCase ?
> 
> Basically we thought the way it is done now is there for a reason :-)

There are a few tests that read the log directly. Otherwise, the log is
only used in the case that the test fails.

So cleaning it up in the general case is probably better.

> 
>> That is
>>  - change the default test case finishLogFile to delete the in memory
>> copy.
>>  - Add a method 'getLogFileContents()' which will read the log file from
>> disk.
>>  - Add a method 'setKeepLogFile()' which if called tells finishLogFile
>> to unlink the file from disk rather than just closing it.
> 
> setKeepLogFile would make the file being unlinked? You mean the other
> way round, right?
> 
>> I think this would be a bit cleaner and generally more useful - it would
>> reduce memory footprint during regular bzr selftest too.
> 
> I tried to implement something like that, see attached patch.
> 
> Cheers,
> 
> Carl Friedrich

...

> === modified file 'bzrlib/tests/__init__.py'
> --- bzrlib/tests/__init__.py	2006-08-15 16:26:01 +0000
> +++ bzrlib/tests/__init__.py	2006-08-15 23:18:04 +0000
> @@ -232,6 +232,7 @@
>  
>      def addFailure(self, test, err):
>          unittest.TestResult.addFailure(self, test, err)
> +        test.setKeepLogfile()
>          self.extractBenchmarkTime(test)
>          if self.showAll:
>              self.stream.writeln(" FAIL %s" % self._testTimeString())

^- what about addError?

> @@ -451,6 +452,7 @@
>  
>      _log_file_name = None
>      _log_contents = ''
> +    _keep_log_file = True
>      # record lsprof data when performing benchmark calls.
>      _gather_lsprof_in_benchmarks = False

^- I *think* you wanted the default to be _keep_log_file = False

>  
> @@ -567,16 +569,20 @@
>      def _finishLogFile(self):
>          """Finished with the log file.
>  
> -        Read contents into memory, close, and delete.
> +        Close the file and delete it, except if setKeepLogfile was called.

^- Close the file and delete it, unless setKeepLogfile was called.

>          """
>          if self._log_file is None:
>              return
>          bzrlib.trace.disable_test_log(self._log_nonce)
> -        self._log_file.seek(0)
> -        self._log_contents = self._log_file.read()
>          self._log_file.close()
> -        os.remove(self._log_file_name)
> -        self._log_file = self._log_file_name = None
> +        self._log_file = None
> +        if not self._keep_log_file:
> +            os.remove(self._log_file_name)
> +            self._log_file_name = None
> +
> +    def setKeepLogfile(self):
> +        """Make the logfile not be deleted when _finishLogFile is called."""
> +        self._keep_log_file = True
>  
>      def addCleanup(self, callable):
>          """Arrange to run a callable when this case is torn down.
> @@ -665,11 +671,10 @@
>  
>      def _get_log(self):
>          """Return as a string the log for this test"""
> -        if self._log_file_name:
> +        if self._log_file_name is not None:
>              return open(self._log_file_name).read()
>          else:
> -            return self._log_contents
> -        # TODO: Delete the log after it's been read in
> +            return "DELETED log file to reduce memory footprint"
>  
>      def capture(self, cmd, retcode=0):
>          """Shortcut that splits cmd into words, runs, and returns stdout"""
> 

I don't know whether it is better to read the log file, and cache the
contents and always delete the log file, or to do it as you have done.
My concern is that the log file might go into /tmp, in which case it
really needs to be deleted, so that we don't clutter up /tmp.

We already have too much leakage into /tmp. I currently have 95
testbzr*.log files in /tmp.

I'm guessing that if a test fails, or if a test explicitly requests that
the log file is kept, then it is okay to buffer the contents in memory.

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060815/5cea9a51/attachment.pgp