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

cmd/go: unable to disable test timeout #14780

Closed
cpuguy83 opened this issue Mar 11, 2016 · 15 comments
Closed

cmd/go: unable to disable test timeout #14780

cpuguy83 opened this issue Mar 11, 2016 · 15 comments
Labels
FrozenDueToAge
Milestone
Go1.10

Comments

@cpuguy83
Copy link

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
    1.6
  2. What operating system and processor architecture are you using (go env)?
    linux/amd64
  3. What did you do?

go test timeouts are implemented in 2 ways:

There is the main timeout which just calls panic when the timeout is reached.
This timeout can be disabled when you set a value less than 1

A secondary timeout is set as a failsafe which calls SIGQUIT to dump a stacktrace and then exits.
This is set to the main timeout value + 1 minute:

go/src/cmd/go/test.go

Line 409 in 9323de3

if dt, err := time.ParseDuration(testTimeout); err == nil && dt > 0 {

If the main timeout is not > 0, it just stays at the default 10m.

  1. What did you expect to see?
    The test timeout should be able to be be disabled.
  2. What did you see instead?
    The secondary timeout is in place even when no timeout is set and there is no way to disable.

We use a test helper that wraps testing.T to provide its own functionality, like setup/teardown tests.
This means the entire suite uses a single testing.T. With that, means the only way to not have go kill our integration tests is to set an really high value for the timeout (which I think also defeats the purpose of the timeout to begin with).
@ianlancetaylor ianlancetaylor changed the title Unable to disable test timeout testing: unable to disable test timeout Mar 11, 2016
@ianlancetaylor
Copy link
Contributor

What is the difference between disabling the timeout and using a very high value? That is, why doesn't setting a very high value solve your problem?

@cpuguy83
Copy link
Author

It works, but doesn't feel right.
And then I either have to set an insanely high value and can leave it alone or set something more inline with the time it takes to run the suite and bump it as we add new tests.

@ianlancetaylor
Copy link
Contributor

What are you suggesting that we do?

Note that I would vote against anything that involves adding another option to go test. Additional knobs carry a cost, and require a corresponding benefit. That benefit needs to be more than "it feels more right."

@cpuguy83
Copy link
Author

I agree on not adding another option. Currently the code checks if the timeout is zero and then sets the fallback if it's not.
If it is zero (or perhaps rather just explicitly set to 0) it can disable the fallback as well?

@ianlancetaylor ianlancetaylor changed the title testing: unable to disable test timeout cmd/go: unable to disable test timeout Mar 24, 2016
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Mar 24, 2016
@ianlancetaylor
Copy link
Contributor

I suppose that is OK with me. Want to send a patch?

@cpuguy83
Copy link
Author

Sorry yes, I've been putting this off until I have a chance to digest the contribution process.

@ALTree ALTree mentioned this issue Apr 23, 2017
Closed
@ALTree ALTree modified the milestones: Go1.9, Unplanned Apr 24, 2017
@gopherbot
Copy link

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

@rsc
Copy link
Contributor

rsc commented Jun 22, 2017

I don't see any argument for changing the meaning of -timeout=0 when -timeout=1000h already does what you want and is much clearer about the effect.

@rsc rsc closed this as completed Jun 22, 2017
@ianlancetaylor
Copy link
Contributor

@rsc The -test.timeout flag is documented as accepting 0 to override the timeout in the -help output of any test binary and also in https://tip.golang.org/cmd/go/#hdr-Description_of_testing_flags . So I think this is either a bug in the implementation or a bug in the documentation.

@rsc
Copy link
Contributor

rsc commented Jul 6, 2017

@ianlancetaylor, that text about timeout=0 does not appear in Go 1.8 docs. I will revert whatever introduced it in the Go 1.9 cycle. Thanks.

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Jul 6, 2017
@rsc
cmd/go: fix docs for -timeout
e7f8634 
The text before CL 45816 was:

	-timeout t
		If a test runs longer than t, panic.
		The default is 10 minutes (10m).

CL 45816 was supposed to be about clarifying test vs test binary,
and it did add the clarification of referring to "duration d",
but it also introduced incorrect text about timeout 0.

The new text in this CL preserves the good change and
eliminates the incorrect one:

	-timeout d
		If a test binary runs longer than duration d, panic.
		The default is 10 minutes (10m).

For #14780.

Change-Id: I4f79d6e48ed9295bc9f34a36aa90d3b03b40d7f5
Reviewed-on: https://go-review.googlesource.com/47571
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@vcabbage
Copy link
Member

vcabbage commented Jul 6, 2017

@rsc I introduced the incorrect text about timeout=0 while trying to make the docs consistent with:

flag.Duration("test.timeout", 0, "panic test binary after duration `d` (0 means unlimited)")

go/src/testing/testing.go

Line 268 in bb3be40

timeout = flag.Duration("test.timeout", 0, "panic test binary after duration `d` (0 means unlimited)")

Is the text in the flag also incorrect or have I misunderstood something here?

@vcabbage
Copy link
Member

vcabbage commented Jul 6, 2017

I think I've answered my own question by running some tests.

  • When executing a test binary (built with go test -c) -test.timeout=0 does mean no timeout, which is also the default.
  • When running tests with go test -timeout=0 the default timeout of 10m is used.

The asymmetry seems confusing.

@rsc rsc modified the milestones: Go1.10, Go1.9 Jul 6, 2017
@rsc
Copy link
Contributor

rsc commented Jul 6, 2017 •
edited

Yes, running under the go command is different from running a test binary by hand. I left the issue open (the CL did not say Fix) because I want to think more about this in Go 1.10. The thing is, no one would ever say -test.timeout=0 when invoking a binary by hand, because that's the default. It may be that the right fix to the binary doc is to change

-test.timeout d
	panic test binary after duration d (0 means unlimited)

to

-test.timeout d
	panic test binary after duration d (default unlimited)

There's still the fact that the go command default is different from a test binary's default, but that seems at least plausibly defensible.

Anyway, at least the docs are correct for Go 1.9.

@gopherbot
Copy link

Change https://golang.org/cl/81499 mentions this issue: cmd/go: honor -timeout=0 to mean no timeout

@bcmills bcmills mentioned this issue Oct 11, 2018
Closed
@golang golang locked and limited conversation to collaborators Dec 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
6 participants
@rsc @cpuguy83 @ianlancetaylor @ALTree @gopherbot @vcabbage