Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testing: mark when parallel test begins running in output #19280

Closed
mvdan opened this issue Feb 24, 2017 · 24 comments

Comments

Projects
None yet
7 participants
@mvdan
Copy link
Member

commented Feb 24, 2017

What version of Go are you using (go version)?

go version devel +d9270ecb3a Fri Feb 24 19:09:41 2017 +0000 linux/amd64

What operating system and processor architecture are you using (go env)?

linux/amd64, doesn't matter really

What did you do?

Write a bunch of parallel subtests. Run them with different values of -parallel N.

package foo

import "testing"

func TestFoo(t *testing.T) {
        for _, s := range []string{
                "sub1", "sub2", "sub3", "sub4",
        } {
                t.Run(s, func(t *testing.T) {
                        t.Parallel()
                        println("output")
                })
        }
}

What did you expect to see?

$ go test -v
=== RUN   TestFoo
=== RUN   TestFoo/sub1
=== RUN   TestFoo/sub2
=== RUN   TestFoo/sub3
=== RUN   TestFoo/sub4
output
output
output
output
[...]
$ go test -v -parallel 1
=== RUN   TestFoo
=== RUN   TestFoo/sub1
output
=== RUN   TestFoo/sub2
output
=== RUN   TestFoo/sub3
output
=== RUN   TestFoo/sub4
output
[...]

What did you see instead?

Second command does not separate subtest outputs:

$ go test -v -parallel 1
=== RUN   TestFoo
=== RUN   TestFoo/sub1
=== RUN   TestFoo/sub2
=== RUN   TestFoo/sub3
=== RUN   TestFoo/sub4
output
output
output
output
[...]

I realise this example is very stupid, but the current behavior is confusing. -parallel does take effect, but go test prints all the === RUN lines at once regardless. It's hard to tell when each subtest actually starts executing.

And, for my particular problem, I can't tell what output is from what subtest with -parallel 1 since the outputs aren't delimited by each of their === RUN lines.

@mvdan mvdan changed the title cmd/go: -parallel on test does not affect the order of === RUN lines cmd/go: -parallel on test does not affect the order of RUN lines Feb 24, 2017

@josharian josharian changed the title cmd/go: -parallel on test does not affect the order of RUN lines testing: -parallel on test does not affect the order of RUN lines Feb 25, 2017

@josharian

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2017

cc @mpvl

@bradfitz bradfitz added this to the Go1.9 milestone Mar 21, 2017

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2017

Bump @mpvl? Happy to work on a patch, but first want to know if the current behaviour is on purpose - i.e. whether a patch would be welcome or not.

@magiconair

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2017

This gets more confusing if your tests have sleeps:

package main

import (
	"log"
	"testing"
	"time"
)

func TestA(t *testing.T) {
	t.Parallel()
	for i := 0; i < 3; i++ {
		log.Println("A")
		time.Sleep(25 * time.Millisecond)
	}
}

func TestB(t *testing.T) {
	t.Parallel()
	for i := 0; i < 3; i++ {
		log.Println("B")
		time.Sleep(25 * time.Millisecond)
	}
}

Output:

$ go test -v -parallel 1
=== RUN   TestA
=== RUN   TestB
2017/04/27 13:17:21 A
2017/04/27 13:17:21 A
2017/04/27 13:17:21 A
2017/04/27 13:17:21 B <--- ???
--- PASS: TestA (0.09s)
2017/04/27 13:17:21 B
2017/04/27 13:17:21 B
--- PASS: TestB (0.08s)
PASS
@mpvl

This comment has been minimized.

Copy link
Member

commented May 1, 2017

In order to see logging interleaved in a more structured way with test output, one would use t.Log, not println, log, or fmt.

If you don't use the logging facilities of the testing package, there is no guarantees and what you see is in the realm of the possible. If you increase the number of subtasks you'll see more interleaving of printing and RUN lines.

If you use t.Log, the logs will be grouped with the respective tests. In this case, tests collect the logs for this test and flush it to the parent on completion. Top-level tests flush to stdout.

That said. The particular order in which things are run is very finicky, but if you find an easy way to make this more like you expect you're welcome to try.

