Running tests in parallel

[Miki Tebeka]

Greetings,

I'd like to add an option to run tests in parallel with gotest.
My motivation comes from running selenium tests where each test is pretty much independent from each other but they take time.

My though (in  http://codereview.appspot.com/5016045) was to do the following:
1. Separate test collection to a different goroutine (genTests), it also assigns the output channel for each test
2. Have a RunTests do the what it does so far using genTests to get the tests and wait for each test channel
3. Have RunTestsParallel which run the tests, and after running all the tests (each in a goroutine as it happens now). Will wait on the results channel at the end.

Note that -parallel can't be used in conjunction with -cpu since -cpu implies setting GOMAXPROCS on every test.

Again, you can see the patch at http://codereview.appspot.com/5016045, sorry for jumping the gun and posting the patch before the discussion.

All the best,
--
Miki

[Rob 'Commander' Pike]

Many tests won't work if run in parallel. (Many will, too.) The presence of a --parallel flag means scripts and so on will want to turn it on, and tests must be able to protect themselves from erroneous runs. That mechanism is missing here.

Another smaller issue is that gotest needs to forward the flag.

-rob

[Miki Tebeka]

Many tests won't work if run in parallel.

That is true, and it's the reason parallel is false by default.
 

(Many will, too.)  The presence of a --parallel flag means scripts and so on will want to turn it on, and tests must be able to protect themselves from erroneous runs. That mechanism is missing here.

Do you mean something as grouping tests together to be run in serial?
A simpler option is to add testing.IsParallel and tests can "bail out" if it's true.
 

Another smaller issue is that gotest needs to forward the flag. 

If you mean pass it to testing flags, the patch implement that in 
    + &flagSpec{name: "parallel", isBool: true, passToTest: true},

[Rob 'Commander' Pike]

On Sep 14, 2011, at 2:41 PM, Miki Tebeka wrote:

>
> Many tests won't work if run in parallel.
> That is true, and it's the reason parallel is false by default.

That's not good enough. Tests designed without it may incorrectly fail if -parallel is on.

>
> (Many will, too.) The presence of a --parallel flag means scripts and so on will want to turn it on, and tests must be able to protect themselves from erroneous runs. That mechanism is missing here.
> Do you mean something as grouping tests together to be run in serial?
> A simpler option is to add testing.IsParallel and tests can "bail out" if it's true.

The tests should run if the flag is set, but need a way to say "not in parallel". Depending on the granularity, it could be as simple to use as a global of some form in the test that turns off parallelism for that package's tests. Having a test just bail will not run it at all.

> Another smaller issue is that gotest needs to forward the flag.
>
> If you mean pass it to testing flags, the patch implement that in
> + &flagSpec{name: "parallel", isBool: true, passToTest: true},
>

Oh yes, there it is. Good.

I'm not a fan of this change. It puts unexpected constraints on the individual tests and we need to think hard about the implications and controls.

-rob

[David Symonds]

I, too, was thinking about this the other day.

A different approach is to have a global variable in the testing package,
var Parallelize bool

and then if a test file knows that it is safe to have test cases run
in parallel it can write
func init() { testing.Parallelize = true }

and then gotest can just do the right thing without needing a flag.
This, then, has the right behaviour for existing tests.

Dave.

[Paul Borman]

I prefer something like this.  I have many tests that will fail if parallel is turned on (because they muck with global state).  It is hard to imagine having a knob that will not be fiddled with by others in a larger organization.

Of course, there is another way to do parallel tests:

func TestMany(t *testing.T) {

    go test1(t)

    go test2(t)

    ....

    wait for tests

}

I have to do something similar when I need a test initializer:

func TestMany(t *testing.T) {

    myInit()

    test1(t)

    test2(t)

    myTearDown()

}

    -Paul

[Gustavo Niemeyer]

> That's not good enough. Tests designed without it may incorrectly fail if -parallel is on.

I've been thinking about putting this in gocheck for a while too, and
how to solve that problem.

I suggest whitelisting individual tests that are knowingly able to run
in parallel, and the easiest way to do this is to name them
differently. A nice way to do this would be to have a different prefix
for tests that can be parallelized (e.g. GoTest rather than Test). We
also don't need command line parameters, IMO. If a test can run in
parallel, the runner decides when to do it. If it can't, it shouldn't
ever be run.

--
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I never filed a patent.

[David Symonds]

On Thu, Sep 15, 2011 at 7:54 AM, Paul Borman <bor...@google.com> wrote:

> I prefer something like this.  I have many tests that will fail if parallel
> is turned on (because they muck with global state).  It is hard to imagine
> having a knob that will not be fiddled with by others in a larger
> organization.

My suggestion covers that. The variable's value would not be persisted
between invocations of gotest, so a test (or collection of test files
in one package) need only be concerned by its own parallelizability.

Dave.

[Paul Borman]

I see my note was ambiguous!

It should have started "I prefer something like what David suggested."

When I added the examples below it made the first sentence ambiguous.  Those examples were simply how to do it with no changes to gotest at all (within a given _test.go file)

[Miki Tebeka]

  func init() { testing.Parallelize = true }

How about passing a parameter for selecting tests for running?
    testing.CanParallelRe(re *regexp.Regexp)
    testing.CanParallel(names []string)

[Rob 'Commander' Pike]

Too complicated and fragile.

-rob

[roger peppe]

On 14 September 2011 23:02, Gustavo Niemeyer <gus...@niemeyer.net> wrote:
>> That's not good enough. Tests designed without it may incorrectly fail if -parallel is on.
>
> I've been thinking about putting this in gocheck for a while too, and
> how to solve that problem.
>
> I suggest whitelisting individual tests that are knowingly able to run
> in parallel, and the easiest way to do this is to name them
> differently. A nice way to do this would be to have a different prefix
> for tests that can be parallelized (e.g. GoTest rather than Test). We
> also don't need command line parameters, IMO. If a test can run in
> parallel, the runner decides when to do it.  If it can't, it shouldn't
> ever be run.

+1 (that was my thought too).

except that i couldn't think of an appropriate name, and i don't
like GoTestXXX.

we're trying to indicate that the test is side-effect-free,
but i don't think something like PureTestXXX works.

PTestXXX ? (P standing for either "pure" or "parallel")

[Russ Cox]

There is some question about what the right default is.
Most of our tests run so fast that parallelizing them is
unnecessary. I think that's probably the right model,
which would suggest parallelizing is the exception,
not the rule.

As an exception, this can be accommodated having a
t.Parallel()
method that a test can call to declare that it is okay
to run in parallel with other tests.

The implementation would be to run each test in sequence,
and then the t.Parallel method would let the main loop
continue on to run other tests while this one ran.

It is unclear to me whether you want t.Parallel to mean
'this can run in parallel with any other test' or
'this can run in parallel with any other test that called t.Parallel'.
Either would be straightforward to implement.

Russ

[roger peppe]

that sounds good to me.

i'm not sure the second option would be easy to implement though,
because if a test marked parallel is running, you'd have
no way of knowing if another test would be marked parallel
until you run it, but by then it's too late.

therefore i vote for the first - a test that calls t.Parallel should
be ok to run concurrently with any other test.

[Russ Cox]

On Thu, Sep 15, 2011 at 11:59, roger peppe <rogp...@gmail.com> wrote:
> i'm not sure the second option would be easy to implement though,
> because if a test marked parallel is running, you'd have
> no way of knowing if another test would be marked parallel
> until you run it, but by then it's too late.

It is easy. You leave that goroutine
hanging during the call to t.Parallel,
run all the other tests (hanging any
others that call t.Parallel too) and then
once all the non-parallel stuff is done
you start activating the delayed parallel ones.

Russ

[roger peppe]

doh, of course. i hadn't thought that the call to t.Parallel might block.

i still think that on balance i'd vote for the first option.

then marking a test as parallel essentially means "don't
hold up the testing process because of this test", not
"run this in parallel with other tests marked parallel".
it makes it useful even if there's only one test marked
parallel.

[Russ Cox]

On Thu, Sep 15, 2011 at 12:14, roger peppe <rogp...@gmail.com> wrote:
> then marking a test as parallel essentially means "don't
> hold up the testing process because of this test", not
> "run this in parallel with other tests marked parallel".
> it makes it useful even if there's only one test marked
> parallel.

only if it doesn't break all the other tests.

[roger peppe]

simple rule:
if it might break other tests, then don't mark
it as parallel.

we guarantee that all tests will be called in sequence,
so if there's some shared state that needs to be set up,
then it can be done before calling t.Parallel.

[roger peppe]

on the other hand (thinking a bit more) there are tests that
really want to be run without other concurrent tests - i'm thinking of
time sensitive tests where another busy process might interfere.

these kind of tests are fairly rare though. perhaps a method could be defined
to signify this kind of test. t.Sequential, or t.Exclusive maybe?

On 15 September 2011 17:17, Russ Cox <r...@golang.org> wrote:

[Russ Cox]

simpler rule: only t.Parallel tests run in parallel with other tests.
the non-parallel tests should be fast. they're inconsequential.

if two tests are slow, then you can t.Parallel them if it bothers you.
if one test is slow, well, one test is slow.

russ

[roger peppe]

On 15 September 2011 18:02, Russ Cox <r...@golang.org> wrote:
> simpler rule: only t.Parallel tests run in parallel with other tests.
> the non-parallel tests should be fast.  they're inconsequential.

individually the non-parallel tests might be fast, but all the non-parallel
tests added together might amount to something.

i don't mind too much though.

[Gustavo Niemeyer]

On Thu, Sep 15, 2011 at 14:02, Russ Cox <r...@golang.org> wrote:
> simpler rule: only t.Parallel tests run in parallel with other tests.
> the non-parallel tests should be fast.  they're inconsequential.

I'd prefer to have a different prefix like GoTest for the parallel
tests. This enables the runner to more easily decide how to run the
parallel tests before actually running them, and it's also pretty easy
to identify externally when a test has been marked as parallel.

[Russ Cox]

On Thu, Sep 15, 2011 at 13:13, Gustavo Niemeyer <gus...@niemeyer.net> wrote:
> I'd prefer to have a different prefix like GoTest for the parallel
> tests. This enables the runner to more easily decide how to run the
> parallel tests before actually running them, and it's also pretty easy
> to identify externally when a test has been marked as parallel.

I am strongly opposed to this. It breaks the simple rule
about how you find tests, and it sets a bad precedent
for future extensions.

Russ

[Gustavo Niemeyer]

> I am strongly opposed to this.  It breaks the simple rule
> about how you find tests, and it sets a bad precedent
> for future extensions.

I'm not strongly in favor of it, so won't argue too much. :-)

