sql/schemachanger: move end to end testing to one test per-file#83545
sql/schemachanger: move end to end testing to one test per-file#83545craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
e8da848 to
bb2ccc3
Compare
| @@ -1911,12 +1911,12 @@ notified job registry to adopt jobs: [2] | |||
| test | |||
| ALTER TABLE db.public.tbl ADD COLUMN l INT NOT NULL DEFAULT nextval('db.public.sq1') | |||
There was a problem hiding this comment.
should this test be removed from this file?
| @@ -914,12 +914,12 @@ notified job registry to adopt jobs: [2] | |||
| test | |||
| ALTER TABLE db.public.tbl ADD COLUMN k INT NOT NULL DEFAULT 42 | |||
There was a problem hiding this comment.
hmm. this is the 2nd test in this file, shouldn't this break because we only allow one test now?
fqazi
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan)
pkg/sql/schemachanger/testdata/alter_table_add_column line 915 at r1 (raw file):
Previously, chengxiong-ruan (Chengxiong Ruan) wrote…
hmm. this is the 2nd test in this file, shouldn't this break because we only allow one test now?
Done.
pkg/sql/schemachanger/testdata/alter_table_add_column line 1912 at r1 (raw file):
Previously, chengxiong-ruan (Chengxiong Ruan) wrote…
should this test be removed from this file?
Ugh really stupid bug with the variable in the loop. Adjusted all the tests, should be one per file now.
postamar
left a comment
There was a problem hiding this comment.
This is close.
Reviewed 3 of 8 files at r1, 10 of 10 files at r2, all commit messages.
Reviewable status:
pkg/sql/schemachanger/sctest/end_to_end.go line 171 at r2 (raw file):
// Run statement phase. deps.IncrementPhase() deps.LogSideEffectf("# begin %s", deps.Phase())
This line and the two above should be before the for-loop, not in its body.
pkg/sql/schemachanger/sctest/end_to_end.go line 174 at r2 (raw file):
state, _, err = scrun.RunStatementPhase(ctx, s.TestingKnobs(), s, state) require.NoError(t, err, "error in %s", s.Phase()) deps.LogSideEffectf("# end %s", deps.Phase())
This line should be right after the for-loop, not in its body.
Previously, we allowed multiple tests per-file for end-to-end testing inside the declarative schema changer. This was inadequate because we plan on extending the end-to-end testing to start injecting additional read/write operations at different stages, which would make it difficult. To address this, this patch will split tests into individual files, with one test per file. Additionally, it extends support to allow multiple statements per-test statement, for transaction support testing (this is currently unused). Release note: None
fqazi
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/schemachanger/sctest/end_to_end.go line 171 at r2 (raw file):
Previously, postamar (Marius Posta) wrote…
This line and the two above should be before the for-loop, not in its body.
Done.
pkg/sql/schemachanger/sctest/end_to_end.go line 174 at r2 (raw file):
Previously, postamar (Marius Posta) wrote…
This line should be right after the for-loop, not in its body.
Done.
postamar
left a comment
There was a problem hiding this comment.
Reviewed 11 of 11 files at r3, all commit messages.
Reviewable status:
| stmtSqls := make([]string, 0, len(stmts)) | ||
| for _, stmt := range stmts { | ||
| stmtSqls = append(stmtSqls, stmt.SQL) |
There was a problem hiding this comment.
Can you help me understand this change, as it pertains to the sctestdeps.WithStatements(stmt.SQL) change below?
There was a problem hiding this comment.
This allows the test command to have multiple SQL statements. Below then we execute all the statements in a single transaction.
There was a problem hiding this comment.
In what case would we need this capability? I also just see one sql stmt under test in all the logic test file below. Did I miss something?
There was a problem hiding this comment.
For transaction testing, it allows us to have a test statement with multiple statements. Only one test is allowed per file.
|
bors r+ |
|
Build succeeded: |
Previously, we allowed multiple tests per-file for end-to-end
testing inside the declarative schema changer. This was inadequate
because we plan on extending the end-to-end testing to start injecting
additional read/write operations at different stages, which would
make it difficult. To address this, this patch will split tests into
individual files, with one test per file. Additionally, it extends
support to allow multiple statements per-test statement, for transaction
support testing (this is currently unused).
Release note: None