@magiconair

This comment has been minimized.

Copy link
Contributor

commented May 1, 2017

@mpvl you are missing the point. The log output in the example does not come from the test but from the code that is tested. I've added it to demonstrate that tests which use t.Parallel() behave differently from tests that don't when used with -parallel 1 which I find surprising. When run with -parallel 1 the tests should behave the same way as when t.Parallel() wasn't there at all. Otherwise, tracing down test errors that stem from running tests in parallel require to comment out the t.Parallel() instead of just disabling it.

In my case, the example was that the last log line of an integration test was attributed to a different test which confused me because it didn't make sense. After commenting out t.Parallel() I noticed that I was chasing a ghost.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented May 1, 2017

My example above was perhaps misleading. My output came from the tested code too.

@mpvl

This comment has been minimized.

Copy link
Member

commented May 2, 2017

Fair enough, I'll look into the issue. May take a bit, as I'm attending a summit at the moment.

@magiconair

This comment has been minimized.

Copy link
Contributor

commented May 2, 2017

@mpvl Thank you.

@mpvl

This comment has been minimized.

Copy link
Member

commented May 3, 2017

This is actually working as intended and an unfortunate consequence of the way t.Parallel works.

Firstly, note that the reported behavior is identical to the case if t.Parallel is called on a bunch of top-level tests.
In general:

  • parallel tests are not run until the calling parent is run to completion (for top-level tests the parent is a hidden overarching main test).
  • In this specific case it would even be illegal to run the parallel children before the parent has completed, because the parent is sequential.
  • RUN is printed before the user-given test function is called.
  • RUN cannot be printed after resuming from a call to Parallel, as 1) there may be output before such a call, and 2) it is not known if Parallel is known at all.

In other words, no matter what the parallelism is, or whether we are talking about top-level or subtests, it is pretty much guaranteed all RUNs are run before the parent test completes and that anything after t.Parallel in a subtest will be run after the parent completes.

Ideally, there would be no t.Parallel and we would use something like t.Go() instead. That way we could print RUN really when the user-given function is called.

So we can't fix RUN. What we could do, though, is to have another header that is printed after a test that was suspended by a call to Parallel is resumed. Something like:

=== RUN TestFoo
=== RUN TestFoo/sub1
=== RUN TestFoo/sub2
=== RUN TestFoo/sub3
=== RUN TestFoo/sub4
=== RESUME TestFoo/sub1
output
=== RESUME TestFoo/sub3
output
=== RESUME TestFoo/sub2
output
=== RESUME TestFoo/sub4
output
--- PASS: TestFoo (0.00s)
--- PASS: TestFoo/sub1 (0.00s)
--- PASS: TestFoo/sub3 (0.00s)
--- PASS: TestFoo/sub2 (0.00s)
--- PASS: TestFoo/sub4 (0.00s)
PASS
ok golang.org/x/text/foo 0.015s

The output would be the same regardless of whether -parallel=1 is passed or not.

Input welcome.

@magiconair

This comment has been minimized.

Copy link
Contributor

commented May 3, 2017

I think what you are describing is a leaking implementation detail. When -parallel 1 is used the call to t.Parallel() should be ignored as if it isn't there at all. After that the two cases behave the same.

@mpvl

This comment has been minimized.

Copy link
Member

commented May 3, 2017

This cannot be changed though; it goes against the spec:
By definition a test that calls t.Parallel is a parallel test.
By definition parallel tests are not run in parallel with sequential tests (including their parent).

Aside from the restrictions opposed by the spec, your suggestion changes the semantics of the parallel test. The parallel tests may depend on initialization of the parent test. Changing this can (and probably will) lead to very obscure and hard to track bugs.

Look at the very contrived example below. The output will be different (though deterministic in both cases) if you remove t.Parallel() or not.

func TestFoo(t *testing.T) {
        var a [4]int
        for i, s := range []string{
                "sub1", "sub2", "sub3", "sub4",
        } {
                i := i
                a[i] = i
                t.Run(s, func(t *testing.T) {
                        t.Parallel()
                        fmt.Println("output", a[3-i])
                })
        }
}

