sql: re-enable flaky show_trace_nonmetamorphic test #66001
Conversation
|
This change is |
cucaroach
changed the title
cucaroach
force-pushed
the
show_trace_58202
branch
from
f585815 to
b5da0fc
Compare
cucaroach
force-pushed
the
show_trace_58202
branch
2 times, most recently
from
b141285 to
6613f47
Compare
There was a problem hiding this comment.
Thanks Tommy! I have had this open a few times but never felt confident enough to decide whether these tests are even testing anything anymore, see the inline comments. Feels like you need someone with more skin in the game to make that call (or to guide you towards figuring out what to actually test here!)
| # This test shows some variation across runs so use statement okay instead | ||
| # of baking in flakey values. | ||
| statement ok | ||
| SELECT DISTINCT node_id, store_id, replica_id |
There was a problem hiding this comment.
Is there a point to this if we're not saying anything about the output?
| SET tracing = on,kv; SELECT * FROM system.eventlog AS OF SYSTEM TIME '-1us'; SET tracing = off | ||
|
|
||
| query TT | ||
| SELECT operation, regexp_replace(regexp_replace(message, 'job_id:[1-9]\d*', 'job_id:...', 'g'), 'drop_time:\d+', 'drop_time:...', 'g') as message |
There was a problem hiding this comment.
What's with all of the regexp_replace that then doesn't seem to apply to anything in the trace? Kinda feels like this has rotted beyond recognition & purpose.
There was a problem hiding this comment.
I'm from the "every test should carry some load or be deleted" camp. Happy to nuke these if that's the case.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @tbg)
pkg/sql/opt/exec/execbuilder/testdata/show_trace_nonmetamorphic, line 280 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Is there a point to this if we're not saying anything about the output?
The only other test we have for this is sql.TestShowTraceReplica. If this adds nothing beyond that test I'll nuke it.
pkg/sql/opt/exec/execbuilder/testdata/show_trace_nonmetamorphic, line 290 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
What's with all of the regexp_replace that then doesn't seem to apply to anything in the trace? Kinda feels like this has rotted beyond recognition & purpose.
Looks like some vestigial id redacting, should I just nuke the whole thing or just remove the regexp_replaces? Feels like we rely on SHOW KV TRACE FOR SESSION for so many other tests now that this test is gratuitous.
cucaroach
force-pushed
the
show_trace_58202
branch
from
6613f47 to
5698722
Compare
cucaroach
force-pushed
the
show_trace_58202
branch
from
5698722 to
de03a91
Compare
There was a problem hiding this comment.
nit: update the PR description to close the github issue.
Reviewed 1 of 1 files at r2.
Reviewable status:
pkg/sql/opt/exec/execbuilder/testdata/show_trace_nonmetamorphic, line 290 at r1 (raw file):
Previously, cucaroach (Tommy Reilly) wrote…
Looks like some vestigial id redacting, should I just nuke the whole thing or just remove the regexp_replaces? Feels like we rely on SHOW KV TRACE FOR SESSION for so many other tests now that this test is gratuitous.
I imagine regexp_replace part was copy-pasted from somewhere and doesn't seem necessary here.
Resolves cockroachdb#58202 Release note: none
cucaroach
force-pushed
the
show_trace_58202
branch
from
de03a91 to
2de304c
Compare
There was a problem hiding this comment.
TFTRs!
Reviewable status:
|
bors r+ |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
Cleanup some commented out tests
Resolves #58202
Release note: none