proposal: testing: expose denyParallel

[arp242]

t.Setenv() and t.Chdir() set t.denyParallel to prevent tests from using t.Parallel(), which would cause problems.

It would be useful to be able to mark tests or custom testing helpers as such. For example in some tests I want to os.Unsetenv() a variable, or set a package-level "test hook" variable. These tests can't be run in parallel, but there is nothing preventing this except careful documentation and usage.

The current workaround is t.Setenv("NONEXISTENT", ""), which is ugly and results in a bad error.

---

Concretely:

// erial marks that this test can only be run in Serial, and
    // prevents this test or any of its ancestors to run in parallel.
func (t *T) Serial() {
	t.checkParallel()
}

And maybe also changing t.denyParallel to a string with the function name, so the error can be changed from:

const parallelConflict = `testing: test using t.Setenv, t.Chdir, or cryptotest.SetGlobalRandom can not use t.Parallel`

to:

const parallelConflict = `testing: test using %s can not use t.Parallel`

---

Related:

proposal: testing: ability to tell if test is running in parallel #70017
proposal: testing: run tests in parallel by default #73805

If #73805 is implemented in the future, t.Serial() could be safely changed to mean "don't run this test in parallel" instead "panic if t.Parallel() is called".

[seankhliao]

to bikeshed: Serial sounds nicer than DenyParallel

[mateusz834]

prevents this test or any of its ancestors to run in parallel.

Shouldn't that prevent any test parallel execution, not only ancestors? i.e. if there are other parallel tests.

[seankhliao]

denyParallel triggers a fatal panic, I think any API that's exposed should just do the right thing and arrange for non parallel execution rather than triggering a panic.

[gabyhelp]

Related Issues

• proposal: testing: ability to tell if test is running in parallel #70017
• testing: add TB.Setenv() #41260 (closed)
• proposal: testing: add a `ParallelCount` func that reports the `go test -p` flag value #50719 (closed)
• proposal: add testing.T.RunParallel for simplifying data race detection in tests #21847 (closed)
• testing: API for deferring a cleanup step when subtests use Parallel #31651 (closed)
• proposal: testing: allow settings flags for the duration of a test #75256 (closed)
• proposal: testing: add Unsetenv to call os.Unsetenv and then restore the value using Cleanup #52817 (closed)
• testing: T.Setenv can be called after T.Parallel #55128 (closed)

Related Code Changes

• testing: fail if T.Setenv is called via T.Run in a parallel test
• testing: add TB.Setenv

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[arp242]

denyParallel triggers a fatal panic, I think any API that's exposed should just do the right thing and arrange for non parallel execution rather than triggering a panic.

t.Parallel() also does that if called twice, or if you called t.Chdir() or t.Setenv(). So not much changes there.

For better or worse, the testing package is already panic-y here.

---

I updated the proposal to use t.Serial() instead of t.DenyParallel(). If #73805 is implemented in the future, t.Serial() could be safely changed to mean "don't run this test in parallel" instead "panic if t.Parallel() is called".

[apparentlymart]

I think this is similar to what I was thinking about in #70017 (comment), except that I was proposing to use one of the t.FailNow()-wrapping functions instead of panicking.

I was expecting it would be used something like this:

func doSomeWeirdGlobalOverrideForTesting(t *testing.T) {
    t.Helper()
    t.Serial()
    oldSomething := globalSomething
    t.Cleanup(func() {
        globalSomething = oldSomething
    })
}

...and then if a test calls this function after it already called t.Parallel it would make the test fail with a log message referring to the line that called this function.

If the calls are in the opposite order so that t.Parallel comes after, then that's trickier because it'd need to track who previously callde t.Serial() to know which function to blame in the failure message. I suppose that's where your idea of specifying a function name in the call would come in, but perhaps an alternative would be to use the name of the deepest active t.Helper caller at the time that t.Serial was called, which would be packagename.doSomeWeirdGlobalOverrideForTesting in my example above.

(What it'd do if nobody has called t.Helper is a good question. I'm not sure what would be best. I've only personally wanted a function like t.Serial in functions that already call t.Helper, but that does seem like a pretty arbitrary restriction. 🤷‍♂️ )