It just feels like a unique and relevant characteristic to the success
of the test that is worth the little cost of the || in the test
collection logic. I can't foresee many other reasons to add a prefix,
and the precedent has already been settled by benchmarks.

That's the end of my argument, though.

[Russ Cox]

On Thu, Sep 15, 2011 at 13:30, Gustavo Niemeyer <gus...@niemeyer.net> wrote:
> It just feels like a unique and relevant characteristic to the success
> of the test that is worth the little cost of the || in the test
> collection logic. I can't foresee many other reasons to add a prefix,
> and the precedent has already been settled by benchmarks.

A benchmark is not a kind of test.

[Gustavo Niemeyer]

> A benchmark is not a kind of test.

<bikeshed comment here>

[Miki Tebeka]

Another question (going the t.Parallel path).

Currently the main test loop does something like:
    for _, test := range(tests) {
        for _, procs := range cpuList {
            runtime.GOMAXPROCS(procs)
            // Run the test
            runtime.GOMAXPROCS(-1)
        }
    }

However if we run tests in parallel, we might have a race condition with GOMAXPROCS.
I see 3 solutions:
1. Have t.Parallel check if len(cpuList) > 1 and exit with error
2. Have t.Parallel check if len(cpuList) > 1 and if so warn and run the test in serial
3. Have "for _, procs := range cpuList {" be the outer loop. This means tests will run in parallel only in the same GOMAXPROCS.

My vote goes to #3

[Russ Cox]

> 3. Have "for _, procs := range cpuList {" be the outer loop. This means tests will run in parallel only in the same GOMAXPROCS.

sure

[Dmitry Vyukov]

On Thu, Sep 15, 2011 at 9:47 AM, roger peppe <rogp...@gmail.com> wrote:

on the other hand (thinking a bit more) there are tests that
really want to be run without other concurrent tests - i'm thinking of
time sensitive tests where another busy process might interfere.

So what have you decided?

There are indeed sensitive tests. Namely all tests that use runtime.MemStats will be broken if parallel tets run in parallel with non-parallel tests.

[Russ Cox]

t.Parallel should mean that it can run in parallel
with other t.Parallel tests, and only those.

[Dmitry Vyukov]

On Fri, Sep 16, 2011 at 9:10 AM, Miki Tebeka <miki....@gmail.com> 

However if we run tests in parallel, we might have a race condition with GOMAXPROCS.
I see 3 solutions:
1. Have t.Parallel check if len(cpuList) > 1 and exit with error
2. Have t.Parallel check if len(cpuList) > 1 and if so warn and run the test in serial
3. Have "for _, procs := range cpuList {" be the outer loop. This means tests will run in parallel only in the same GOMAXPROCS.

My vote goes to #3

There are some tests that are sensitive to concurrency level. For such tests I always want them to run in exclusive mode (not in parallel with other tests), because otherwise there are basically no guarantees on concurrency level (if -cpu=4 and there are 4 parallel tests, they all can be executed as if with -cpu=1).

So the solution seem to be to just not set t.Parallel() for them.

Then, #3 must do. However, I am not even sure that for t.Parallel() tests it makes sense to execute them several times for different setting of GOMAXPROCS (when -cpu=1,2,3,4). They are parallel, so I don't actually care about concurrency level, it is OK to execute parallel tests just once.

[David Symonds]

There's a trade-off between the t.Parallel() option and my suggestion
of a testing.Parallelize var:

In favour of t.Parallel(),
- You get the fidelity of selecting specific test functions for
being run in parallel.
- It's always safe to add a test function that must be run serially.

In favour of testing.Parallelize,
- Your test code doesn't need a per-function notation.
- Adding a test later will pick up the existing parallelism.
- It's much easier to understand the behaviour.

I see the CL out for t.Parallel() already, but I wonder whether we've
settled the decision yet.

Dave.

[Russ Cox]

I like t.Parallel. It's clear which tests are parallel,
it's not a global default, so you can apply it just to
the tests where it is both appropriate and necessary,
and the behavior is great: all the non-parallel run
first - meaning the easy ones - and then the parallel
ones can chew cpu.

Russ