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

dev: replace stress with --runs_per_test #104228

Merged
merged 1 commit into from
Aug 2, 2023

Conversation

rickystewart
Copy link
Collaborator

@rickystewart rickystewart commented Jun 1, 2023

--runs_per_test does the same thing that stress does (runs a test
many times to weed out flakes), but better, in that:

  1. Better Bazel support -- we had to hack Bazel support into stress
    to keep it working
  2. Bazel gives us statistics and control over how the tests are
    scheduled in a way that is standardized as opposed to the ad-hoc
    arguments one can pass to stress
  3. Bazel can collect logs and artifacts for different test runs and
    expose them to the user in a way that stress cannot
  4. Drop-in support for remote execution when we have access to it,
    obviating the need for roachprod-stress

For now, the default implementation of dev test --stress is to
run the test 1,000 times, stopping the build if any test fails.
This is meant to replicate how people use stress (i.e., just start it
running and stop if there are any failures). The 1,000 figure is
arbitrary and meant to just be a "very high number" of times to run unit
tests. The default figure of 1,000 can be adjusted with --count. Also
update documentation with some new recipes and add extra logging to
warn about the change in behavior.

Closes #102879

Epic: none
Release note: None

@cockroach-teamcity
Copy link
Member

cockroach-teamcity commented Jun 1, 2023

CLA assistant check
All committers have signed the CLA.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rickystewart rickystewart changed the title dev: have --count use --runs_per_test instead of -test.count dev: replace stress with --runs_per_test Jun 1, 2023
@rickystewart rickystewart force-pushed the replace-stress-in-dev branch 2 times, most recently from 196234d to b87822d Compare June 2, 2023 19:15
@rickystewart rickystewart marked this pull request as ready for review June 2, 2023 19:18
@rickystewart rickystewart requested a review from a team as a code owner June 2, 2023 19:18
@rickystewart
Copy link
Collaborator Author

No rush to merge this PR by the way, we can take our time to make sure this is working correctly.

@rickystewart
Copy link
Collaborator Author

This is ready to review.

Copy link
Member

@rail rail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nit: pkg/cmd/dev/util.go:134:15: func (*dev).getDevBin is unused (U1000)

`--runs_per_test` does the same thing that `stress` does (runs a test
many times to weed out flakes), but better, in that:

1. Better Bazel support -- we had to hack Bazel support into `stress`
   to keep it working
2. Bazel gives us statistics and control over how the tests are
   scheduled in a way that is standardized as opposed to the ad-hoc
   arguments one can pass to `stress`
3. Bazel can collect logs and artifacts for different test runs and
   expose them to the user in a way that `stress` cannot
4. Drop-in support for remote execution when we have access to it,
   obviating the need for `roachprod-stress`

For now, the default implementation of `dev test --stress` is to
run the test 1,000 times, stopping the build if any test fails.
This is meant to replicate how people use `stress` (i.e., just start it
running and stop if there are any failures). The 1,000 figure is
arbitrary and meant to just be a "very high number" of times to run unit
tests. The default figure of 1,000 can be adjusted with `--count`. Also
update documentation with some new recipes and add extra logging to
warn about the change in behavior.

Closes cockroachdb#102879

Epic: none
Release note: None
@rickystewart
Copy link
Collaborator Author

bors r=rail

@craig
Copy link
Contributor

craig bot commented Aug 2, 2023

Build succeeded:

@craig craig bot merged commit 729212e into cockroachdb:master Aug 2, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bazel: print path to test log and crdb server logs after stress failure
4 participants