cockroachdb / cockroachdb/cockroach

logictest: brittleness to unrelated changes

Open
#135,989 0 comments 0 reactions 0 assignees View on GitHub
C-enhancement T-sql-queries
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

It has been observed that some seemingly isolated changes, for example adding a new table to the system catalog that is not yet used by or affecting any other features, can require updating large numbers of tests, in particular logictests, that are over-specifying aspects of the system beyond strictly the behavior the test was written to test.

For example, a test of the optimizer's join planning may today be written by looking at the exact string explaining the query trace, however this includes the key ranges -- and range IDs containing those key ranges -- scanned by the requests. This test was intended to test the behavior of the join plan, not the specific ID of the range containing the first user created table (the one being joined here), but this approach to testing -- asserting the exact content of some large string that depends on many behavior in addition to the behavior being tested -- means the test now is also a test of this range ID. Thus this test needs to be updated, along with thousands of other tests right now, if this ID changes/the set of indexes+columns in the catalog changes/the ID of the first table created changes/etc.

The aggregate effect of this across thousands of tests is that engineers become habituated to simply running `--rewrite` on swaths of tests they don't believe are relevant to their change if they fail, but this defeats the exact point of a test, which is to detect _and prevent_ unintended behavior changes, i.e. bugs, from being introduced by a code change.

Some ideas proposed in slack discussions of this:

- Add a length limit to the default logictest driver on the size of the output it will compare, to nudge the author of new tests to be more selective about what they they want to test, e.g. use a regex or more specific SELECT/LIMIT clause to extract only the specific values they need to observe.
- Extend the logictest to default to regex'ing anything that looks like a specific tableID or rangeID to placeholders, e.g. `/Table/\d+` -> /Table/XX and `r\d+` -> rX.
- Pick a few members of the system and virtual catalog to serve as canaries and write a linter that their names they don't appear anywhere outside some allowlist, to flag tests that are blindly enumerating entire catalogs without a WHERE clause

Jira issue: CRDB-44819

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.