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

Jump to bottom

doc: document that developers working in the go repo should run 'go test' #29266

Open
bcmills opened this issue Dec 14, 2018 · 12 comments
Open

doc: document that developers working in the go repo should run 'go test' #29266

bcmills opened this issue Dec 14, 2018 · 12 comments
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Milestone
Backlog

Comments

@bcmills
Copy link
Contributor

bcmills commented Dec 14, 2018 •
edited

For a lot of folks (like me!), the name all.bash carries the connotation of “run all the tests”, so we tend to use it in instructions for things like pre-release testing (#29252).

Unfortunately, all.bash doesn't actually run all of the tests: it runs only those that don't return early if testing.Short() returns true. So the all in all.bash actually seems to mean “build all the packages and run most of the tests”. Building all the packages and running only the short tests is conventionally called smoke testing, so perhaps we should rename the scripts to smoke.{bash,bat,rc} instead.

(Or perhaps the all means “run all the other scripts in src/”? But as far as I can tell it doesn't run nearly all of those either.)
@gopherbot gopherbot added this to the Proposal milestone Dec 14, 2018
@bcmills
Copy link
Contributor Author

bcmills commented Dec 14, 2018

(Today's mood: cinnamon.)

@bcmills bcmills added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Dec 14, 2018
@bradfitz
Copy link
Contributor

That would surely break a lot of people if all.bash went away.

If anything, we could make all.bash actually be all and add short.bash or smoke.bash.

@bcmills
Copy link
Contributor Author

bcmills commented Dec 14, 2018 •
edited

we could make all.bash actually be all and add short.bash or smoke.bash.

I'd be on board with that. (It would be equivalent to renaming all.bash and replacing it with a similar-but-different script.) Retitled proposal accordingly.

@bcmills bcmills changed the title proposal: rename 'all.{bash,bat,rc}' proposal: rename 'all.{bash,bat,rc}' or make it include long tests Dec 14, 2018
@josharian
Copy link
Contributor

Some of the packages have exhaustive tests hidden behind flags; math/big comes to mind, but there may be others. Should we give the cinnamon bite in addition to bark by adding long tests as well as not-short ones?

@bcmills
Copy link
Contributor Author

bcmills commented Dec 15, 2018

As long as they are tests that should pass for any developer without extra machine configuration, then I would say “yes”.

@cherrymui
Copy link
Member

What about all.bash -long?

@bcmills
Copy link
Contributor Author

bcmills commented Dec 15, 2018

@cherrymui, all.bash -long doesn't address the mislabeling problem. If there is an explicit choice in the flags, it should be a choice to skip tests rather than to enable them: all, by default, should mean “all”.

@rsc
Copy link
Contributor

rsc commented Dec 19, 2018

These names are baked into people's fingers and shell scripts. I am strongly opposed to renaming them or changing their meanings. Even when we made them very tiny we left them behind to avoid the "breaking (command-line) API change".

And all.bash time is something we try very hard to keep small. It makes a big difference whether it's 3 minutes or 10 minutes. The exhaustive regexp tests for example need not run in all.bash.

I'm sorry that all.bash sounded like it was "absolutely all tests in the world". all means make+run.

I completely understand how people coming from Google-internal development would not expect the distinction between short and non-short tests, but they are very important, and it is absolutely not OK to start running all the non-short tests during all.bash.

We do need to make a few things clearer in docs:

  • all.bash does not mean absolutely everything in the world.
  • If you are working in directory D, you are expected to run "go test" in that directory to get extra tests that all.bash does not run.

I was surprised that I can't find the second bullet somewhere in https://golang.org/doc/contribute.html. It should be there at least.

Let's use this bug to document better how to run tests.

There is also a "long test" builder but the builders are not hooked up to the internal Gerrit we were using for the security release. Maybe they should be.

@rsc rsc changed the title proposal: rename 'all.{bash,bat,rc}' or make it include long tests proposal: build: rename 'all.{bash,bat,rc}' or make it include long tests Dec 19, 2018
@josharian
Copy link
Contributor

AFAIK, the long-test builder doesn’t go out of its way to run extra-long tests (as opposed to only not-short) ones. I don’t know how much time (and value) they would add.

@rsc rsc changed the title proposal: build: rename 'all.{bash,bat,rc}' or make it include long tests proposal: doc: document that developers should run 'go test' in the main tree Jan 9, 2019
@rsc rsc changed the title proposal: doc: document that developers should run 'go test' in the main tree proposal: doc: document that developers working in the go repo should run 'go test' Jan 9, 2019
@rsc rsc changed the title proposal: doc: document that developers working in the go repo should run 'go test' doc: document that developers working in the go repo should run 'go test' Jan 9, 2019
@rsc
Copy link
Contributor

rsc commented Jan 9, 2019

This is presumably for doc/contribute.html.

@rsc rsc modified the milestones: Proposal, Go1.13 Jan 9, 2019
@ianlancetaylor ianlancetaylor added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jan 9, 2019
@bcmills bcmills mentioned this issue Jan 18, 2019
Closed
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@gopherbot
Copy link

Change https://golang.org/cl/222897 mentions this issue: doc: document neither all.bash nor run.bash runs all tests

@gopherbot
Copy link

Change https://golang.org/cl/328771 mentions this issue: cmd/internal/moddeps: use a temporary directory for GOMODCACHE if needed

gopherbot pushed a commit that referenced this issue Jun 21, 2021
cmd/internal/moddeps: use a temporary directory for GOMODCACHE if needed
761edf7 
CL 328770 should be sufficient to fix the specific failure in the
report, but when attempting to reproduce it I noticed a related
failure mode, triggered by the environment variables set in
src/run.bash.

The failure mode is currently masked on the Go project builders due to
the lack of any 'longtest' builder running as a non-root user
(#10719).

It is also masked from Go contributors running 'run.bash' locally
because 'run.bash' does not actually run all of the tests unless
GO_TEST_SHORT=0 is set in the environment (#29266, #46054).

Fixes #46695

Change-Id: I272c09dae462734590dce59b3d3c5b6d3f733c92
Reviewed-on: https://go-review.googlesource.com/c/go/+/328771
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
8 participants
@bradfitz @josharian @rsc @andybons @ianlancetaylor @bcmills @gopherbot @cherrymui