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

proposal: testing: run tests in parallel by default #21214

Closed
posener opened this issue Jul 29, 2017 · 7 comments
Closed

proposal: testing: run tests in parallel by default #21214

posener opened this issue Jul 29, 2017 · 7 comments
Labels
FrozenDueToAge Proposal v2 An incompatible library change
Milestone

Comments

@posener
Copy link

posener commented Jul 29, 2017

The testing framework of go is powerful, one of it's strong points is it's natural ability to run tests in parallel.
I assert that most tests should be able to run in parallel, and if not - a detailed explanation should be given why this test should run on its own. I can gather several reasons for this proposal:

  • Faster tests: running all possible tests in parallel will result in shorter test time.
  • Explicitly: Having a programmer write an explicit reason for the test not to be able run in parallel: t.Serial("Modifies a database which is used by several tests").
  • Bugs detection: running tests in parallel is one of the ways to detect bugs in program design, which might be hidden in a serial invocation of the tests.
  • Less code/ Programmer friendlier: almost every test deceleration takes two lines of code: the test function deceleration and a following line t.Parallel(). It is a common case to see only the first, and the main reason for that is the the programmer forgot to type the second.

I can propose two ways to accept this change:

  1. Breaking change: save it to Go2, since Go1 is good enough as it is. 😄
  2. Can add an additional flag for the test command that sets the default behavior to parallel, unless the Serial function was called on a test.
@gopherbot gopherbot added this to the Proposal milestone Jul 29, 2017
@ianlancetaylor ianlancetaylor added the v2 An incompatible library change label Jul 29, 2017
@bcmills
Copy link
Contributor

bcmills commented Aug 4, 2017

See also #17751 and the attached boilerplate "parallelize" changes.

@ianlancetaylor
Copy link
Member

It would be very difficult to make this change with the existing testing package, as that would be certain to break many existing tests.

Writing t.Parallel() is not hard.

This might be something to consider in a new testing package, but I don't see any path from the current package to a place where tests are run in parallel by default.

I could imagine permitting people to say t.AllParallel() in their TestMain function, but that ought to be a separate proposal.

Closing this issue.

@posener
Copy link
Author

posener commented Mar 9, 2018

Writing t.Parallel() is not hard.

Right, but as said in the proposal: one might forget it. and it is the same line wring over and over again. Also, when not used, race detection are less likely to happened.

This might be something to consider in a new testing package, but I don't see any path from the current package to a place where tests are run in parallel by default.

What do you mean by "new testing package"?

I could imagine permitting people to say t.AllParallel() in their TestMain function, but that ought to be a separate proposal.

What is TestMain function?

Closing this issue.

Closing this means it won't be considered to what is called "Go2"?

Thanks!

@mvdan
Copy link
Member

mvdan commented Mar 9, 2018

What is TestMain function?

https://golang.org/pkg/testing/#hdr-Main

@ianlancetaylor
Copy link
Member

What do you mean by "new testing package"?

For Go 2 it is possible that we could create a new testing package under a different import name. However, there are no current plans to do so.

Closing this means it won't be considered to what is called "Go2"?

Yes. Sorry.

Go 2 has to be largely backward compatible with Go 1. It can only break compatibility where necessary. This benefit of this change is not high enough.

@posener
Copy link
Author

posener commented Mar 9, 2018

OK, thanks @ianlancetaylor .
One last question, sorry about your time. What about an additional flag to go test, as proposed in no. 2 in the proposal? It is not breaking anything as far as I can see, and I am willing to implement it if it is acceptable.

@ianlancetaylor
Copy link
Member

ianlancetaylor commented Mar 9, 2018

An additional flag seems possible and doesn't break compatibility. Why don't you open a separate proposal for that and we can see what the proposal review committee thinks. Thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Proposal v2 An incompatible library change
Projects
None yet
Development

No branches or pull requests

5 participants