Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util/must: improve Expensive build tag gating #107425

Closed
erikgrinaker opened this issue Jul 23, 2023 · 2 comments
Closed

util/must: improve Expensive build tag gating #107425

erikgrinaker opened this issue Jul 23, 2023 · 2 comments
Assignees
Labels
A-testing Testing tools and infrastructure C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-testeng TestEng Team

Comments

@erikgrinaker
Copy link
Contributor

erikgrinaker commented Jul 23, 2023

In #106508, must.Expensive() assertions are gated on the race build tag via util.RaceEnabled. We should reconsider this. Some considerations:

  1. They should not be enabled in any binaries by default.

  2. They should be possible to enable when building binaries.

  3. They should never be enabled in benchmarks (neither roachperf nor go bench).

  4. They should be enabled in as many tests as possible.

  5. They may cause timing-sensitive tests to flake. If they do, we must be able to skip certain tests under expensive assertions, similarly to skip.UnderRace and skip.UnderDeadlock.

  6. Pebble's invariant assertions should be enabled via the same mechanism. They are currently gated on invariants or race build tags.

  7. We may need two degrees of expensive assertions (moderate and very), similarly to how we currently gate some on crdb_test and others on race.

Some options:

  1. Gate them on crdb_test, currently used for all Go tests.

    • May violate 5: we can't exclude tests from crdb_test (they would never run). But maybe it won't be needed.
    • May violate 2 and 4: does it make sense to build binaries with crdb_test? If so, can we use them in roachtests?
    • May violate 7: there only one degree of expensive assertion.
  2. Gate them on invariants, a new tag for CRDB, but already used by Pebble. Would need a new nightly run (possibly both for Go tests and roachtests).

    • May violate 4: depends on how widely we enable them, e.g. new nightly run, nightly stress run, CI run, roachtest run, etc.
  3. Gate them on race, currently used for Go tests under race detector.

    • Violates 2: nodes are too slow to run under race.
    • Violates 4: only enabled in race builds, where many tests are skipped, and can't be used in roachtests.

I think I'm leaning towards trying 1 first, and if it doesn't work because too many timing-sensitive tests start flaking, do 2. To determine this quickly, we can enable Pebble's invariant tests under crdb_test, and use Expensive for some of the most expensive assertions in our hottest code paths (e.g. MVCC iterators).

Jira issue: CRDB-30035

@erikgrinaker erikgrinaker added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-testing Testing tools and infrastructure T-testeng TestEng Team labels Jul 23, 2023
@erikgrinaker erikgrinaker self-assigned this Jul 23, 2023
@blathers-crl
Copy link

blathers-crl bot commented Jul 23, 2023

cc @cockroachdb/test-eng

@erikgrinaker erikgrinaker changed the title util/must: improve Expensive gating util/must: improve Expensive build tag gating Jul 23, 2023
@erikgrinaker
Copy link
Contributor Author

must is no more, see #108272.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing Testing tools and infrastructure C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-testeng TestEng Team
Projects
No open projects
Status: Done
Development

No branches or pull requests

1 participant