I'm open to adding an additional RESUME or RUN PARALLEL message, but the suggest change cannot be done.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

@mpvl your RESUME lines idea seems interesting. I agree with you that "disabling" the concurrent execution of the tests altogether when given -parallel 1 would be too big a change.

I think adding the RESUME lines would be a bit confusing and too verbose for most people, though.

How about a separate flag that ignored all t.Parallel() lines? Perhaps something like -noparallel or -sequential? As far as I understand, such a flag forcing all tests to be sequential would not go against the spec, as long as it's documented properly.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

To clarify my last point; This is going back to my initial problem that led to opening this issue. I have a lot of parallel tests, some are failing and I want to clearly see what output comes from what test. Unless I run them one by one with -run, I can't use -parallel 1 to force sequential execution. For that, I need to manually remove all t.Parallel() lines. Hence the separate flag.

@magiconair

This comment has been minimized.

Copy link
Contributor

commented May 11, 2017

you could define t.Parallel() as a variable and replace it with a noop function for testing. That might also be something that could be added to the testing lib with a special flag or parallel value, e.g. -1 disables this behavior.

diff --git a/src/testing/testing.go b/src/testing/testing.go
index bd19a31c27..363e372d97 100644
--- a/src/testing/testing.go
+++ b/src/testing/testing.go
@@ -569,8 +569,14 @@ func (c *common) Skipped() bool {
 }

 // Parallel signals that this test is to be run in parallel with (and only with)
-// other parallel tests.
+// other parallel tests. This has no effect when test.parallel is negative.
 func (t *T) Parallel() {
+       if parallel >= 0 {
+               t.parallel()
+       }
+}
+
+func (t *T) parallel() {
        if t.isParallel {
                panic("testing: t.Parallel called multiple times")
        }

(note: I didn't test this yet)

@mpvl

This comment has been minimized.

Copy link
Member

commented May 30, 2017

I would be okay with adding an additional flag or setting parallelism to -1. I tend to prefer the latter one, as I think it will be easier to discover and document.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented May 30, 2017

@mpvl good point, and thanks for all the info and guidance. I'll come up with a CL soon.

@mvdan mvdan assigned mvdan and unassigned mpvl May 30, 2017

@magiconair

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

Would -parallel 0 make more sense? Does that already have a meaning?

@mvdan mvdan changed the title testing: -parallel on test does not affect the order of RUN lines testing: make -parallel=-1 run all tests sequentially May 30, 2017

@mvdan

This comment has been minimized.

Copy link
Member Author

commented May 30, 2017

@magiconair I don't believe 0 has a meaning, but I would think -1 is more commonly regarded as "disable this entirely". 0 could also be confused with the default value of GOMAXPROCS.

@magiconair

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

@mvdan true.

@gopherbot

This comment has been minimized.

Copy link

commented May 31, 2017

CL https://golang.org/cl/44530 mentions this issue.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2017

It seems too difficult to explain why -parallel 1 should mean something different from -parallel 0 or -parallel -1. It seems like the plausible paths forward are:

  • Change the definition of -parallel 1 to mean completely sequential. It's not obvious to me that this is a breaking change. It does change things but I can't come up with any reasonable use of t.Parallel that would depend on the tests running later. @mpvl, do you have a real-world example of how sequentializing everything with -parallel 1 would cause problems?
  • Introduce the new headers @mpvl suggested (ideally cut to 4 letters somehow) to make the source of output clearer.
@rsc

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2017

It sounds like the RESUME headers are the best solution, and they make the output parseable for -parallel=1. Will mark accepted assuming that's the solution. CONT might be better since we've got various output that lines up to 4 letters.

@rsc rsc changed the title testing: make -parallel=-1 run all tests sequentially testing: mark when parallel test begins running in output Jul 17, 2017

@rsc rsc added the NeedsFix label Jul 17, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Jul 18, 2017

CL https://golang.org/cl/49430 mentions this issue.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2017

I hit a stuck test where I thought this would be helpful, so I implemented it. (Didn't help me, but now the CL is out for Go 1.10 at least.)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.