Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

persistedsqlstats: de-flake TestSQLStatsPersistedLimitReached #105917

Merged
merged 1 commit into from
Jun 30, 2023

Conversation

zachlite
Copy link
Contributor

TestSQLStatsPersistedLimitReached would previously flake because it did not consider background SQL activity happening in the cluster.

This commit re-writes the test to only make assertions that are not dependent on background cluster activity.

Resolves #105846, #97488
Epic: none
Release note: None

@zachlite zachlite requested a review from a team June 30, 2023 17:57
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@zachlite zachlite force-pushed the 230628.flush_test_flake branch from 9ba1daa to dc4da0a Compare June 30, 2023 17:59
Copy link
Contributor

@j82w j82w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @zachlite)


pkg/sql/sqlstats/persistedsqlstats/flush_test.go line 495 at r1 (raw file):

	sqlConn.Exec(t, "SELECT 1, 2, 3")

	s.SQLServer().(*sql.Server).

nit: create func or a variable to point to persisted stats to avoid the long line everywhere it is called

@zachlite zachlite force-pushed the 230628.flush_test_flake branch from dc4da0a to f93c423 Compare June 30, 2023 18:25
Copy link
Contributor Author

@zachlite zachlite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @j82w)


pkg/sql/sqlstats/persistedsqlstats/flush_test.go line 495 at r1 (raw file):

Previously, j82w (Jake) wrote…

nit: create func or a variable to point to persisted stats to avoid the long line everywhere it is called

Done.

@zachlite zachlite added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Jun 30, 2023
TestSQLStatsPersistedLimitReached would previously flake
because it did not consider background SQL activity happening
in the cluster.

This commit re-writes the test to only make assertions that are
not dependent on background cluster activity.

Resolves cockroachdb#105846, cockroachdb#97488
Epic: none
Release note: None
@zachlite zachlite force-pushed the 230628.flush_test_flake branch from f93c423 to a9a8a57 Compare June 30, 2023 18:27
@zachlite
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jun 30, 2023

Build succeeded:

@rafiss
Copy link
Collaborator

rafiss commented Jun 30, 2023

NB: the following line

Resolves #105846, #97488

will only auto-close #105846. for it to work properly, each issue needs to be listed separately, like:

Resolves #105846, Resolves #97488

https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

@zachlite
Copy link
Contributor Author

Resolves #105846, #97488

Thanks Rafi!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

persistedsqlstats: TestSQLStatsPersistedLimitReached flakes under stress and Bazel Essential CI
4 participants