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

stress: skip.UnderStress() doesn't #84312

Closed
HonoreDB opened this issue Jul 12, 2022 · 0 comments · Fixed by #84364
Closed

stress: skip.UnderStress() doesn't #84312

HonoreDB opened this issue Jul 12, 2022 · 0 comments · Fixed by #84364
Labels
A-build-system C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-dev-inf

Comments

@HonoreDB
Copy link
Contributor

HonoreDB commented Jul 12, 2022

Describe the problem

skip.UnderStress(), skip.UnderStressRace(), and skip.NightlyStress() all reference the COCKROACH_NIGHTLY_STRESS environment variable to check if we're running under stress. This variable isn't set by make stress or ./dev test --stress, though, it's set by shell scripts that call those commands. So running dev test --stress locally won't skip those tests.

I haven't done the forensics to figure out whether this is intentional--it's plausibly a good idea. But it is definitely confusing, because some people will use skip.UnderStress if they have a test that fails under stress (due to a timeout or something) and thus effectively break ./dev test --stress for their package outside nightly test runs.

To Reproduce

Create a test that goes

skip.UnderStress(t)
t.FailNow()

and run it with ./dev test --stress.

Expected behavior

Maybe rename it to skip.UnderNightlyStress() and call it a day?

My preference though would I think be to have the ability to actually skip tests under stress. For example, right after I file this issue I'm going to go add skip.UnderStressRace() to TestChangefeedRestartMultiNode because it reliably hangs under stressrace, I think because it's calling TestCluster.WaitForFullReplication() and under stressrace we just never get there. I'd rather not have that be a trap for anyone running tests locally.

So maybe --stress could set a different flag that's also visible to skip, and then we could have an API like

skip.WhenExpensive() // skips when SKIP_EXPENSIVE_TESTS is set, set this in nightly stress tests
skip.UnderStress() // skips when CRDB_STRESS is set, set this during all stress invocations

Jira issue: CRDB-17584

@HonoreDB HonoreDB added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-build-system T-dev-inf labels Jul 12, 2022
@craig craig bot closed this as completed in 477644e Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-system C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-dev-inf
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant