server: add tests for combined stmts api source tables #112106
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
xinhaoz
force-pushed
the
add-stmts-api-tests
branch
from
4fcc4d2 to
f854072
Compare
xinhaoz
changed the title
xinhaoz
requested review from
a team and
abarganier
and removed request for
a team
xinhaoz
force-pushed
the
add-stmts-api-tests
branch
2 times, most recently
from
2d46114 to
d005e98
Compare
maryliag
reviewed
Oct 10, 2023
There was a problem hiding this comment.
Reviewed 7 of 7 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @abarganier and @xinhaoz)
pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/sqlstatstestutil/testutils.go line 365 at r2 (raw file):
statistics, // statistics plan, // plan // TODO allow these values to be mocked. No need ffor them for now.
nit: typo ffor
xinhaoz
force-pushed
the
add-stmts-api-tests
branch
8 times, most recently
from
317a5eb to
7d6d4c5
Compare
xinhaoz
force-pushed
the
add-stmts-api-tests
branch
from
7d6d4c5 to
9c963e8
Compare
maryliag
approved these changes
Oct 30, 2023
There was a problem hiding this comment.
just one nit, otherwise
Reviewed 5 of 14 files at r3, 5 of 5 files at r4, 5 of 9 files at r5, all commit messages.
Reviewable status:
pkg/sql/sqlstats/persistedsqlstats/sqlstatstestutil/testutils.go line 1 at r4 (raw file):
// Copyright 2021 The Cockroach Authors.
nit: 2023
xinhaoz
force-pushed
the
add-stmts-api-tests
branch
2 times, most recently
from
302ee56 to
bfb6d0c
Compare
xinhaoz
force-pushed
the
add-stmts-api-tests
branch
from
bfb6d0c to
5a1ac14
Compare
This patch creates the sqlstatstestutils pkg in sqlstats/persistedsqlstats. It moves the function `GetRandomizedCollectedStatementStatisticsForTest` which was previously in sqlstatsutil. This pkg is introduced in preparation for adding more testing utilities to mock sql stats tables without creating cyclic dependencies with sqlstatsutil. Epic: none Release note: None
Add testing to verify that the combined stmts api uses the correct source tables given different table states. Epic: none Release note: None
xinhaoz
force-pushed
the
add-stmts-api-tests
branch
from
5a1ac14 to
392a28b
Compare
|
tftr! |
|
Build failed (retrying...): |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch creates the sqlstatstestutils pkg in
sqlstats/persistedsqlstats. It moves the function
GetRandomizedCollectedStatementStatisticsForTestwhich waspreviously in sqlstatsutil. This pkg is introduced in
preparation for adding more testing utilities to mock sql
stats tables without creating cyclic dependencies with
sqlstatsutil.
Epic: none
Release note: None
2: server: add tests for combined stmts api
Add testing to verify that the combined stmts api uses the
correct source tables given different table states.
Epic: none