sql: datadriven telemetry tests #47133
Conversation
|
This change is |
|
❌ The GitHub CI (Cockroach) build has failed on 3683cd58. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. I am experimental - my owner is otan. |
RaduBerinde
force-pushed
the
telemetry-stuff
branch
from
3683cd5 to
0ca3976
Compare
| return errors.Errorf("expected cluster id %v got %v", expected, actual) | ||
| } | ||
| if expected, actual := ts.node.Descriptor.NodeID, r.last.Node.NodeID; expected != actual { | ||
| if expected, actual := ts.node.Descriptor.NodeID, last.Node.NodeID; expected != actual { |
There was a problem hiding this comment.
is there maybe a way to pretty-print the report in the datadriven test, and make all these assertions in the datadriven test too?
| if errors.HasAssertionFailure(err) { | ||
| td.Fatalf(t, "%+v", err) | ||
| } | ||
| return fmt.Sprintf("error: %v\n", err) |
There was a problem hiding this comment.
your tests don't have expected errors. I'd fatal the test upon any error here.
There was a problem hiding this comment.
TFTR!
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @dt, @knz, @otan, and @yuzefovich)
pkg/server/updates_test.go, line 486 at r3 (raw file):
Previously, knz (kena) wrote…
is there maybe a way to pretty-print the report in the datadriven test, and make all these assertions in the datadriven test too?
Yeah I plan to think about something for this too. Just wanted to get a review before I go too deep into the rabbit hole :)
pkg/sql/telemetry_test.go, line 101 at r3 (raw file):
Previously, knz (kena) wrote…
your tests don't have expected errors. I'd fatal the test upon any error here.
There will be cases where we expect errors - at least the crdb_internal.force_error tests from TestReportUsage; I was also thinking of adding a telemetry counter for when someone tries to use a recursive CTE that uses UNION instead of UNION ALL, which is not currently supported.
This change moves the code related to diagnostics URLs to `diagnosicspb` and adds infrastructure to override the URLs via testing knobs (rather than fiddling with global values). Release note: None
Move `mockRecorder` to `diagutil.Server` and clean up the API. Release note: None
This commit adds `sql.TestTelemetry` which uses a datadriven approach to telemetry testing. See the comment for that function for more details. `TestCBOReportUsage` is converted to a datadriven test. Similar tests should be moved over in subsequent changes. Informs cockroachdb#46730. Release note: None
RaduBerinde
force-pushed
the
telemetry-stuff
branch
from
0ca3976 to
73b40cc
Compare
|
bors r+ |
Build succeeded |
server: clean up telemetry URL code
This change moves the code related to diagnostics URLs to
diagnosicspbandadds infrastructure to override the URLs via testing knobs (rather than fiddling
with global values).
Release note: None
server: move mockRecorder
Move
mockRecordertodiagutil.Serverand clean up the API.Release note: None
sql: add datadriven telemetry tests
This commit adds
sql.TestTelemetrywhich uses a datadriven approach totelemetry testing. See the comment for that function for more details.
TestCBOReportUsageis converted to a datadriven test. Similar tests should bemoved over in subsequent changes.
Informs #46730.
Release note: None