spanconfigkvaccessor: add datadriven tests for interface#71795
spanconfigkvaccessor: add datadriven tests for interface#71795craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
|
Thank you for updating your pull request. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
4fa3429 to
1bbd626
Compare
arulajmani
left a comment
There was a problem hiding this comment.
This is neat! I mostly have nits and minor suggestions.
Reviewed 10 of 14 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @irfansharif, and @nvanbenschoten)
pkg/roachpb/span_config.go, line 20 at r1 (raw file):
// Equal compares two span config entries. //lint:ignore U1001 unused
Let's get rid of this method entirely if it's unused.
pkg/spanconfig/spanconfigkvaccessor/datadriven_test.go, line 11 at r1 (raw file):
// licenses/APL.txt. package spanconfigkvaccessor_test
Looking ahead, considering these directives will be used to test the KVSubscriber as well, should we move this file a level above to the spanconfig_test package?
spanconfigkvaccessor and spanconfigkvsubscriber can have maintain their own testadata directories that this thing can walk.
pkg/spanconfig/spanconfigkvaccessor/datadriven_test.go, line 22 at r1 (raw file):
"github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/spanconfig/spanconfigkvaccessor" sctestutils "github.com/cockroachdb/cockroach/pkg/spanconfig/spanconfigtestutils"
nit: I personally prefer spanconfigtestutils over sctestutils fwiw
pkg/spanconfig/spanconfigkvaccessor/datadriven_test.go, line 30 at r1 (raw file):
) func TestDatadriven(t *testing.T) {
This is missing a comment describing the query language for this data-driven test.
pkg/spanconfig/spanconfigkvaccessor/datadriven_test.go, line 30 at r1 (raw file):
) func TestDatadriven(t *testing.T) {
s/TestDatadriven/TestDataDriven/g
pkg/spanconfig/spanconfigkvaccessor/testdata/basic, line 27 at r1 (raw file):
span [b,d) ---- [a,b):D
Should we add a test for kvaccessor-get with partially overlapping and enclosed spans as well?
pkg/spanconfig/spanconfigkvaccessor/testdata/full, line 55 at r1 (raw file):
[d,e):D # Verify that updating entries (including noops) show up as such
nit: fullstop
pkg/spanconfig/spanconfigkvaccessor/testdata/full, line 93 at r1 (raw file):
[d,e):x # Attempts to delete non-existent spans should error out.
Let's add one where we try to upsert a span that overlaps with another span and ensure we error out.
pkg/spanconfig/spanconfigkvaccessor/testdata/full, line 108 at r1 (raw file):
# Verify that we're able to merge span configs correctly. kvaccessor-update
It might be worth doing a kvaccessor-get before doing this update for readability -- the upsert semantics here which require us to delete [d,e) but not [c,d) are subtle.
pkg/spanconfig/spanconfigstore/store_test.go, line 22 at r1 (raw file):
"github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/spanconfig" testutils "github.com/cockroachdb/cockroach/pkg/spanconfig/spanconfigtestutils"
nit: s/testutils//
Makes it easier to reason about what's going on. We're also planning on using these datadriven testing primitives when testing the KVSubscriber (cockroachdb#69614). Preview of what's ahead: In cockroachdb#69614 we'll issue these same datadriven directives against a KVAccessor persisting state to an appropriate SQL table. Given the KVSubscriber can maintain an up-to-date view over such a table, we'll verify that it's receiving+propagating all the right span updates, and that the exported view of a spanconfig.StoreReader has the right span configs. Release note: None
1bbd626 to
283d3ba
Compare
irfansharif
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/roachpb/span_config.go, line 20 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Let's get rid of this method entirely if it's unused.
Done.
pkg/spanconfig/spanconfigkvaccessor/datadriven_test.go, line 11 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Looking ahead, considering these directives will be used to test the
KVSubscriberas well, should we move this file a level above to thespanconfig_testpackage?
spanconfigkvaccessorandspanconfigkvsubscribercan have maintain their own testadata directories that this thing can walk.
Easy enough to do it in the PR that re-uses these components. I was actually thinking to just copy some of these methods over instead of making a package out of it -- it's just parsing code and is for a new datadriven test syntax anyway.
pkg/spanconfig/spanconfigkvaccessor/datadriven_test.go, line 22 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
nit: I personally prefer spanconfigtestutils over sctestutils fwiw
Sure, done.
pkg/spanconfig/spanconfigkvaccessor/datadriven_test.go, line 30 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
This is missing a comment describing the query language for this data-driven test.
Done.
pkg/spanconfig/spanconfigkvaccessor/datadriven_test.go, line 30 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
s/TestDatadriven/TestDataDriven/g
Done.
pkg/spanconfig/spanconfigkvaccessor/testdata/basic, line 27 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Should we add a test for
kvaccessor-getwith partially overlapping and enclosed spans as well?
Done.
pkg/spanconfig/spanconfigkvaccessor/testdata/full, line 93 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Let's add one where we try to upsert a span that overlaps with another span and ensure we error out.
Done.
pkg/spanconfig/spanconfigkvaccessor/testdata/full, line 108 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
It might be worth doing a
kvaccessor-getbefore doing this update for readability -- the upsert semantics here which require us to delete[d,e)but not[c,d)are subtle.
Done.
arulajmani
left a comment
There was a problem hiding this comment.
Reviewed 4 of 6 files at r2.
Reviewable status:
pkg/cmd/dev/datadriven_test.go, line 40 at r2 (raw file):
// runnable through: // // go test -run TestDataDriven/<fname>
revert these?
pkg/spanconfig/spanconfigkvaccessor/datadriven_test.go, line 11 at r1 (raw file):
Previously, irfansharif (irfan sharif) wrote…
Easy enough to do it in the PR that re-uses these components. I was actually thinking to just copy some of these methods over instead of making a package out of it -- it's just parsing code and is for a new datadriven test syntax anyway.
No objections to handling this in a followup PR, though I think we can structure it in a way that avoids copying over some of these directives.
pkg/spanconfig/spanconfigkvaccessor/testdata/full, line 93 at r1 (raw file):
Previously, irfansharif (irfan sharif) wrote…
Done.
Is this done?
|
AFK, but yeah added a new test file with the cases you suggested. bors r+ |
|
Build succeeded: |
Makes it easier to reason about what's going on. We're also planning on
using these datadriven testing primitives when testing the KVSubscriber
(#69614).
Preview of what's ahead: In #69614 we'll issue these same datadriven
directives against a KVAccessor persisting state to an appropriate SQL
table. Given the KVSubscriber can maintain an up-to-date view over such
a table, we'll verify that it's receiving+propagating all the right span
updates, and that the exported view of a spanconfig.StoreReader has the
right span configs.
Release note: None