cockroachdb / cockroachdb/cockroach

bazci/codwowners: improve test lookup logic to handle unit test failures with ambiguous test.xml

Open
#159,708 4 comments 0 reactions 0 assignees View on GitHub
A-ci A-testeng-infra C-enhancement T-testeng
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

e.g. https://github.com/cockroachdb/cockroach/issues/159438
See Comment: https://github.com/cockroachdb/cockroach/pull/158632#issuecomment-3657926675

There is a gap in the current test owner routing mechanism when the test.xml does not contain the expected bazel test's qualified name which happens when you see `exited with error code 142` i.e. when the test binary doesn't execute (e.g. if `init()` panics or `bazel test` times out before the test binary executes). There may be other causes as well. I verified both of the above failure modes locally.

> If the test binary did not successfully execute or we encountered some sort of other severe issue, test.xml may contain no useful information including no test name and only a distorted version of the package name (e.g. pkg/sql/opt/testutils/testcat/testcat_test_/testcat_test instead of pkg/sql/opt/testutils/testcat)

[test.xml](https://teamcity.cockroachdb.com/repository/download/Cockroach_Ci_TestsIbmCloudLinuxS390x_UnitTestsS390x/20854537:id/bazel-testlogs/pkg/sql/opt/testutils/testcat/testcat_test/test.xml )
```



```

This is an issue because the current routing mechanism depends on name fields of testsuite and testcase that looks like
```


...

=== RUN TestWatchTenants test_log_scope.go:171: test logs
captured to:
...
FAIL: TestWatchTenants (1.02s)

```
i.e. a fully qualified name to the unit test including the package and test name (and optionally subtest name). If that's not present you get something that looks like this in `bazci` / `codeowners` when trying to find the owning team
[Team City Build](https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_Ci_TestsIbmCloudLinuxS390x_UnitTestsS390x/20854537?buildTab=log&linesState=300.125206.125207#%2Fbazel-testlogs%2Fpkg%2Fsql%2Fopt%2Ftestutils%2Ftestcat;%2Fbazel-testlogs%2Fpkg%2Fsql%2Fopt%2Ftestutils%2Fopttester;%2Fbazel-testlogs%2Fpkg%2Fsql%2Fopt%2Ftestutils%2Ftestutils_test)
```
[23:09:59]W: [Run unit tests] consolidating failed subtest "sql/opt/testutils/testcat/testcat_test_/testcat_test" into parent test "pkg"
[23:09:59]W: [Run unit tests] failed parent test "pkg" (no subtests)
[23:10:00]W: [Run unit tests] couldn't find test pkg in pkg/sql/opt/testutils/testcat/testcat_test_/testcat_test: warning: could not open directory 'pkg/sql/opt/testutils/testcat/testcat_test_/testcat_test/': No such file or directory
[23:10:00]W: [Run unit tests] exit status 1
[23:10:00]W: [Run unit tests] couldn't find test pkg in pkg/sql/opt/testutils/testcat/testcat_test_: warning: could not open directory 'pkg/sql/opt/testutils/testcat/testcat_test_/': No such file or directory
[23:10:00]W: [Run unit tests] exit status 1
[23:10:00]W: [Run unit tests] couldn't find test pkg in pkg/sql/opt/testutils/testcat: exit status 1
[23:10:00]W: [Run unit tests] couldn't find test pkg in pkg/sql/opt/testutils: exit status 1
[23:10:00]W: [Run unit tests] couldn't find test pkg in pkg/sql/opt: exit status 1
[23:10:00]W: [Run unit tests] couldn't find test pkg in pkg/sql: exit status 1
[23:10:00]W: [Run unit tests] couldn't find test pkg in pkg: exit status 1
[23:10:00]W: [Run unit tests] error getting file:line for pkg/sql/opt/testutils/testcat/testcat_test_/testcat_test.pkg: could not find test pkg
[23:10:00]W: [Run unit tests] assigning pkg/sql/opt/testutils/testcat/testcat_test_/testcat_test.pkg to 'test-eng' as catch-all
[23:10:00]W: [Run unit tests] created new GitHub issue #159438
```

`testsuite.name=name="pkg/sql/opt/testutils/testcat/testcat_test_/testcat_test` and `testcase.name="name="pkg/sql/opt/testutils/testcat/testcat_test_/testcat_test"` are the problematic fields and from what I looked up, it seems like `bazel test` can't give us better names

So then the next question is can we translate the ambiguous names into their fully qualified names so the current routing mechanism can work?

> It is not hard to translate pkg/sql/opt/testutils/testcat/testcat_test_/testcat_test into pkg/sql/opt/testutils/testcat, so at least that part is possible: we can produce the package name in this scenario.
> However, it is impossible to attribute the failure to one particular test in this scenario, and in fact there might not even be one particular test failure at all, for example if the thing timed out before any tests started running.
> As you have observed, the test lookup logic depends on us finding the test name so we can match it with a filename. The problem you will encounter is that the tests in any one directory can be owned by multiple teams. For example, pkg/foo/bar/a_test.go can be owned by KV, and pkg/foo/bar/b_test.go can be owned by SQL queries. So, just finding the package name pkg/foo/bar is not enough information to figure out a test owner.

**Potential Root Cause Followups**
> See if the package directory itself (e.g., pkg/sql/opt/testutils/testcat) has an owner. If it does, that's a good guess for who the owner should be.

> Enumerate all the possible owners under the directory (for example, say teams A, B, and C all own files in the package pkg/sql/opt/testutils/testcat). Then choose one of these randomly or arbitrarily.

Note:
> BUILD.bazel and other random files are not good candidates for solving this problem. BUILD.bazel files are all owned by dev-inf. Git committers are also not a good strategy in my opinion, for the sole reason that people routinely commit to code they don't necessarily own -- again, dev-inf is a good example of this, since we will make non-functional changes to different parts of the codebase when upgrading Go or making similar changes.

**Minimum Follow Ups**
We should add some better titles / descriptions for this failure mode so test-eng can more easily triage / route these tests to the appropriate teams.
```
Current Issue Title: pkg/sql/opt/testutils/testcat/testcat_test_/testcat_test: pkg failed
Possible Issue New Title: pkg/sql/opt/testutils/testcat/testcat_test_/testcat_test failed
Possible New Description: Unit test failed with error 142. Test likely never ran due to init() error or bazci test level timeout. If initial routed team (test-eng) is not the owner, manually route to owning team.
```

See the above linked comment for a branch with some very rough notes and a rough explanation on the code level.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.