gotestyourself / gotestsum

'go test' runner with output optimized for humans, JUnit XML for CI integration, and a summary of the test results.
Apache License 2.0
2.03k stars 119 forks source link

Re-running of failed tests does not handle multiple levels of subtests #341

Closed brycekahle closed 1 year ago

brycekahle commented 1 year ago

If you have multiple levels of nesting for subtests, the re-running of failed tests does not handle re-running properly and will end up running all the levels in-between the root and the subtest.

Example JSON output:

{"Package": "pkg", "Action": "run"}
{"Package": "pkg", "Test": "TestParent", "Action": "run"}
{"Package": "pkg", "Test": "TestParent/TestNested", "Action": "run"}
{"Package": "pkg", "Test": "TestParent/TestNested/TestOne", "Action": "run"}
{"Package": "pkg", "Test": "TestParent/TestNested/TestOne", "Action": "pass"}
{"Package": "pkg", "Test": "TestParent/TestNested/TestTwo", "Action": "run"}
{"Package": "pkg", "Test": "TestParent/TestNested/TestTwo", "Action": "fail"}
{"Package": "pkg", "Test": "TestParent/TestNested", "Action": "fail"}
{"Package": "pkg", "Test": "TestParent", "Action": "fail"}
{"Package": "pkg", "Action": "fail"}

This should end up running:

go test -json -test.run=^TestParent$/^TestNested$/^TestTwo$ pkg

but it runs:

go test -json -test.run=^TestParent$/^TestNested/TestTwo$ pkg
go test -json -test.run=^TestParent$/^TestNested$ pkg

Notice there are two errors here:

  1. The regexp for the first line should be ^TestParent$/^TestNested$/^TestTwo$ (missing $ and ^ around second /)
  2. It should not run TestParent/TestNested.

I do not know what is causing error number 1, but I tracked the cause for number 2 down to the hasSubTestFailed being incorrect for the in-between levels. I added a test case that fails if added to testjson/execution_test.go:

func TestScanOutput_WithNestedSubTestFailures(t *testing.T) {
    source := []byte(`{"Package": "pkg", "Action": "run"}
    {"Package": "pkg", "Test": "TestParent", "Action": "run"}
    {"Package": "pkg", "Test": "TestParent/TestNested", "Action": "run"}
    {"Package": "pkg", "Test": "TestParent/TestNested/TestOne", "Action": "run"}
    {"Package": "pkg", "Test": "TestParent/TestNested/TestOne", "Action": "pass"}
    {"Package": "pkg", "Test": "TestParent/TestNested/TestTwo", "Action": "run"}
    {"Package": "pkg", "Test": "TestParent/TestNested/TestTwo", "Action": "fail"}
    {"Package": "pkg", "Test": "TestParent/TestNested", "Action": "fail"}
    {"Package": "pkg", "Test": "TestParent", "Action": "fail"}
    {"Package": "pkg", "Action": "fail"}`)

    handler := &captureHandler{}
    cfg := ScanConfig{
        Stdout:  bytes.NewReader(source),
        Handler: handler,
    }
    exec, err := ScanTestOutput(cfg)
    assert.NilError(t, err)
    pkg := exec.Package("pkg")
    for _, tc := range pkg.TestCases() {
        switch string(tc.Test) {
        case "TestParent/TestNested", "TestParent":
            assert.Equal(t, tc.hasSubTestFailed, true, "test %s should have failing subtests", tc.Test)
        default:
            assert.Equal(t, tc.hasSubTestFailed, false, "test %s should not have failing subtests", tc.Test)
        }
    }
}
=== RUN   TestScanOutput_WithNestedSubTestFailures
    execution_test.go:262: assertion failed: false (tc.hasSubTestFailed bool) != true (true bool): test TestParent/TestNested should have failing subtests
--- FAIL: TestScanOutput_WithNestedSubTestFailures (0.00s)
FAIL
dnephin commented 1 year ago

Thank you for the bug report! The missing ^ and $ around extra path segments definitely sounds like a bug. That should be easy to fix by splitting on all / and adding the necessary characters. The code for that is here: https://github.com/gotestyourself/gotestsum/blob/049fc26eed877666573a323de5917cf3365e0866/cmd/rerunfails.go#L141

The second issue it a bit more nuanced and more difficult to solve. There's no way to run a Go sub test without running its parent. The sub tests are defined dynamically in code, so you can't discover them without running their parent. In your example TestParent/TestNested is actually run twice (once for each call to go test). If there were 3 failing tests it would have run 4 times, etc. The problem in this case is that the extra call to go test actually re-runs all its sub tests as well. This is definitely a bug.

When I first wrote this there were limitations with the go test -run flag that required running all tests individually (https://github.com/golang/go/issues/39904). I think now that https://github.com/golang/go/commit/025308fe084264538f49924b3f52d8d6b6359658 is done (and has been in all Go versions since go1.18), this can be fixed to re-run all the tests in a package at once. That should remove most of the extra calls to parent tests.

I think FilterFailedUnique using hasSubTestFailed was probably a mistake. Instead of hasSubTestFailed, I think FilterFailedUnique needs to filter out any test names that are substrings of other test names. It should be able to do this by looking at the list of all the failed tests in the package. Once those test names are filtered to just the most unique set, then a regex to run all of them should be correct, and should minimize re-running of the intermediary sub tests.

brycekahle commented 1 year ago

I opened #342 to address the regex generation bug

brycekahle commented 1 year ago

I took a crack at fixing the problem in FilterFailedUnique in #343

dnephin commented 1 year ago

I created #347 to track a future improvement of running all the tests in a package at once, instead of each test individually. I believe this issue should be fixed now so I'm going to close it.