[MERGE] (correct one!) Create fork and reinvoke parallel testing support.

[Robert Collins]

On Fri, 2009-03-27 at 10:03 -0500, John Arbash Meinel wrote:

> === modified file 'bzrlib/tests/__init__.py'
> - --- bzrlib/tests/__init__.py	2009-03-26 12:15:45 +0000
> +++ bzrlib/tests/__init__.py	2009-03-27 09:31:43 +0000
> 
> ...
> 
>                  run += 1
>              actionTaken = "Listed"
>          else:
> - -            test.run(result)
> +            try:
> +                from testtools import ThreadsafeForwardingResult
> +            except ImportError:
> +                test.run(result)
> +            else:
> +                if type(result) == ThreadsafeForwardingResult:
> +                    test.run(BZRTransformingResult(result))
> +                else:
> +                    test.run(result)
>              run = result.testsRun
> 
> ^- This looks like you are trying to do a try/import/except inside an
> inner function. (namely, inside the result handler of the test suite.)
> 
> This seems a whole lot better factored out to a top level import test.
> Going further, it seems this is a genuine case where "isinstance()"
> would be better than "type() ==" (not to mention that it should be
> "type() is ").
> 
> Otherwise, you can't ever subclass ThreadsafeForwardingResult to
> customize it somehow.

Point, OTOH really, BZRTransformingResult shouldn't be needed, but I
didn't want to block this on getting it removed.

> ...
> 
> +def local_concurrency():
> +    try:
> +        content = file('/proc/cpuinfo', 'rb').read()
> +        concurrency = cpucount(content)
> +    except Exception, e:
> +        concurrency = 1
> +    return concurrency
> +
> 
> ^- This is pretty ugly, and means that you can't run the test suite in
> parallel on any platform other than one that supports /proc/cpuinfo. Why
> not just let the user supply -j 2.

I don't need -j X :). I'm not against having -j X.

> Especially since, just because I have 8 cores, maybe I want to keep 1
> around so that I can actually have my editor stay responsive.

Your editor should stay responsive regardless. If it doesn't, may I
suggest vim on Unix ? :).

More seriously, it would be great if you, a Mac user, etc, could
contribute support.

FWIW, most build tools I know of that support -j, have both autodetect
and some capped-heuristic for this sort of thing. Which is fine - they
are generic frameworks. We can do it that way if you really want, but
I'd rather have you get a windows one working and see how it flies.

> Certainly Windows doesn't have /proc/cpuinfo, does Mac? Aix? BSD? (They
> might, I don't know.)
> 
> Autodetect can be nice, but having it be the only way (and perhaps many
> times get it wrong), seems to indicate that having a user flag really
> would be better.

And I should add we can always add that later.

> You also went to all the effort to provide a "subprocess"
> implementation, so that systems without "fork" could still have a
> parallel test suite. It seems a bit silly to then force concurrency == 1
> everywhere.

Actually, subprocess was added on the way to getting ec2 up and running,
and I left it in for Windows, as I figured you might like it :).

> ...
> 
> +    def _error_looks_like(self, prefix, err):
> +        """Deserialize exception and returns the stringify value."""
> +        import subunit
> +        value = None
> +        typ, exc, _ = err
> +        if isinstance(exc, subunit.RemoteException):
> +            # stringify the exception gives access to the remote traceback
> +            # We search the last line for 'prefix'
> +            lines = str(exc).split('\n')
> +            if len(lines) > 1:
> +                last = lines[-2] # -1 is empty, final \n
> +            else:
> +                last = lines[-1]
> +            if last.startswith(prefix):
> +                value = last[len(prefix):]
> +        return value
> +
> 
> ^- This is also a bit ugly, though I understand why it is being done. (I
> believe the intermediate layers don't understand KnownFailure and
> UnavailableFeature, so they just get reported as generic failure and
> errors, which then need to get translated again.) Are there plans to
> ultimately improve this situation?

Yes, this class shouldn't exist.

-Rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090328/d28f89d6/attachment-0001.pgp