Merge pull request #252 from buildkite/tet-491-passed-on-retry-count-…

…incorrect

Fix issue where a test incorrectly reported as "Passed on Retry"

internal/runner/jest.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"os"
 	"os/exec"
+	"path/filepath"
 	"regexp"
 	"slices"
 	"strings"
@@ -112,12 +113,22 @@ func (j Jest) Run(result *RunResult, testCases []plan.TestCase, retry bool) erro
 				status = TestStatusSkipped
 			}
 
+			wordDir, err := os.Getwd()
+			if err != nil {
+				return fmt.Errorf("failed to get current working directory: %v", err)
+			}
+			testPath, err := filepath.Rel(wordDir, testResult.FileName)
+			if err != nil {
+				return fmt.Errorf("failed to get relative path of test file: %v", err)
+			}
+
 			// The scope and name has to match with the scope generated by Buildkite test collector.
 			// For more details, see:
 			// [Buildkite Test Collector - Jest implementation](https://github.com/buildkite/test-collector-javascript/blob/42b803a618a15a07edf0169038ef4b5eba88f98d/jest/reporter.js#L40)
 			testCase := plan.TestCase{
 				Name:  example.Title,
 				Scope: strings.Join(example.AncestorTitles, " "),
+				Path:  fmt.Sprintf("%s:%d:%d", testPath, example.Location.Line, example.Location.Column),
 			}
 
 			result.RecordTestResult(testCase, status)
@@ -136,13 +147,18 @@ type JestExample struct {
 	Status         string   `json:"status"`
 	Title          string   `json:"title"`
 	AncestorTitles []string `json:"ancestorTitles"`
+	Location       struct {
+		Line   int
+		Column int
+	}
 }
 
 type JestReport struct {
 	NumFailedTests            int
 	NumRuntimeErrorTestSuites int
 	TestResults               []struct {
 		AssertionResults []JestExample
+		FileName         string `json:"name"`
 	}
 }
 

internal/runner/jest_test.go

@@ -134,6 +134,7 @@ func TestJestRun_TestFailed(t *testing.T) {
 		{
 			Scope: "this will fail",
 			Name:  "for sure",
+			Path:  "failure.spec.js:2:3",
 		},
 	}
 
@@ -176,12 +177,12 @@ func TestJestRun_TestSkipped(t *testing.T) {
 		t.Errorf("Jest.Run(%q) RunResult.Status = %v, want %v", testCases, result.Status(), RunStatusPassed)
 	}
 
-	test := result.tests["this will be skipped/for sure"]
+	test := result.tests["this will be skipped/for sure/skipped.spec.js:2:3"]
 	if test.Status != TestStatusSkipped {
 		t.Errorf("Jest.Run(%q) test.Status = %v, want %v", testCases, test.Status, TestStatusSkipped)
 	}
 
-	todoTest := result.tests["this will be skipped/todo yeah"]
+	todoTest := result.tests["this will be skipped/todo yeah/skipped.spec.js:6:6"]
 	if todoTest.Status != TestStatusSkipped {
 		t.Errorf("Jest.Run(%q) todoTest.Status = %v, want %v", testCases, todoTest.Status, TestStatusSkipped)
 	}

internal/runner/playwright_test.go

@@ -124,7 +124,7 @@ func TestPlaywrightRun_TestSkipped(t *testing.T) {
 		t.Errorf("Playwright.Run(%q) RunResult.Status = %v, want %v", testCases, result.Status(), RunStatusPassed)
 	}
 
-	test := result.tests[" chromium skipped.spec.js it is skipped/it is skipped"]
+	test := result.tests[" chromium skipped.spec.js it is skipped/it is skipped/skipped.spec.js:4"]
 	if test.Status != TestStatusSkipped {
 		t.Errorf("Playwright.Run(%q) test.Status = %v, want %v", testCases, test.Status, TestStatusSkipped)
 	}

internal/runner/rspec_test.go

@@ -202,7 +202,7 @@ func TestRspecRun_TestSkipped(t *testing.T) {
 		t.Errorf("Rspec.Run(%q) RunResult.Status = %v, want %v", testCases, result.Status(), RunStatusPassed)
 	}
 
-	test := result.tests["skipped/is skipped"]
+	test := result.tests["skipped/is skipped/./testdata/rspec/spec/skipped_spec.rb[1:1]"]
 	if test.Status != TestStatusSkipped {
 		t.Errorf("Rspec.Run(%q) test.Status = %v, want %v", testCases, test.Status, TestStatusSkipped)
 	}

internal/runner/run_result.go

@@ -36,7 +36,7 @@ func NewRunResult(mutedTests []plan.TestCase) *RunResult {
 	}
 
 	for _, testCase := range mutedTests {
-		identifier := testIdentifier(testCase)
+		identifier := mutedTestIdentifier(testCase)
 		r.mutedTestLookup[identifier] = true
 	}
 	return r
@@ -67,7 +67,7 @@ func (r *RunResult) RecordTestResult(testCase plan.TestCase, status TestStatus)
 	test := r.getTest(testCase)
 	test.Status = status
 	test.ExecutionCount++
-	if r.mutedTestLookup[testIdentifier(testCase)] {
+	if r.mutedTestLookup[mutedTestIdentifier(testCase)] {
 		test.Muted = true
 	}
 }

internal/runner/run_result_test.go

@@ -172,21 +172,22 @@ func TestMutedTests(t *testing.T) {
 func TestRunStatistics(t *testing.T) {
 	r := NewRunResult([]plan.TestCase{
 		// muted tests
-		{Scope: "mango", Name: "is sweet"},
+		{Scope: "mango", Name: "is sweet", Path: "mango.rb:1"},
 		{Scope: "mango", Name: "is sour"},
 		{Scope: "cat", Name: "is not a fruit"}, // unrecorded (not related to this run) test case should be ignored
 	})
 
-	// passed on first run: 2
+	// passed on first run: 3
 	r.RecordTestResult(plan.TestCase{Scope: "apple", Name: "is red"}, TestStatusPassed)
-	r.RecordTestResult(plan.TestCase{Scope: "mango", Name: "is red"}, TestStatusPassed)
+	r.RecordTestResult(plan.TestCase{Scope: "mango", Name: "is red", Path: "mango.rb:3"}, TestStatusPassed)
+	r.RecordTestResult(plan.TestCase{Scope: "mango", Name: "is red", Path: "mango.rb:7"}, TestStatusPassed) // Different tests with the same name and scope should be counted separately
 
 	//passed on retry: 1
 	r.RecordTestResult(plan.TestCase{Scope: "apple", Name: "is green"}, TestStatusFailed)
 	r.RecordTestResult(plan.TestCase{Scope: "apple", Name: "is green"}, TestStatusPassed)
 
 	// muted: 1 failed, 1 passed
-	r.RecordTestResult(plan.TestCase{Scope: "mango", Name: "is sweet"}, TestStatusPassed)
+	r.RecordTestResult(plan.TestCase{Scope: "mango", Name: "is sweet", Path: "mango.rb:5"}, TestStatusPassed) // This test matched with a test in the muted tests lists even though the path is different because we only compare the scope and name for muted tests
 	r.RecordTestResult(plan.TestCase{Scope: "mango", Name: "is sour"}, TestStatusFailed)
 
 	// failed: 1
@@ -199,8 +200,8 @@ func TestRunStatistics(t *testing.T) {
 	stats := r.Statistics()
 
 	if diff := cmp.Diff(stats, RunStatistics{
-		Total:            7,
-		PassedOnFirstRun: 2,
+		Total:            8,
+		PassedOnFirstRun: 3,
 		PassedOnRetry:    1,
 		MutedPassed:      1,
 		MutedFailed:      1,

internal/runner/test_result.go

@@ -18,6 +18,16 @@ type TestResult struct {
 	Muted          bool
 }
 
+// testIdentifier returns a unique identifier for a test case based on its scope, name and path.
+// Different tests can have the same name and scope, therefore the path is included in the identifier
+// to make it unique.
 func testIdentifier(testCase plan.TestCase) string {
+	return testCase.Scope + "/" + testCase.Name + "/" + testCase.Path
+}
+
+// mutedTestIdentifier returns a unique identifier for a muted test case based on its scope and name.
+// Test Engine server identify a unique tests by its scope and name only, therefore we need follow the same logic
+// to match a local test with the list of muted tests received from the server.
+func mutedTestIdentifier(testCase plan.TestCase) string {
 	return testCase.Scope + "/" + testCase.Name
 }

internal/runner/testdata/jest/jest.config.js

@@ -1,3 +1,8 @@
 // This file is intentionally left blank.
 // Jest requires a jest.config.js file to run. It can be empty, so it doesn't
 // serve much use other than to satisfy Jest for the Jest tests.
+const config = {
+  "testLocationInResults": true,
+};
+
+module.exports = config;