From 104df61ac51c375175b331177931b37414c61574 Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Wed, 24 Nov 2021 13:02:48 -0700 Subject: [PATCH 1/5] Add `test_timeout` field # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- src/python/pants/backend/go/goals/test.py | 28 +++++++++++++--- .../pants/backend/go/goals/test_test.py | 32 +++++++++++++++++-- .../pants/backend/go/subsystems/gotest.py | 12 ++++--- src/python/pants/backend/go/target_types.py | 14 ++++++++ 4 files changed, 74 insertions(+), 12 deletions(-) diff --git a/src/python/pants/backend/go/goals/test.py b/src/python/pants/backend/go/goals/test.py index cac0022119b..ab6fbdc94af 100644 --- a/src/python/pants/backend/go/goals/test.py +++ b/src/python/pants/backend/go/goals/test.py @@ -7,7 +7,11 @@ from typing import Sequence from pants.backend.go.subsystems.gotest import GoTestSubsystem -from pants.backend.go.target_types import GoPackageSourcesField, SkipGoTestsField +from pants.backend.go.target_types import ( + GoPackageSourcesField, + GoTestTimeoutField, + SkipGoTestsField, +) from pants.backend.go.util_rules.build_pkg import ( BuildGoPackageRequest, FallibleBuildGoPackageRequest, @@ -65,14 +69,17 @@ class GoTestFieldSet(TestFieldSet): required_fields = (GoPackageSourcesField,) sources: GoPackageSourcesField + timeout: GoTestTimeoutField @classmethod def opt_out(cls, tgt: Target) -> bool: return tgt.get(SkipGoTestsField).value -def transform_test_args(args: Sequence[str]) -> tuple[str, ...]: +def transform_test_args(args: Sequence[str], timeout_field_value: int | None) -> tuple[str, ...]: result = [] + if timeout_field_value is not None: + result.append(f"-test.timeout={timeout_field_value}s") i = 0 next_arg_is_option_value = False @@ -80,7 +87,8 @@ def transform_test_args(args: Sequence[str]) -> tuple[str, ...]: arg = args[i] i += 1 - # If this argument is an option value, then append it to the result and continue to next argument. + # If this argument is an option value, then append it to the result and continue to next + # argument. if next_arg_is_option_value: result.append(arg) next_arg_is_option_value = False @@ -106,9 +114,16 @@ def transform_test_args(args: Sequence[str]) -> tuple[str, ...]: option_value = "" if arg_name in TEST_FLAGS: + no_opt_provided = TEST_FLAGS[arg_name] and option_value == "" + if arg_name == "timeout" and timeout_field_value is not None: + if no_opt_provided: + # Skip the option value. + i += 1 + continue + rewritten_arg = f"{arg[0:start_index]}test.{arg_name}{option_value}" result.append(rewritten_arg) - if TEST_FLAGS[arg_name] and option_value == "": + if no_opt_provided: next_arg_is_option_value = True else: result.append(arg) @@ -249,7 +264,10 @@ def compilation_failure(exit_code: int, stderr: str) -> TestResult: result = await Get( FallibleProcessResult, Process( - ["./test_runner", *transform_test_args(go_test_subsystem.args)], + [ + "./test_runner", + *transform_test_args(go_test_subsystem.args, field_set.timeout.value), + ], input_digest=binary.digest, description=f"Run Go tests: {field_set.address}", cache_scope=cache_scope, diff --git a/src/python/pants/backend/go/goals/test_test.py b/src/python/pants/backend/go/goals/test_test.py index df3aca2f554..1f4a395d4e2 100644 --- a/src/python/pants/backend/go/goals/test_test.py +++ b/src/python/pants/backend/go/goals/test_test.py @@ -52,15 +52,41 @@ def rule_runner() -> RuleRunner: def test_transform_test_args() -> None: - assert transform_test_args(["-v", "--", "-v"]) == ("-test.v", "--", "-v") - assert transform_test_args(["-run=TestFoo", "-v"]) == ("-test.run=TestFoo", "-test.v") - assert transform_test_args(["-run", "TestFoo", "-foo", "-v"]) == ( + assert transform_test_args(["-v", "--", "-v"], timeout_field_value=None) == ( + "-test.v", + "--", + "-v", + ) + assert transform_test_args(["-run=TestFoo", "-v"], timeout_field_value=None) == ( + "-test.run=TestFoo", + "-test.v", + ) + assert transform_test_args(["-run", "TestFoo", "-foo", "-v"], timeout_field_value=None) == ( "-test.run", "TestFoo", "-foo", "-test.v", ) + assert transform_test_args(["-timeout=1m", "-v"], timeout_field_value=None) == ( + "-test.timeout=1m", + "-test.v", + ) + assert transform_test_args(["-timeout", "1m", "-v"], timeout_field_value=None) == ( + "-test.timeout", + "1m", + "-test.v", + ) + assert transform_test_args(["-v"], timeout_field_value=100) == ("-test.timeout=100s", "-test.v") + assert transform_test_args(["-timeout=1m", "-v"], timeout_field_value=100) == ( + "-test.timeout=100s", + "-test.v", + ) + assert transform_test_args(["-timeout", "1m", "-v"], timeout_field_value=100) == ( + "-test.timeout=100s", + "-test.v", + ) + def test_internal_test_success(rule_runner: RuleRunner) -> None: rule_runner.write_files( diff --git a/src/python/pants/backend/go/subsystems/gotest.py b/src/python/pants/backend/go/subsystems/gotest.py index bf4d30d5789..ce27926d968 100644 --- a/src/python/pants/backend/go/subsystems/gotest.py +++ b/src/python/pants/backend/go/subsystems/gotest.py @@ -20,10 +20,14 @@ def register_options(cls, register): member_type=shell_str, passthrough=True, help=( - 'Arguments to pass directly to the Go test binary, e.g. `--go-test-args="-run TestFoo -v"` ' - "Known Go test options will be transformed into the form expected by the test binary, " - "e.g. `-v` becomes `-test.v`. Run `go help testflag` from the Go SDK to learn more about " - "the options supported by Go test binaries." + "Arguments to pass directly to the Go test binary, e.g. " + '`--go-test-args="-run TestFoo -v"`.\n\n' + "Known Go test options will be transformed into the form expected by the test " + "binary, e.g. `-v` becomes `-test.v`. Run `go help testflag` from the Go SDK to " + "learn more about the options supported by Go test binaries.\n\n" + "The `-timeout` arg can be used to set a default timeout for each `go_package`, " + "e.g. `-timeout=2m`. The default can be overridden for each `go_package` via the " + "`test_timeout` field." ), ) diff --git a/src/python/pants/backend/go/target_types.py b/src/python/pants/backend/go/target_types.py index cea85908ecc..a631b36b943 100644 --- a/src/python/pants/backend/go/target_types.py +++ b/src/python/pants/backend/go/target_types.py @@ -16,11 +16,13 @@ AsyncFieldMixin, BoolField, Dependencies, + IntField, InvalidFieldException, InvalidTargetException, MultipleSourcesField, StringField, Target, + ValidNumbers, ) # ----------------------------------------------------------------------------------------------- @@ -127,12 +129,24 @@ class SkipGoTestsField(BoolField): help = "If true, don't run this package's tests." +class GoTestTimeoutField(IntField): + alias = "test_timeout" + help = ( + "A timeout (in seconds) when running this package's tests.\n\n" + "You can set a default timeout by setting `-timeout=2m`, for example, in the " + "`[go-test].args` option. Otherwise, if this field is not set, the test will never time " + "out." + ) + valid_numbers = ValidNumbers.positive_and_zero + + class GoPackageTarget(Target): alias = "go_package" core_fields = ( *COMMON_TARGET_FIELDS, GoPackageDependenciesField, GoPackageSourcesField, + GoTestTimeoutField, SkipGoTestsField, ) help = ( From 5e90305e9c846b5375aec46abdc972c24457a2a3 Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Thu, 2 Dec 2021 10:03:50 -0700 Subject: [PATCH 2/5] CLI args override field # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- pants.toml | 2 +- src/python/pants/backend/go/goals/test.py | 12 +++++------- src/python/pants/backend/go/goals/test_test.py | 7 ++++--- src/python/pants/backend/go/target_types.py | 4 +--- 4 files changed, 11 insertions(+), 14 deletions(-) diff --git a/pants.toml b/pants.toml index 2bcf90a0b13..f0046781e3f 100644 --- a/pants.toml +++ b/pants.toml @@ -54,7 +54,7 @@ pants_ignore.add = [ build_ignore.add = [ # Disable Go targets by default so Pants developers do not need Go installed. - "testprojects/src/go/**", +# "testprojects/src/go/**", ] # NB: Users must still set `--remote-cache-{read,write}` to enable the remote cache. diff --git a/src/python/pants/backend/go/goals/test.py b/src/python/pants/backend/go/goals/test.py index ab6fbdc94af..d2512f911e2 100644 --- a/src/python/pants/backend/go/goals/test.py +++ b/src/python/pants/backend/go/goals/test.py @@ -78,11 +78,9 @@ def opt_out(cls, tgt: Target) -> bool: def transform_test_args(args: Sequence[str], timeout_field_value: int | None) -> tuple[str, ...]: result = [] - if timeout_field_value is not None: - result.append(f"-test.timeout={timeout_field_value}s") - i = 0 next_arg_is_option_value = False + timeout_is_set = False while i < len(args): arg = args[i] i += 1 @@ -116,10 +114,7 @@ def transform_test_args(args: Sequence[str], timeout_field_value: int | None) -> if arg_name in TEST_FLAGS: no_opt_provided = TEST_FLAGS[arg_name] and option_value == "" if arg_name == "timeout" and timeout_field_value is not None: - if no_opt_provided: - # Skip the option value. - i += 1 - continue + timeout_is_set = True rewritten_arg = f"{arg[0:start_index]}test.{arg_name}{option_value}" result.append(rewritten_arg) @@ -128,6 +123,9 @@ def transform_test_args(args: Sequence[str], timeout_field_value: int | None) -> else: result.append(arg) + if not timeout_is_set and timeout_field_value is not None: + result.append(f"-test.timeout={timeout_field_value}s") + result.extend(args[i:]) return tuple(result) diff --git a/src/python/pants/backend/go/goals/test_test.py b/src/python/pants/backend/go/goals/test_test.py index 1f4a395d4e2..d6f270d86e7 100644 --- a/src/python/pants/backend/go/goals/test_test.py +++ b/src/python/pants/backend/go/goals/test_test.py @@ -77,13 +77,14 @@ def test_transform_test_args() -> None: "1m", "-test.v", ) - assert transform_test_args(["-v"], timeout_field_value=100) == ("-test.timeout=100s", "-test.v") + assert transform_test_args(["-v"], timeout_field_value=100) == ("-test.v", "-test.timeout=100s") assert transform_test_args(["-timeout=1m", "-v"], timeout_field_value=100) == ( - "-test.timeout=100s", + "-test.timeout=1m", "-test.v", ) assert transform_test_args(["-timeout", "1m", "-v"], timeout_field_value=100) == ( - "-test.timeout=100s", + "-test.timeout", + "1m", "-test.v", ) diff --git a/src/python/pants/backend/go/target_types.py b/src/python/pants/backend/go/target_types.py index a631b36b943..60b472aea62 100644 --- a/src/python/pants/backend/go/target_types.py +++ b/src/python/pants/backend/go/target_types.py @@ -133,9 +133,7 @@ class GoTestTimeoutField(IntField): alias = "test_timeout" help = ( "A timeout (in seconds) when running this package's tests.\n\n" - "You can set a default timeout by setting `-timeout=2m`, for example, in the " - "`[go-test].args` option. Otherwise, if this field is not set, the test will never time " - "out." + "If this field is not set, the test will never time out." ) valid_numbers = ValidNumbers.positive_and_zero From 71531f5a209d6b888f68e6cad1e37b958d01b370 Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Thu, 2 Dec 2021 11:21:25 -0700 Subject: [PATCH 3/5] Fix fieldset not working # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- pants.toml | 2 +- src/python/pants/backend/go/goals/test.py | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/pants.toml b/pants.toml index f0046781e3f..2bcf90a0b13 100644 --- a/pants.toml +++ b/pants.toml @@ -54,7 +54,7 @@ pants_ignore.add = [ build_ignore.add = [ # Disable Go targets by default so Pants developers do not need Go installed. -# "testprojects/src/go/**", + "testprojects/src/go/**", ] # NB: Users must still set `--remote-cache-{read,write}` to enable the remote cache. diff --git a/src/python/pants/backend/go/goals/test.py b/src/python/pants/backend/go/goals/test.py index d2512f911e2..a9ed7ad8be3 100644 --- a/src/python/pants/backend/go/goals/test.py +++ b/src/python/pants/backend/go/goals/test.py @@ -4,6 +4,7 @@ from __future__ import annotations import os +from dataclasses import dataclass from typing import Sequence from pants.backend.go.subsystems.gotest import GoTestSubsystem @@ -65,6 +66,7 @@ } +@dataclass(frozen=True) class GoTestFieldSet(TestFieldSet): required_fields = (GoPackageSourcesField,) From c5ccc65936f0711ea69d7b6c6117e9a60d72587b Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Thu, 2 Dec 2021 11:27:23 -0700 Subject: [PATCH 4/5] Fix bad help string # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- src/python/pants/backend/go/subsystems/gotest.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/python/pants/backend/go/subsystems/gotest.py b/src/python/pants/backend/go/subsystems/gotest.py index ce27926d968..651bde17139 100644 --- a/src/python/pants/backend/go/subsystems/gotest.py +++ b/src/python/pants/backend/go/subsystems/gotest.py @@ -24,10 +24,7 @@ def register_options(cls, register): '`--go-test-args="-run TestFoo -v"`.\n\n' "Known Go test options will be transformed into the form expected by the test " "binary, e.g. `-v` becomes `-test.v`. Run `go help testflag` from the Go SDK to " - "learn more about the options supported by Go test binaries.\n\n" - "The `-timeout` arg can be used to set a default timeout for each `go_package`, " - "e.g. `-timeout=2m`. The default can be overridden for each `go_package` via the " - "`test_timeout` field." + "learn more about the options supported by Go test binaries." ), ) From 7ffb4c9d4f957881bc2ed6892e22b3587462ecc3 Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Thu, 2 Dec 2021 11:29:11 -0700 Subject: [PATCH 5/5] Clean up test args handling, oops # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- src/python/pants/backend/go/goals/test.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/python/pants/backend/go/goals/test.py b/src/python/pants/backend/go/goals/test.py index a9ed7ad8be3..ea2cf77cb30 100644 --- a/src/python/pants/backend/go/goals/test.py +++ b/src/python/pants/backend/go/goals/test.py @@ -114,12 +114,13 @@ def transform_test_args(args: Sequence[str], timeout_field_value: int | None) -> option_value = "" if arg_name in TEST_FLAGS: - no_opt_provided = TEST_FLAGS[arg_name] and option_value == "" - if arg_name == "timeout" and timeout_field_value is not None: + if arg_name == "timeout": timeout_is_set = True rewritten_arg = f"{arg[0:start_index]}test.{arg_name}{option_value}" result.append(rewritten_arg) + + no_opt_provided = TEST_FLAGS[arg_name] and option_value == "" if no_opt_provided: next_arg_is_option_value = True else: