logictest: improve non-metamorphic test handling#59208
logictest: improve non-metamorphic test handling#59208craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
RaduBerinde
left a comment
There was a problem hiding this comment.
Awesome, thank you!
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/sql/logictest/logic.go, line 3108 at r1 (raw file):
// If set, mutations.MaxBatchSize will be overridden to use the non-test // value. overrideMaxBatchSize bool
[nit] I would name this forceDefaultMaxBatchSize (override suggests the opposite)
pkg/sql/opt/exec/execbuilder/testdata/autocommit_nonmetamorphic, line 113 at r1 (raw file):
AND message NOT LIKE '%QueryTxn%' ---- dist sender send r35: sending batch 2 CPut to (n1,s1):1
This diff has been fixed in a PR that just merged, it should go away if you rebase.
4358ed3 to
b58956f
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/logic.go, line 3108 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] I would name this
forceDefaultMaxBatchSize(override suggests the opposite)
Done.
pkg/sql/opt/exec/execbuilder/testdata/autocommit_nonmetamorphic, line 113 at r1 (raw file):
Previously, RaduBerinde wrote…
This diff has been fixed in a PR that just merged, it should go away if you rebase.
Thanks, rebased.
I was also curious why this diff was needed in the first place - to me it seems like some of the changes in the diff are wrong (or at least non-intuitive). I bisected it to the enabling of "always on" tracing PR. I'll open up a separate issue and tag Irfan to look into that.
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/exec/execbuilder/testdata/autocommit_nonmetamorphic, line 113 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Thanks, rebased.
I was also curious why this diff was needed in the first place - to me it seems like some of the changes in the diff are wrong (or at least non-intuitive). I bisected it to the enabling of "always on" tracing PR. I'll open up a separate issue and tag Irfan to look into that.
Oh, just saw the discussion on the PR that merged the diff. Never mind then.
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
|
Please stress the |
b58956f to
8f1bb31
Compare
|
It turned out that I was afraid that |
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
This commit refactors how non-metamorphic logic tests are handled. Previously, if a logic test file had `!metamorphic` directive, then they would be skipped when the build happened to be metamorphic (which occurs in 80% of the time). However, this led to multiple flakes when PRs were merged with green CI, but they didn't update the corresponding tests (this happened because those tests were skipped). Looking over all of those issues, we see that they are due to a couple of randomizations - of `mutations.MaxBatchSize` and `row.kvBatchSize`, so this commit adds a testing knob to override both of them. Now, for every config and for every logic test file path we remember whether non-metamorphic directive was specified and possibly disable the randomizations of `mutations.MaxBatchSize` and `row.kvBatchSize`. This required a bit of plumbing, but it looks acceptable. Release note: None
8f1bb31 to
dfa8a1a
Compare
|
Needed to make a minor adjustment to avoid a nil pointer in some cases. TFTR! bors r+ |
|
Build succeeded: |
This commit refactors how non-metamorphic logic tests are handled.
Previously, if a logic test file had
!metamorphicdirective, then theywould be skipped when the build happened to be metamorphic (which occurs
in 80% of the time). However, this led to multiple flakes when PRs were
merged with green CI, but they didn't update the corresponding tests
(this happened because those tests were skipped).
Looking over all of those issues, we see that they are due to a couple
of randomizations - of
mutations.MaxBatchSizeandrow.kvBatchSize,so this commit adds a testing knob to override both of them. Now, for
every config and for every logic test file path we remember whether
non-metamorphic directive was specified and possibly disable the
randomizations of
mutations.MaxBatchSizeandrow.kvBatchSize. Thisrequired a bit of plumbing, but it looks acceptable.
Fixes: #59186.
Release note: None