testutils: add infrastructure for reusable test fixtures#99624
testutils: add infrastructure for reusable test fixtures#99624craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
27eb2da to
fa91cd1
Compare
jbowens
left a comment
There was a problem hiding this comment.
thanks for doing this! using make for these has been a nuisance
Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @erikgrinaker, @RaduBerinde, and @rickystewart)
pkg/testutils/testfixtures/test_fixtures.go line 99 at r1 (raw file):
tb.Logf("generating fixture %q in %q", name, dir) generate(dir) if err := os.WriteFile(completedPath, []byte("foo"), 0755); err != nil {
maybe should we write the current timestamp to the file?
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/testutils/testfixtures/test_fixtures.go line 99 at r1 (raw file):
Previously, jbowens (Jackson Owens) wrote…
maybe should we write the current timestamp to the file?
Hehe, that same thought crossed my mind, but as I was doing it I realized the file will have a modification time anyway. Is there some other info we may want in there?
jbowens
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/testutils/testfixtures/test_fixtures.go line 99 at r1 (raw file):
Previously, RaduBerinde wrote…
Hehe, that same thought crossed my mind, but as I was doing it I realized the file will have a modification time anyway. Is there some other info we may want in there?
:) good point. I think ideally we'd have the git SHA that built the fixture, but I don't think we have that available during test builds anymore. Can't think of anything else.
|
@rickystewart could I get a review of the |
rickystewart
left a comment
There was a problem hiding this comment.
Benchmarks run in CI. Do any of the scripts in build/teamcity need to updated accordingly to give the benchmarks write-access to the cache directory?
pkg/cmd/dev/test.go
Outdated
| args = append(args, "--test_sharding_strategy=disabled") | ||
| } | ||
|
|
||
| args = append(args, d.getGoTestEnvArgs()...) |
There was a problem hiding this comment.
Looks like this reusable fixture stuff is used for benchmarks rather than unit tests (which is the correct behavior as far as I can tell). Can you please add this extra argument to dev bench invocations and NOT dev test invocations?
RaduBerinde
left a comment
There was a problem hiding this comment.
The bench build inside Bazel Extended CI passed on this PR. I checked the logs from that build and we are using /home/roach/.cache/:
generating fixture "mvcc-fmtver_10-numKeys_100000-numVersions_2-valueBytes_8-numColFams_0-numRangeKeys_0-transactional_true" in "/home/roach/.cache/crdb-test-fixtures/mvcc-fmtver_10-numKeys_100000-numVersions_2-valueBytes_8-numColFams_0-numRangeKeys_0-transactional_true"
But I'm not sure how that works because bench_impl.sh doesn't pass the env flag or add .cache to the sandbox.. Any idea how it works? Perhaps they are not running inside a sandbox here?
In any case, note that if we are in a sandbox and the special env var isn't set, we fall back to using a temp dir (which would also be ok behavior for CI).
Reviewable status:
Ah -- so in CI the benchmarks run with |
Certain storage-related tests/benchmarks generate fixtures and attempt to reuse them across invocations. This is important because fixtures can be large and slow to generate; but more importantly the generation is non-deterministic and we want to use the exact same fixture when comparing benchmark data. Currently the tests achieve this by using a `.gitignore`d subdirectory inside the source tree. This does not work with bazel (which executes the test in a sandbox). This commit addresses the issue by adding new test infrastructure for reusable fixtures. We use the user's `.cache` directory instead of the source tree (specifically $HOME/.cache/crdb-test-fixtures/...). For bazel, we make sure the path is available (and writable) in the sandbox and we pass the path to the test through an env var. Fixes cockroachdb#83599. Release note: None Epic: none
fa91cd1 to
9759972
Compare
RaduBerinde
left a comment
There was a problem hiding this comment.
Cool, so we should be good. TFTR!
Reviewable status:
pkg/cmd/dev/test.go line 350 at r1 (raw file):
Previously, rickystewart (Ricky Stewart) wrote…
Looks like this reusable fixture stuff is used for benchmarks rather than unit tests (which is the correct behavior as far as I can tell). Can you please add this extra argument to
dev benchinvocations and NOTdev testinvocations?
Done.
|
bors r+ |
|
Build succeeded: |
Certain storage-related tests/benchmarks generate fixtures and attempt to reuse them across invocations. This is important because fixtures can be large and slow to generate; but more importantly the generation is non-deterministic and we want to use the exact same fixture when comparing benchmark data.
Currently the tests achieve this by using a
.gitignored subdirectory inside the source tree. This does not work with bazel (which executes the test in a sandbox).This commit addresses the issue by adding new test infrastructure for reusable fixtures. We use the user's
.cachedirectory instead of the source tree (specifically $HOME/.cache/crdb-test-fixtures/...). For bazel, we make sure the path is available (and writable) in the sandbox and we pass the path to the test through an env var.Fixes #83599.
Release note: None
Epic: none