Repository navigation
test: close test-suite coverage gaps across cmd, collector, scanner, database - #290
Merged
Merged
Conversation
internal/cmd/initialize had zero test coverage, and no test verified that re-running the schema-init DDL is a safe no-op, even though every statement in ddl.sql relies on IF NOT EXISTS for that guarantee. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
internal/cmd/server had zero test coverage. This adds a unit test for the two flag-consistency checks in RunE (no cluster needed, since they return before any connection is attempted) and an integration test verifying that the server registers its node on startup and deregisters it on shutdown. The node store methods themselves are already covered by internal/store/node_test.go; this only exercises server.go's wiring of them. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
TestParseAuthLine had a wantErr field that no case ever set, and the loop body ran a.NoError(err) unconditionally, so setting wantErr on a future case would not even have worked. Add a truncated-JSON case and short-circuit the loop when wantErr is set. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Format was cast straight from the YAML string with no allow-list, unlike Scope in the collection config. An invalid value only surfaced later inside scanner.FromConfig, which scannerServer.refresh logs and skips, so a bad Format silently dropped the scan instead of failing the upload. Mirror the Scope validation pattern to reject it at unmarshal time, and add a test in server_test.go that locks in the scanner-layer skip as a safety net for values that reach the store by some other path. The scan2 fixture in admin_test.go needed a valid Format since it now fails validation on the round trip through the "put" command. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
… FromCollection yaml.go validated Scope against an allow-list but never validated metric Kind, and never checked for duplicate Labels entries. Add both checks at unmarshal time, mirroring the existing Scope pattern. Separately, the Kind switch in FromCollection only logged on an unrecognized Kind without setting the returned error, so a bad Kind that reached the database through some path other than this YAML loader would be silently dropped instead of failing collector construction. Fix it to return an error, and add a test that could not otherwise assert correct behavior. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Refresh is the SIGHUP path for picking up rotated certificates, but nothing confirmed it actually swapped and closed the old pgxpool.Pool rather than merely reconnecting in place. The test needs white-box access to the underlying pgxpool.Pool inside Pool, which the existing database_test integration tests cannot get without an import cycle through internal/testutil, so it uses the standard export_test.go pattern to bridge the two. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
…scounted Real CockroachDB logs interleave lines that do not start with the [FEIW]yymmdd prefix, most commonly a panic stack trace. Every existing fixture happened to have that prefix on every line, so the early return in CRDBv2Parser.Parse and CRDBv2AuthParser.Parse for non-matching lines was never exercised through the full scan/parse pipeline. Add a fixture that mixes valid and invalid lines and confirm only the valid ones are counted. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Add go.uber.org/goleak (already a transitive dependency, now direct) and check for leaked goroutines after TestRefreshScanners stops its scanners and after TestLeaseLifecycle tears down both servers. These are exactly the concurrent Start/Stop paths a leak would otherwise slip through unnoticed. Two categories of goroutines needed explicit ignore rules, both started by third-party libraries with no way for this code to close them: nxadm/tail lazily starts a single process-wide file-watcher pair reused by every Tail for the life of the process, and database.Pool has no Close method since its pools are designed to live for the process lifetime too. Neither is a leak in the code these tests exist to verify. TestLeaseLifecycle registers its own check via t.Cleanup rather than a plain defer, since t.Cleanup callbacks run after a test function's own defers: a plain defer would have run before newTestServer's scheduler.Stop cleanups and flagged their still-running executor goroutines as leaks. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes several test-coverage gaps identified in a prior audit of the test suite (excluding histogram-related gaps, since histograms are being deprecated):
internal/cmd/initializeandinternal/cmd/server, which previously had none:Store.Initidempotency, the "init" command own idempotency, flag validation, and the "start" command node registration/heartbeat/deregistration wiring.TestParseAuthLine(awantErrfield no case ever set, and a bug in the loop that would have masked it anyway) and add a case for malformed JSON.Formatand collection metricKind/duplicateLabels, matching the existingScopevalidation pattern. Previously these were silently dropped later at the scanner/collector layer instead of failing the config upload.collector.FromCollection: an unsupported metricKindwas logged but not returned as an error, so the collection would report success while silently dropping the metric.database.Pool.Refresh(confirms the old pool is actually closed, not just replaced), for scanner log lines that do not match the CRDB prefix (e.g. panic traces), and goroutine-leak detection (viagoleak) around scanner and lease Start/Stop lifecycles.Co-Authored-By: roachdev-claude roachdev-claude-bot@cockroachlabs.com