pytest-pants: add JUnit XML output support, make test collector optional

docs/pytest-pants.md

@@ -10,64 +10,70 @@
 >
 > - Filter test files
 > - Split slow files by individual test example
+> - Filter test by tags
 > - Skip tests
 
-To integrate bktec with pants, you need to [install and configure Buildkite Test Collector for pytest](https://buildkite.com/docs/test-engine/python-collectors#pytest-collector) first. Then set the `BUILDKITE_TEST_ENGINE_TEST_RUNNER` environment variable to `pytest-pants`.
+Set the `BUILDKITE_TEST_ENGINE_TEST_RUNNER` environment variable to `pytest-pants` to use bktec with pants.
 
-Look at the example configuration files in the [pytest_pants testdata directory](../internal/runner/testdata/pytest_pants) for an example of how to add buildkite-test-collector to the pants resolve used by pytest. Specifically:
+```sh
+export BUILDKITE_TEST_ENGINE_TEST_RUNNER=pytest-pants
+bktec run
+```
 
-- [pants.toml](../internal/runner/testdata/pytest_pants/pants.toml) - pants configuration
-- [3rdparty/python/BUILD](../internal/runner/testdata/pytest_pants/3rdparty/python/BUILD) - python_requirement targets
-- [3rdparty/python/pytest-requirements.txt](../internal/runner/testdata/pytest_pants/3rdparty/python/pytest-requirements.txt) - Python requirements.txt
+bktec works with pytest-pants in two modes depending on which result flag you include in your test command:
 
-In the example in the repository, you would need to generate a lockfile next, i.e.
+- **With `--junit-xml={{resultPath}}`** (default): bktec uses JUnit XML output. No additional dependencies are required.
+- **With `--json={{resultPath}}`**: bktec uses the collector's JSON output for richer test result data. This requires [Buildkite Test Collector for pytest](https://buildkite.com/docs/test-engine/python-collectors#pytest-collector) to be added to the pants resolve used by pytest.
+
+## Configure test command
+
+There is no default command for pants. You must set `BUILDKITE_TEST_ENGINE_TEST_CMD`.
+
+bktec determines the output format by detecting `--junit-xml` or `--json=` in your test command. Below are a few recommendations for specific scenarios.
+
+### JUnit XML output (default)
 
 ```sh
-pants generate-lockfiles --resolve=pytest
+export BUILDKITE_TEST_ENGINE_TEST_CMD="pants --filter-target-type=python_test test //:: -- --junit-xml={{resultPath}}"
 ```
 
-Only running `pants test` with `python_test` targets is supported at this time.
+This command is a good option if you want to run all python tests in your repository.
 
 ```sh
-export BUILDKITE_TEST_ENGINE_TEST_RUNNER=pytest-pants
-export BUILDKITE_TEST_ENGINE_TEST_CMD="pants --filter-target-type=python_test --changed-since=HEAD~1 test -- --json={{resultPath}} --merge-json"
-bktec run
+export BUILDKITE_TEST_ENGINE_TEST_CMD="pants --filter-target-type=python_test --changed-since=HEAD~1 test -- --junit-xml={{resultPath}}"
 ```
 
-## Configure test command
+This command is a good option if you want to only run the python tests that were impacted by any changes made since `HEAD~1`. Checkout [pants Advanced target selection doc][pants-advanced-target-selection] for more information on `--changed-since`.
 
-While pants support is experimental there is no default command. That means it is required to set `BUILDKITE_TEST_ENGINE_TEST_CMD`.
-Below are a few recommendations for specific scenarios:
+> [!IMPORTANT]
+> Make sure to include `-- --junit-xml={{resultPath}}` in your custom pants test command, as bktec requires this option to read the test results for retries and verification purposes.
 
----
+### JSON output (with buildkite-test-collector)
 
-```sh
-export BUILDKITE_TEST_ENGINE_TEST_CMD="pants --filter-target-type=python_test test //:: -- --json={{resultPath}} --merge-json""
-```
+To use JSON output, add `buildkite-test-collector` to the pants resolve used by pytest. Look at the example configuration files in the [pytest_pants testdata directory](../internal/runner/testdata/pytest_pants) for reference:
 
-This command is a good option if you want to run all python tests in your repository.
+- [pants.toml](../internal/runner/testdata/pytest_pants/pants.toml) - pants configuration
+- [3rdparty/python/BUILD](../internal/runner/testdata/pytest_pants/3rdparty/python/BUILD) - python_requirement targets
+- [3rdparty/python/pytest-requirements.txt](../internal/runner/testdata/pytest_pants/3rdparty/python/pytest-requirements.txt) - Python requirements.txt
 
----
+After updating the configuration, generate a lockfile:
 
 ```sh
-export BUILDKITE_TEST_ENGINE_TEST_CMD="pants --filter-target-type=python_test --changed-since=HEAD~1 test -- --json={{resultPath}} --merge-json"
+pants generate-lockfiles --resolve=pytest
 ```
 
-This command is a good option if you want to only run the python tests that were
-impacted by any changes made since `HEAD~1`. Checkout [pants Advanced target
-selection doc][pants-advanced-target-selection] for more information on
-`--changed-since`.
+Then set your test command to use `--json={{resultPath}} --merge-json`:
 
----
-
-In both commands, `{{resultPath}}` is replaced with a unique temporary path created by bktec. `--json` option is a custom pytest option added by Buildkite Test Collector to save the result into a JSON file at given path. You can further customize the test command for your specific use case.
+```sh
+export BUILDKITE_TEST_ENGINE_TEST_CMD="pants --filter-target-type=python_test test //:: -- --json={{resultPath}} --merge-json"
+```
 
 > [!IMPORTANT]
-> Make sure to append `-- --json={{resultPath}} --merge-json` in your custom pants test command, as bktec requires these options to read the test results for retries and verification purposes.
+> Make sure to include `-- --json={{resultPath}} --merge-json` in your custom pants test command, as bktec requires these options to read the test results for retries and verification purposes.
 
 ## Filter test files
 
-There is not support for filtering test files at this time.
+There is no support for filtering test files at this time.
 
 ## Automatically retry failed tests
 

internal/runner/pytest_pants.go

@@ -13,20 +13,28 @@ import (
 
 type PytestPants struct {
 	RunnerConfig
+	resultFormat string
 }
 
 func (p PytestPants) Name() string {
 	return "pytest-pants"
 }
 
 func NewPytestPants(c RunnerConfig) PytestPants {
-	fmt.Fprintln(os.Stderr, "Info: Python package 'buildkite-test-collector' is required and will not be verified by bktec. Please ensure it is added to the pants resolve used by pytest. See https://github.com/buildkite/test-engine-client/blob/main/docs/pytest-pants.md for more information.")
-
 	if c.TestCommand == "" {
 		fmt.Fprintln(os.Stderr, "Error: The test command must be set via BUILDKITE_TEST_ENGINE_TEST_CMD.")
 		os.Exit(1)
 	}
 
+	var resultFormat string
+	switch {
+	case strings.Contains(c.TestCommand, "--junit-xml"):
+		resultFormat = "junit"
+	case strings.Contains(c.TestCommand, "--json="):
+		resultFormat = "json"
+		fmt.Fprintln(os.Stderr, "Info: Python package 'buildkite-test-collector' is required and will not be verified by bktec. Please ensure it is added to the pants resolve used by pytest. See https://github.com/buildkite/test-engine-client/blob/main/docs/pytest-pants.md for more information.")
+	}
+
 	if c.TestFilePattern != "" || c.TestFileExcludePattern != "" {
 		fmt.Fprintln(os.Stderr, "Warning: Pants test runner variant does not support discovering test files. Please ensure the test command is set correctly via BUILDKITE_TEST_ENGINE_TEST_CMD and do *not* set either:")
 		fmt.Fprintf(os.Stderr, "  BUILDKITE_TEST_ENGINE_TEST_FILE_PATTERN=%q\n", c.TestFilePattern)
@@ -42,12 +50,26 @@ func NewPytestPants(c RunnerConfig) PytestPants {
 	}
 
 	if c.ResultPath == "" {
-		c.ResultPath = getRandomTempFilename("pytest-results.json")
+		if resultFormat == "junit" {
+			c.ResultPath = getRandomTempFilename("pytest-results.xml")
+		} else {
+			c.ResultPath = getRandomTempFilename("pytest-results.json")
+		}
 	}
 
 	return PytestPants{
 		RunnerConfig: c,
+		resultFormat: resultFormat,
+	}
+}
+
+func (p PytestPants) ResultFormat() string {
+	if p.resultFormat == "junit" {
+		return "junit"
 	}
+	// JSON results are uploaded by buildkite-test-collector directly, so
+	// returning "" prevents bktec from double-uploading.
+	return ""
 }
 
 func (p PytestPants) SupportedFeatures() SupportedFeatures {
@@ -76,13 +98,24 @@ func (p PytestPants) Run(result *RunResult, testCases []plan.TestCase, retry boo
 		return cmdErr
 	}
 
-	tests, parseErr := parseTestEngineTestResult(p.ResultPath)
+	var parseErr error
+	if p.resultFormat == "junit" {
+		parseErr = p.runParseJUnit(result)
+	} else {
+		parseErr = p.runParseJSON(result)
+	}
 
 	if parseErr != nil {
-		fmt.Printf("Buildkite Test Engine Client: Failed to read json output, failed tests will not be retried: %v\n", parseErr)
-		// We don't want to fail the build if we fail to parse the report,
-		// therefore we return the command error (which can be nil), instead of the parse error.
-		return cmdErr
+		fmt.Printf("Buildkite Test Engine Client: Failed to read test output, failed tests will not be retried: %v\n", parseErr)
+	}
+
+	return cmdErr
+}
+
+func (p PytestPants) runParseJSON(result *RunResult) error {
+	tests, parseErr := parseTestEngineTestResult(p.ResultPath)
+	if parseErr != nil {
+		return parseErr
 	}
 
 	for _, test := range tests {
@@ -97,8 +130,27 @@ func (p PytestPants) Run(result *RunResult, testCases []plan.TestCase, retry boo
 		}, test.Result)
 	}
 
-	// Return any command error after processing the report
-	return cmdErr
+	return nil
+}
+
+func (p PytestPants) runParseJUnit(result *RunResult) error {
+	tests, parseErr := loadAndParseJUnitXML(p.ResultPath)
+	if parseErr != nil {
+		return parseErr
+	}
+
+	for _, test := range tests {
+		scope, path := pytestNodeIDFromJUnit(test.Classname, test.Name)
+		result.RecordTestResult(plan.TestCase{
+			Identifier: path,
+			Format:     plan.TestCaseFormatExample,
+			Scope:      scope,
+			Name:       test.Name,
+			Path:       path,
+		}, test.Result)
+	}
+
+	return nil
 }
 
 func (p PytestPants) GetFiles() ([]string, error) {
@@ -125,14 +177,22 @@ func (p PytestPants) CommandNameAndArgs(testCases []plan.TestCase, retry bool) (
 		return "", []string{}, fmt.Errorf("please ensure the test command in BUILDKITE_TEST_ENGINE_TEST_CMD includes a -- separator")
 	}
 
-	// Check that both required flags are after the --
 	afterDash := parts[1]
-	if !strings.Contains(afterDash, "--json={{resultPath}}") {
-		return "", []string{}, fmt.Errorf("please ensure the test command in BUILDKITE_TEST_ENGINE_TEST_CMD includes --json={{resultPath}} after the -- separator")
-	}
 
-	if !strings.Contains(afterDash, "--merge-json") {
-		return "", []string{}, fmt.Errorf("please ensure the test command in BUILDKITE_TEST_ENGINE_TEST_CMD includes --merge-json after the -- separator")
+	switch p.resultFormat {
+	case "junit":
+		if !strings.Contains(afterDash, "--junit-xml={{resultPath}}") {
+			return "", []string{}, fmt.Errorf("please ensure the test command in BUILDKITE_TEST_ENGINE_TEST_CMD includes --junit-xml={{resultPath}} after the -- separator")
+		}
+	case "json":
+		if !strings.Contains(afterDash, "--json={{resultPath}}") {
+			return "", []string{}, fmt.Errorf("please ensure the test command in BUILDKITE_TEST_ENGINE_TEST_CMD includes --json={{resultPath}} after the -- separator")
+		}
+		if !strings.Contains(afterDash, "--merge-json") {
+			return "", []string{}, fmt.Errorf("please ensure the test command in BUILDKITE_TEST_ENGINE_TEST_CMD includes --merge-json after the -- separator")
+		}
+	default:
+		return "", []string{}, fmt.Errorf("please ensure the test command in BUILDKITE_TEST_ENGINE_TEST_CMD includes either --junit-xml={{resultPath}} or --json={{resultPath}} after the -- separator")
 	}
 
 	cmd = strings.Replace(cmd, "{{resultPath}}", p.ResultPath, 1)

internal/runner/pytest_pants_test.go

@@ -153,6 +153,148 @@ func TestPytestPantsGetExamples(t *testing.T) {
 	}
 }
 
+func TestPytestPantsRun_JUnit_TestPassed(t *testing.T) {
+	changeCwd(t, "./testdata/pytest_pants")
+
+	pytest := PytestPants{
+		RunnerConfig: RunnerConfig{
+			TestCommand: "pants test //passing_test.py -- --junit-xml={{resultPath}}",
+			ResultPath:  "result-passed.xml",
+		},
+		resultFormat: "junit",
+	}
+
+	testCases := []plan.TestCase{{Path: "passing_test.py"}}
+	result := NewRunResult([]plan.TestCase{})
+	err := pytest.Run(result, testCases, false)
+	if err != nil {
+		t.Errorf("PytestPants.Run(%q) error = %v", testCases, err)
+	}
+
+	if result.Status() != RunStatusPassed {
+		t.Errorf("PytestPants.Run(%q) RunResult.Status = %v, want %v", testCases, result.Status(), RunStatusPassed)
+	}
+}
+
+func TestPytestPantsRun_JUnit_TestFailed(t *testing.T) {
+	changeCwd(t, "./testdata/pytest_pants")
+
+	pytest := PytestPants{
+		RunnerConfig: RunnerConfig{
+			TestCommand: "pants test //:: -- --junit-xml={{resultPath}}",
+			ResultPath:  "result-failed.xml",
+		},
+		resultFormat: "junit",
+	}
+
+	testCases := []plan.TestCase{{Path: "failing_test.py"}}
+	result := NewRunResult([]plan.TestCase{})
+	err := pytest.Run(result, testCases, false)
+
+	exitError := new(exec.ExitError)
+	assert.ErrorAs(t, err, &exitError)
+
+	if result.Status() != RunStatusFailed {
+		t.Errorf("PytestPants.Run(%q) RunResult.Status = %v, want %v", testCases, result.Status(), RunStatusFailed)
+	}
+
+	failedTest := result.FailedTests()
+	if len(failedTest) != 1 {
+		t.Errorf("len(result.FailedTests()) = %d, want 1", len(failedTest))
+	}
+
+	wantFailedTests := []plan.TestCase{
+		{
+			Format:     "example",
+			Identifier: "tests/failing_test.py::test_failed",
+			Name:       "test_failed",
+			Path:       "tests/failing_test.py::test_failed",
+			Scope:      "tests/failing_test.py",
+		},
+	}
+
+	if diff := cmp.Diff(failedTest, wantFailedTests); diff != "" {
+		t.Errorf("PytestPants.Run(%q) RunResult.FailedTests() diff (-got +want):\n%s", testCases, diff)
+	}
+}
+
+func TestPytestPantsResultFormat_JUnit(t *testing.T) {
+	pytest := PytestPants{resultFormat: "junit"}
+	if got := pytest.ResultFormat(); got != "junit" {
+		t.Errorf("ResultFormat() = %q, want %q", got, "junit")
+	}
+}
+
+func TestPytestPantsResultFormat_JSON(t *testing.T) {
+	pytest := PytestPants{resultFormat: "json"}
+	if got := pytest.ResultFormat(); got != "" {
+		t.Errorf("ResultFormat() = %q, want %q", got, "")
+	}
+}
+
+func TestPytestPantsCommandNameAndArgs_JUnit_ValidCommand(t *testing.T) {
+	testCases := []plan.TestCase{{Path: "failing_test.py"}, {Path: "passing_test.py"}}
+	testCommand := "pants test //:: -- --junit-xml={{resultPath}}"
+
+	pytest := PytestPants{
+		RunnerConfig: RunnerConfig{
+			TestCommand: testCommand,
+			ResultPath:  "result.xml",
+		},
+		resultFormat: "junit",
+	}
+
+	gotName, gotArgs, err := pytest.CommandNameAndArgs(testCases, false)
+	if err != nil {
+		t.Errorf("commandNameAndArgs(%q, %q) error = %v", testCases, testCommand, err)
+	}
+
+	wantName := "pants"
+	wantArgs := []string{"test", "//::", "--", "--junit-xml=result.xml"}
+
+	if diff := cmp.Diff(gotName, wantName); diff != "" {
+		t.Errorf("commandNameAndArgs() diff (-got +want):\n%s", diff)
+	}
+	if diff := cmp.Diff(gotArgs, wantArgs); diff != "" {
+		t.Errorf("commandNameAndArgs() diff (-got +want):\n%s", diff)
+	}
+}
+
+func TestPytestPantsCommandNameAndArgs_JUnit_WithoutResultPath(t *testing.T) {
+	testCases := []plan.TestCase{{Path: "failing_test.py"}}
+	testCommand := "pants test //:: -- --merge-json"
+
+	pytest := PytestPants{
+		RunnerConfig: RunnerConfig{
+			TestCommand: testCommand,
+			ResultPath:  "result.xml",
+		},
+		resultFormat: "junit",
+	}
+
+	_, _, err := pytest.CommandNameAndArgs(testCases, false)
+	if err == nil {
+		t.Error("commandNameAndArgs() error = nil, want error")
+	}
+}
+
+func TestPytestPantsCommandNameAndArgs_NeitherResultFlag(t *testing.T) {
+	testCases := []plan.TestCase{{Path: "failing_test.py"}}
+
+	pytest := PytestPants{
+		RunnerConfig: RunnerConfig{
+			TestCommand: "pants test //:: -- --some-other-flag",
+			ResultPath:  "result.json",
+		},
+		resultFormat: "",
+	}
+
+	_, _, err := pytest.CommandNameAndArgs(testCases, false)
+	if err == nil {
+		t.Error("commandNameAndArgs() error = nil, want error")
+	}
+}
+
 func TestPytestPantsCommandNameAndArgs_WithoutMergeJson(t *testing.T) {
 	testCases := []plan.TestCase{{Path: "failing_test.py"}, {Path: "passing_test.py"}}
 	testCommand := "pants test //:: -- --json={{resultPath}}"