Merge lp:~jameinel/bzr/2.4-disable-selftest-fdatasync-837293 into lp:bzr/2.4

[John A Meinel]

Bug #837293, disable fsync and fdatasync while running 'bzr selftest'.

This changes 'bzr selftest' to hack out fsync and fdatasync for the runtime of the test suite.

I'd like to land it in 2.4 because it is the version which slowed down, and it would be nice to have any patches we land there not take 2x the time. I could live with just trunk, though, since we land a lot more patches there. (Though secretly I'd like to land this before I have to merge up my 2.3 patch :).

I chose to do os.* rather than osutils.* because that allows me to not change the test suite. (we still call os.fsync it just is a no-op)

I did run this through PQM and it dropped the runtime about in half. (So 1.5hrs down from 2.5-3.5hrs.)

[Vincent Ladeuil (vila)]

Do we still have tests that would revert the effect of your patch and explicitly test fdatasync ?

If not, a follow-up addressing that would be nice (features.with_fdatasync or something), I see no point in *always* testing fdatasync, but I'm a bit concerned about *never* testing it.

review: Approve

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/02/2011 11:16 AM, Vincent Ladeuil wrote:
> Review: Approve
> Do we still have tests that would revert the effect of your patch and explicitly test fdatasync ?
>
> If not, a follow-up addressing that would be nice (features.with_fdatasync or something), I see no point in *always* testing fdatasync, but I'm a bit concerned about *never* testing it.

You can run "bzr selftest --sync". We still have the test that is
asserting os.fdatasync is getting called if it exists.

Is there something else that you need?

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5gpEMACgkQJdeBCYSNAAMdKQCgpGSstlSjZV3EyIhNJjWncwPM
PecAoJeThVdSA6jiJDVsT1IKXdz6y6AM
=BGdd
-----END PGP SIGNATURE-----

[John A Meinel (jameinel)]

sent to pqm by email

[Vincent Ladeuil (vila)]

>>>>> John A Meinel writes:

    > On 09/02/2011 11:16 AM, Vincent Ladeuil wrote:
    >> Review: Approve
    >> Do we still have tests that would revert the effect of your patch and explicitly test fdatasync ?
    >>

    >> If not, a follow-up addressing that would be nice
    >> (features.with_fdatasync or something), I see no point in
    >> *always* testing fdatasync, but I'm a bit concerned about *never*
    >> testing it.

    > You can run "bzr selftest --sync". We still have the test that is
    > asserting os.fdatasync is getting called if it exists.

I realise that.

    > Is there something else that you need?

More coverage. Doubling the runs on babune for example, won't be a good
usage of resources, but up to 10/20 tests with fdatasync enabled and
always running will be a better fit.

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2011-07-06 13:30:21 +0000
+++ bzrlib/builtins.py	2011-09-02 09:08:24 +0000
@@ -3733,6 +3733,9 @@
                                 param_name='starting_with', short_name='s',
                                 help=
                                 'Load only the tests starting with TESTID.'),
+                     Option('sync',
+                            help="By default we disable fsync and fdatasync"
+                                 " while running the test suite.")
                      ]
     encoding_type = 'replace'
 
@@ -3746,7 +3749,8 @@
             first=False, list_only=False,
             randomize=None, exclude=None, strict=False,
             load_list=None, debugflag=None, starting_with=None, subunit=False,
-            parallel=None, lsprof_tests=False):
+            parallel=None, lsprof_tests=False,
+            sync=False):
         from bzrlib import tests
 
         if testspecs_list is not None:
@@ -3781,6 +3785,8 @@
             exclude_pattern = None
         else:
             exclude_pattern = '(' + '|'.join(exclude) + ')'
+        if not sync:
+            self._disable_fsync()
         selftest_kwargs = {"verbose": verbose,
                           "pattern": pattern,
                           "stop_on_failure": one,
@@ -3808,6 +3814,15 @@
             cleanup()
         return int(not result)
 
+    def _disable_fsync(self):
+        """Change the 'os' functionality to not synchronize."""
+        self._orig_fsync = getattr(os, 'fsync', None)
+        if self._orig_fsync is not None:
+            os.fsync = lambda filedes: None
+        self._orig_fdatasync = getattr(os, 'fdatasync', None)
+        if self._orig_fdatasync is not None:
+            os.fdatasync = lambda filedes: None
+
 
 class cmd_version(Command):
     __doc__ = """Show version of bzr."""

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-08-30 14:35:11 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-09-02 09:08:24 +0000
@@ -38,6 +38,10 @@
 * ``dirstate.fdatasync`` and ``repository.fdatasync`` can now properly be
   disabled. (Vincent Ladeuil, #824513)
 
+* Disable ``os.fsync`` and ``os.fdatasync`` by default when running
+  ``bzr selftest``. You can use ``--sync`` to re-enable them.
+  (John Arbash Meinel, #837293)
+
 * Fix i18n use when no environment variables are set. (Jelmer Vernooij, #810701)
 
 * Avoid UnicodeDecode error when reporting EINVAL from transports.