-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
doc: document that developers working in the go repo should run 'go test' #29266
Comments
(Today's mood: cinnamon.) |
That would surely break a lot of people if If anything, we could make |
I'd be on board with that. (It would be equivalent to renaming |
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? |
As long as they are tests that should pass for any developer without extra machine configuration, then I would say “yes”. |
What about |
@cherrymui, |
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:
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. |
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. |
This is presumably for doc/contribute.html. |
Change https://golang.org/cl/222897 mentions this issue: |
Change https://golang.org/cl/328771 mentions this issue: |
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 <[email protected]> Run-TryBot: Bryan C. Mills <[email protected]> TryBot-Result: Go Bot <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]>
Change https://go.dev/cl/632897 mentions this issue: |
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 iftesting.Short()
returns true. So theall
inall.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 tosmoke.{bash,bat,rc}
instead.(Or perhaps the
all
means “run all the other scripts insrc/
”? But as far as I can tell it doesn't run nearly all of those either.)The text was updated successfully, but these errors were encountered: