Add test_timeout fields to go_package

[Eric-Arellano]

Closes #13530.

Unlike Python, we use Go's native timeout support instead of the engine's. Users could have already set the timeout globally via [go-test].args, so we have to special-case that the field should override those args.

[tdyas]

What if the user provides a -timeout option on the command-line? Shouldn't that take precedence?

[Eric-Arellano]

I was thinking the option should be a global default. Note that unlike Go, the timeout would be applied per package - not an overall timeout for the whole run.

Also how often are you changing the timeout on the fly via CLI arg rather than permanent config in field or pants.toml? For python, we do know it's common to disable timeouts globally though.

--

The other approach we can do is to consider banning timeout in the Go pass through args and mirror how Python handles timeouts. Global options for a default and max, a timeout field for the target, and a --timeouts bool option to temporarily disable timeouts.

That would be way more consistent.

Wdyt?

[tdyas]

I was thinking the option should be a global default. Note that unlike Go, the timeout would be applied per package - not an overall timeout for the whole run.

Also how often are you changing the timeout on the fly via CLI arg rather than permanent config in field or pants.toml? For python, we do know it's common to disable timeouts globally though.

--

The other approach we can do is to consider banning timeout in the Go pass through args and mirror how Python handles timeouts. Global options for a default and max, a timeout field for the target, and a --timeouts bool option to temporarily disable timeouts.

That would be way more consistent.

Wdyt?

Maybe just add a separate "default test timeout" option then and apply it if a -timeout option has not already been supplied? (and choose between that 'default test timeout" option and the one on the target)

As a user, I expect the timeout supplied on the command-line to take precedence if I run a command like ./pants test path/to/pkg -- -timeout=2m (If I had specified a Pants option on the command-line, it would supersede environment variable and pants.toml. I expect the same precedence here.)

[Eric-Arellano]

K, so to recap, you think this?

• test_timeout field
• [go-test].timeout_default, which you fall back to if the field is not set. Defaults to not being set, meaning no timeouts.
• [go-test].timeout_maximum
• [go-test].timeouts to toggle the feature entirely. Useful when debugging to disable timeouts

And then I had suggested banning timeout in --go-test-args, but you suggest we have it override everything else. Seems fine to me.

[tdyas]

What is the motivation for timeout_maximum? Otherwise those options (besides timeout_maximum) along with an explicit -timeout taking precedence seems fine.

[Eric-Arellano]

So we decided to not spend more time on this PR to focus on other priorities. Meaning not (yet) implementing [go-test].timeout_default, [go-test].timeout_maximum, and [go-test].timeouts.

It's still an open question what to do for --go-test-args including timeout. Options:

1. test_timeout field overrides those args, which this PR does.
2. the args override test_timeout, which @tdyas thinks we should do.
3. error due to ambiguity

@tdyas do you still think #2 makes sense even without those other options implemented? I think it's a bad idea to document using [go-test].args to set a global default..that should be done by the upcoming [go-test].timeout_default.

[tdyas]

do you still think #2 makes sense even without those other options implemented? I think it's a bad idea to document using [go-test].args to set a global default..that should be done by the upcoming [go-test].timeout_default.

The issue for me is not that the user will set -timeout in [go-test].args in a pants.toml, but rather that they will provide -timeout on the command-line as a pass-through argument. I expect that the pass-through argument should take effect over anything else if given on the command-line.