Enhance Go testing support and add integration tests #332
Conversation
scyyx5
commented
Nov 11, 2025
scyyx5
commented
- Update go_test to ensure proper handling of testing definitions.
- Modify please_repo_e2e_test to capture command output for assertions.
- Add integration test for testing_flag_test with expected output validation.
- Introduce necessary BUILD_FILE configurations for plugins and dependencies.
- Update go_test to ensure proper handling of testing definitions. - Modify please_repo_e2e_test to capture command output for assertions. - Add integration test for testing_flag_test with expected output validation. - Introduce necessary BUILD_FILE configurations for plugins and dependencies.
|
|
||
|
|
||
| test_cmd = f'$TEST {flags} 2>&1 | tee $TMP_DIR/test.results' | ||
| test_cmd = f'$TEST {flags} 2>&1 | tee -- "$TMP_DIR/test.results"' |
There was a problem hiding this comment.
Why have you made this change?
There was a problem hiding this comment.
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.
| elif isinstance(definitions, list): | ||
| test_definitions = list(definitions) | ||
| else: | ||
| test_definitions = list(definitions) |
There was a problem hiding this comment.
This can be simplified
| # 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) | ||
| if testing_definition not in test_definitions: | ||
| test_definitions.append(testing_definition) |
There was a problem hiding this comment.
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
| _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): |
There was a problem hiding this comment.
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
| @@ -0,0 +1,8 @@ | |||
| [Parse] | |||
| BuildFileName = BUILD_FILE | |||
| BuildFileName = BUILD | |||
There was a problem hiding this comment.
| BuildFileName = BUILD |
2 participants
