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

server, ccl: adjust more tests to work with test tenant #107470

Merged
merged 4 commits into from
Jul 26, 2023

Conversation

yuzefovich
Copy link
Member

Informs: #76378.
Epic: CRDB-18499.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich yuzefovich force-pushed the tt-misc branch 2 times, most recently from af8058b to 6a79d39 Compare July 24, 2023 21:30
@yuzefovich yuzefovich marked this pull request as ready for review July 24, 2023 22:27
@yuzefovich yuzefovich requested review from a team as code owners July 24, 2023 22:27
@yuzefovich yuzefovich requested a review from a team July 24, 2023 22:27
@yuzefovich yuzefovich requested review from a team as code owners July 24, 2023 22:27
@yuzefovich yuzefovich requested a review from a team July 24, 2023 22:27
@yuzefovich yuzefovich requested a review from a team as a code owner July 24, 2023 22:27
@yuzefovich yuzefovich requested review from rhu713, rharding6373, samiskin, herkolategan, smg260 and Santamaura and removed request for a team July 24, 2023 22:27
@yuzefovich yuzefovich requested review from knz, stevendanna, a team and shermanCRL and removed request for samiskin, herkolategan, smg260, Santamaura, a team and shermanCRL July 24, 2023 22:27
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

LGTM with a request for clarification.

Reviewed 3 of 3 files at r1, 9 of 9 files at r2, 22 of 22 files at r3, 13 of 13 files at r4, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @stevendanna and @yuzefovich)


-- commits line 32 at r4:
nit: you can mention that the long-term fix will be to move the tests to at _test package.


pkg/sql/sqlstats/test_utils.go line 68 at r2 (raw file):

// Note: SQL Stats’s read path uses follower read
//
//	(AS OF SYSTEM TIME follower_read_timestamp()) to ensure that contention

nit: something is off with the comment formatting.


pkg/server/server_test.go line 778 at r2 (raw file):

			respString := string(respBytes)
			// Since server test package links ccl, build.GetInfo() says that we

I'm confused - why is this true?

This particular file (server_test.go) has package server at the top. It's not server_test. How come this logic is necessary?

Copy link
Member Author

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

TFTR!

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz and @stevendanna)


pkg/server/server_test.go line 778 at r2 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

I'm confused - why is this true?

This particular file (server_test.go) has package server at the top. It's not server_test. How come this logic is necessary?

Discussed this offline.

@craig
Copy link
Contributor

craig bot commented Jul 25, 2023

Build failed (retrying...):

@renatolabs
Copy link
Contributor

Any chance this could be making TestZoneConfigAppliesToTemporaryIndex flaky? It failed on the bors run above.

@yuzefovich
Copy link
Member Author

Yes, this PR is the culprit, thanks.

bors r-

@craig
Copy link
Contributor

craig bot commented Jul 26, 2023

Canceled.

Now that `ccl` package is linked into server tests, this commit needed
to adjust `TestServeIndexHTML` a little (even though that test is in
`server` and not `server_test`).

Additionally, this commit moves the creation of SQLStats testing knobs
into `sqlstats` package as well as removes it from one place where it's
unnecessary.

Release note: None
These calls are redundant. Also adjust the tests a bit to work with the
test tenant.

Release note: None
This commit adjusts tests in these package to run with the test tenant.

Note that the test tenant won't still run in `cli` because `ccl` isn't
linked (I briefly tried, and I think there was an import cycle - the
longer term fix is to move tests into `cli_test` package, but it's left
for later).

Release note: None
@yuzefovich
Copy link
Member Author

bors r+

@craig craig bot merged commit 68985a2 into cockroachdb:master Jul 26, 2023
@craig
Copy link
Contributor

craig bot commented Jul 26, 2023

Build succeeded:

@yuzefovich yuzefovich deleted the tt-misc branch July 26, 2023 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants