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

sql: log non-internal errors for explain-redaction test #113104

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

DrewKimball
Copy link
Collaborator

This patch adds logging to TestExplainRedact and TestExplainRedactDDL in the case of non-internal errors, which were previously silently skipped. This should add visibility into the "leaked goroutine" test flakes that have been appearing in these tests.

Informs #112868

Release note: None

@DrewKimball DrewKimball requested review from michae2 and a team October 25, 2023 22:59
@blathers-crl
Copy link

blathers-crl bot commented Oct 25, 2023

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

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

:lgtm: Thank you! Just one suggestion.

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


pkg/sql/tests/explain_test_util.go line 99 at r1 (raw file):

										t.Error(err)
									} else {
										t.Logf("encountered non-internal error: %s\n", err)

The test doesn't bother to avoid doing silly things like EXPLAIN (OPT, JSON), so I suggest using a condition like } else if !strings.Contains(msg, "syntax error") { to keep syntax errors muted.

This patch adds logging to `TestExplainRedact` and `TestExplainRedactDDL`
in the case of non-internal errors, which were previously silently skipped.
This should add visibility into the "leaked goroutine" test flakes that have
been appearing in these tests.

Informs cockroachdb#112868

Release note: None
Copy link
Collaborator Author

@DrewKimball DrewKimball 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! 1 of 0 LGTMs obtained (waiting on @michae2)


pkg/sql/tests/explain_test_util.go line 99 at r1 (raw file):

Previously, michae2 (Michael Erickson) wrote…

The test doesn't bother to avoid doing silly things like EXPLAIN (OPT, JSON), so I suggest using a condition like } else if !strings.Contains(msg, "syntax error") { to keep syntax errors muted.

Done.

@DrewKimball
Copy link
Collaborator Author

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Oct 27, 2023

Build succeeded:

@craig craig bot merged commit 471f406 into cockroachdb:master Oct 27, 2023
3 checks passed
@DrewKimball DrewKimball deleted the explain-fail branch October 27, 2023 17:40
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.

3 participants