Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions build_defs/go.build_defs
Original file line number Diff line number Diff line change
Expand Up @@ -847,10 +847,29 @@ def go_test(name:str, srcs:list, resources:list=None, data:list|dict=None, deps:
import_path = "main",
_generate_pkg_info = False,
)
cmds, tools = _go_binary_cmds(name, static=static, definitions=definitions, gcov=cgo, test=True)
# Match go test: testing/testing.go:692 (Go 1.25.3) relies on testing.testing=true when linking tests.
testing_definition = "testing.testing=true"
if definitions is None:
test_definitions = [testing_definition]
elif isinstance(definitions, dict):
if "testing.testing" in definitions:
test_definitions = definitions
else:
test_definitions = definitions | {"testing.testing": "true"}
else:
if isinstance(definitions, str):
test_definitions = [definitions]
elif isinstance(definitions, list):
test_definitions = list(definitions)
else:
test_definitions = list(definitions)
Comment on lines +862 to +865
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified

if testing_definition not in test_definitions:
test_definitions.append(testing_definition)
Comment on lines +850 to +867
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can all be removed in favour of updating/extending https://github.com/please-build/go-rules/blob/master/build_defs/go.build_defs#L1750 - please check if that testing.testBinary definition is still relevant


cmds, tools = _go_binary_cmds(name, static=static, definitions=test_definitions, gcov=cgo, test=True)


test_cmd = f'$TEST {flags} 2>&1 | tee $TMP_DIR/test.results'
test_cmd = f'$TEST {flags} 2>&1 | tee -- "$TMP_DIR/test.results"'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have you made this change?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dded the -- here to ensure robustness. It explicitly tells tee to stop processing arguments as options, treating $TMP_DIR/test.results strictly as a file name, even if it happens to start with a hyphen.

worker_cmd = f'$(worker {worker})' if worker else ""
if worker_cmd:
test_cmd = f'{worker_cmd} && {test_cmd} '
Expand Down
9 changes: 7 additions & 2 deletions test/build_defs/e2e.build_defs
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,16 @@ subinclude("///e2e//build_defs:e2e")

_please_repo_e2e_test = please_repo_e2e_test

def please_repo_e2e_test(name, expected_output=None, plz_command, repo, labels=[]):
def please_repo_e2e_test(name, expected_output=None, plz_command, repo, labels=[], expect_output_contains=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The upstream already has a expect_output_contains parameter which we shouldn't shadow. You're also introducing a second meaning of the term "output", which is going to be confusing. At a minimum I think the new parameter should be expected_plz_command_output, but I think it'd be nicer to support asserting on stdout and stderr independently, rather than capturing both in one file (at which point expect_stdout_contains and expect_stderr_contains are nice unambiguous names)

command > >(tee stdout.log) 2> >(tee stderr.log >&2) might work here, but you'll need to test that it preserves the exit code correctly. tee is a nice utility for duplicating stdin to a file. See https://stackoverflow.com/a/692407 for more explanation.

Either way, I think you should implement this as part of the upstream rule in https://github.com/please-build/plugin-integration-testing/blob/master/build_defs/e2e.build_defs so that other plugins can benefit from this change.

aside: can you spot the bug in https://github.com/please-build/plugin-integration-testing/blob/master/build_defs/e2e.build_defs#L89

plz_cmd = plz_command.replace("plz ", "plz -o please.PluginRepo:file://$TMP_DIR/$DATA_PLUGIN_REPO -o plugin.go.gotool:$TOOLS_GO -o plugin.go.pleasegotool:$TOOLS_PLEASE_GO ")
# Persist command output so subsequent assertions can inspect it while keeping the original exit code.
wrapped_plz_command = f"( {plz_cmd} >output 2>&1 ); STATUS=$?; cat output; exit $STATUS"

return _please_repo_e2e_test(
name = name,
expected_output = expected_output,
plz_command = plz_command.replace("plz ", "plz -o please.PluginRepo:file://$TMP_DIR/$DATA_PLUGIN_REPO -o plugin.go.gotool:$TOOLS_GO -o plugin.go.pleasegotool:$TOOLS_PLEASE_GO "),
expect_output_contains = expect_output_contains,
plz_command = wrapped_plz_command,
repo = repo,
data = {
"PLUGIN_REPO": ["//test:export"],
Expand Down
10 changes: 10 additions & 0 deletions test/testing_definition/BUILD
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
subinclude("//build_defs:go")
subinclude("//test/build_defs:e2e")

go_test(
name = "testing_definition_test",
Expand All @@ -7,3 +8,12 @@ go_test(
"///third_party/go/github.com_stretchr_testify//assert",
],
)

please_repo_e2e_test(
name = "testing_definition_integration_test",
plz_command = "plz test --show_all_output //sample:testing_flag_test -- -test.v",
repo = "test_repo",
expect_output_contains = {
"output": "testing.Testing() = true",
},
)
8 changes: 8 additions & 0 deletions test/testing_definition/test_repo/.plzconfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
[Parse]
BuildFileName = BUILD_FILE
BuildFileName = BUILD
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
BuildFileName = BUILD

PreloadSubincludes = ///go//build_defs:go

[Plugin "go"]
Target = //plugins:go
Stdlib = //third_party/go:std
4 changes: 4 additions & 0 deletions test/testing_definition/test_repo/plugins/BUILD_FILE
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
plugin_repo(
name = "go",
revision = "master",
)
4 changes: 4 additions & 0 deletions test/testing_definition/test_repo/sample/BUILD_FILE
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
go_test(
name = "testing_flag_test",
srcs = ["testing_flag_test.go"],
)
13 changes: 13 additions & 0 deletions test/testing_definition/test_repo/sample/testing_flag_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package sample

import (
"fmt"
"testing"
)

func TestTestingFlag(t *testing.T) {
fmt.Printf("testing.Testing() = %v\n", testing.Testing())
if !testing.Testing() {
t.Fatalf("expected testing.Testing() to be true")
}
}
3 changes: 3 additions & 0 deletions test/testing_definition/test_repo/third_party/go/BUILD_FILE
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
go_stdlib(
name = "std",
)