Repository navigation
test: raise coverage of files below 90% with table-driven tests - #4274
aryanmehrotra merged 16 commits into
Conversation
Table-driven tests for the root package: App setup and registration, gRPC server wiring, websocket services, subscriber handling, CRUD handlers, external datasource wiring, context helpers, run/shutdown, exporter and telemetry.
Covers LLM/tool lookup, Kafka config validation paths (no network), connection-from-context rows, the SQL mock container helpers and the metrics cardinality fallback.
Adds trie-router edge cases, form-data binding for unusual kinds, the response capture writer (new test file, none existed), HTTP error types, and basic/API-key auth middleware paths.
Token-bucket edge cases, the Redis-backed store via redismock, the denied path for every HTTP method, and auth option error paths.
…al, zip Logger level/format paths, metrics registration, exporter/provider build paths, LLM wire decoding, gRPC stream auth interceptors, terminal progress, zip extraction errors and testutil.
SQL dialect/TLS/connection paths with sqlmock, Redis health and PubSub stream paths with miniredis/redismock, and CommonFileSystem / CommonFile / row reader behavior.
…e flake Google Pub/Sub via the in-process pstest server, Kafka conn/helper/health with gomock and loopback listeners, the MQTT default client, and the shared pubsub log/message helpers. Also fixes TestMQTT_SubscribeSuccess, which could hang until the 10m test timeout: its goroutine read the subscription before Subscribe registered it and sent on a nil channel. It now waits for the registration.
SQL, Redis, OpenTSDB, ArangoDB, Dgraph, SurrealDB, Cassandra, ScyllaDB and Elasticsearch migrators, including lock-retry cancellation (no sleeps) and deterministic filesystem error paths.
In-process fakes only: httptest for Azure/GCS, the existing goftp server helper, an in-memory SFTP server over net.Pipe, and gomock for S3. New fs_dir_test.go (s3) and file_test.go (sftp) where none existed.
NATS client/connection/stream/subscription managers and the JetStream wrapper, SQS connect/config loading against an httptest endpoint with an isolated AWS env, and the Event Hub paths reachable without a live namespace.
…acle, dbresolver, dgraph, arangodb gocql wrapper error paths via a closed session, dbresolver factory and routing via SQLite, Cloud SQL dialer paths with fake credentials, and httptest/in-process gRPC fakes for Dgraph and ArangoDB.
f2c5f0b to
b9428c9
Compare
…r, elasticsearch, kv-stores, gcp httptest fakes for SurrealDB (CBOR), InfluxDB, OpenTSDB, Solr and Elasticsearch; Couchbase paths that need no live cluster; NATS/Badger/ DynamoDB kv-store paths; and the GCP exporter caching detector.
b9428c9 to
87628ca
Compare
aryanmehrotra
left a comment
aryanmehrotra
left a comment
There was a problem hiding this comment.
I reviewed this in three passes so the whole diff actually got read rather than skimmed: datasource/pubsub + datasource/file (36 files), the other 17 datasource submodules (36 files), and everything outside datasource/ (47 files). Roughly 17,400 lines read, 43 mutations planted in production code, and every package run at -count=1, -race and -count=5 against both this branch and development.
The headline first, because it is the question worth asking of a coverage PR: these tests are functional, not written to move the number.
| scope | functional share of added lines | mutations caught |
|---|---|---|
| pubsub + file | ~98.1% | 17 / 18 |
| the other 17 datasources | 92.8% | 12 / 14 |
outside datasource/ |
93.8% | 10 / 11 conclusive |
I went in expecting mock round-trips — a fake returning what it was told to return — and that is not what this is. 11 of 17 datasource packages get a real in-process fake: an httptest CBOR RPC server driving the actual SurrealDB client, a real gRPC api.DgraphServer, a hand-rolled NATS TCP handshake, a real embedded BadgerDB, miniredis, sqlmock plus a real SQLite file, pstest.NewServer for GCP Pub/Sub, sftp.NewRequestServer over net.Pipe() with no sockets at all. Zero t.Skip, zero os.Getenv, zero reachable-network dependencies across all 119 files — I ran the whole thing with no Docker and nothing hung or skipped.
Some of it is genuinely excellent:
- surrealdb 22.2% → 92.3% with a real protocol fake. Best test engineering in the PR.
Test_APIKeyAuthMiddlewaregets deleted and rewritten. It was literallyt.Logf("Test_APIKeyAuthMiddleware"). Replacing a vacuous test with a real 3-row table is the opposite of coverage farming and I want to say so explicitly.dgraph/interfaces_test.goasserts the requestdgobuilds (RespFormat: api.Request_RDFvs_JSON,ReadOnly,BestEffort). That is exactly the right assertion for a thin wrapper, and it is why my QueryRDF→Query mutation died.context_test.go:734writes a sentinel after the call under test and asserts the first message received, using in-order delivery to prove nothing was written. Deterministic, no sleep.mqtt_test.go:238-256fixes a pre-existing-racefailure. Ondevelopmentpubsub/mqtt -racefails two tests; here it fails one.assert.Equal(t, tt.expStderr == "", stderr == "")in the nats tests turns "expected no output" from an always-trueContains(x, "")into a real assertion that also catches spurious logging. I'd steal that.- The PR body lists six production bugs found and declines to lock any in. Two I independently confirmed.
So this is good work. I'm requesting changes because a handful of specific items need fixing before it lands, and at this size they will not survive being left as optional comments.
1. Blocking: run_test.go:245 will os.Exit(1) the whole pkg/gofr test binary
TestApp_Run_StartupHookFailure, subtest "hook error aborts startup", calls app.Run() with an OnStart hook returning an error and expects Run to return — the comment says "Run must return on its own".
PR #3942 changes exactly that path: handleStartupHooks returns startupFailed → finishAbandonedStartup → abortStartup() → os.Exit(exitCodeStartupFailed), because app := New() never sets the a.exit seam.
I merged the two locally and ran it: the test binary exits silently with no --- FAIL marker, killing every test after it in the package. Only the context.Canceled subtest survives, because that path returns startupCanceled and doesn't exit.
Neither PR's CI can see this — each is green alone. Whichever lands second turns pkg/gofr red and truncates the suite.
Smallest fix on this side is app.exit = func(int) {} in that subtest. Happy to carry it on #3942 instead if you'd rather — just tell me which, so we don't both do it or neither.
2. Three tests pin defects as correct behavior
surrealdb/surrealdb_test.go:1196-1204 — this is the expensive one. Connect() calls authenticateCredentials, and on errInvalidCredentialsConfig (username set, password empty — returned un-logged at surrealdb.go:192-194) it does a bare return. c.db was already assigned at surrealdb.go:151, so the client reports itself connected with unauthenticated credentials and not one log line is emitted. The new case asserts expConnDone: true with only a Debugf expectation — certifying that as correct.
I added c.logError("failed to authenticate with SurrealDB", err); c.db = nil to that branch and got --- FAIL: Test_Connect/incomplete_credentials_abort_connection_setup. So the next person to fix this has to argue with a test. Could you drop or invert that case, and ideally file the Connect() bug separately?
opentsdb/opentsdb_test.go:725-731 — opentsdb.go:204 returns errInvalidResponseType ("invalid response type") when the datapoints argument has the wrong type. Wrong sentinel, pre-existing. The case {"invalid datapoints type", datas: "not-datapoints", expErr: errInvalidResponseType} freezes it — changing :204 to errInvalidParam fails the test.
container/mockcontainer_test.go:337 — cements three defects in sql_mock.go: :79 assigns the canned response before validating query and args (the test's own comment says so, then asserts it); :83 passes (query, expectedText) to "expected query: %q, actual query: %q" — arguments swapped; :94 does the same on args, with %d for an any. The test uses gomock.Any() for both format arguments, so it pins the wording while being structurally unable to catch the swap.
3. influxdb_test.go:885-1038 — 154 lines that cannot catch a wrapper bug
fakeBucketsAPI returns f.bucket from six different methods, so the wrapper can call the wrong upstream and nothing notices. Two mutations survived:
internal.go:60FindBucketByID→b.api.FindBucketByName(...)→ test passesinternal.go:28CreateOrganizationWithName→a.api.FindOrganizationByName(...)→ test passes
That's 31% of the package's added lines, and influxdb is the biggest coverage jump in its scope (+19.2pp) — so this is the one place where the number moved without assertions behind it. dgraph/interfaces_test.go already has the right pattern: per-method recorders (lastQuery/lastAlter/lastLogin). Same shape here would make these 154 lines real.
4. Two new lint regressions
golangci-lint 2.12.2 over both trees: development 23 issues, this branch 27.
pubsub/google/google_test.go:588and:614—importShadow: shadow of imported package 'status'(gocritic). The newimport "google.golang.org/grpc/status"atgoogle_test.go:22is shadowed by two pre-existingstatus, details := g.getWriterDetails()lines. Renaming the import alias fixes both.pubsub/kafka/health.go— twogoconstfindings (isController,status), collateral from the new test pushing package-wide counts over the threshold.
Separately, in the datasource submodules there are 16 new goconst findings, all on production lines this PR never touches (surrealdb :253,464,500,541,578,615, sql :477,481,551, dgraph :302, dbresolver :517, couchbase :496, and others), for the same reason — goconst counts literals in _test.go. CI won't fail on any of this (only-new-issues: true), but the next PR that touches those lines inherits them. Worth considering ignore-tests for goconst in .golangci.yml as a follow-up.
5. Tests to drop or tighten
file:line |
Problem |
|---|---|
gcs/fs_test.go:256 TestStartRetryConnect_ExitsImmediately |
Doesn't check immediacy. I deleted the IsRetryDisabled() early return at gcs/fs.go:89 and it still passed — just took 61.7s instead of 13.7s, rescued by the duplicate check at :97. Needs a deadline or a completion channel. |
eventhub/message_test.go:13 TestMessage_Commit |
Both table rows execute the identical line (message.go:25-27's else branch never reads a.event), and the only assertion is an exact Debugf string. |
metrics/exporters/exporter_test.go (35 ln) |
Mutation Config{AppName, AppVersion} → Config{} survived the whole package suite. Prometheus() is a 3-line deprecated shim already covered by provider_test.go. |
testutil/error_test.go (17 ln) |
Both this and the above target files that were already at 100% under CI's measurement — see §6. |
kv-store/nats/nats_test.go:705 |
Tests MockKeyValueEntry, a test double that ships in the production file interface.go:38-44. Seven stubs returning literals; the test asserts those literals. |
opentsdb/observability_test.go:65 |
Nine table cases differing only by a span name that is never asserted. Both span-name mutations survived. One case would prove the nil-tracer branch. |
cassandra/internal_test.go:84, scylladb/internal_test.go:93 |
Assert only zero values; gutting the three iterator methods to constants leaves both green. Between them that's half of cassandra's added lines. |
nats/client_helper_test.go "consumer info unavailable skips deletion" |
Its only written assertion is assert.Contains(t, stdout, "") — always true. It survives on the strict gomock having no DeleteConsumer expectation. |
6. The PR body's coverage baseline isn't CI's
The per-file table uses per-package coverage; CI uses whole-module -coverpkg (.github/workflows/go.yml:60,76). Reproducing CI's method, several "before" numbers are much higher, and two files the body says it rescued were already at 100%:
| file | body | CI's method |
|---|---|---|
metrics/exporters/exporter.go |
0.0% → 100% | 100% → 100% |
testutil/error.go |
0.0% → 100% | 100% → 100% |
container/container.go |
67.6% → 95.0% | 79.3% → 95.0% |
grpc/middleware/common.go |
50.0% → 100% | 58.3% → 100% |
Not dishonest, but "84.9% → 95.4%, 121 files below 90% → 16" is computed on a narrower basis than the gate, and those two 100%→100% files are exactly where the padding sits. A footnote would fix it. (Under CI's method the whole main module goes 90.72% → 95.68%, which is still a real +4.97pp.)
Also unlisted and still 0%: pkg/gofr/responder.go and pkg/gofr/traces/exporters/config.go. Trivial, but the table claims completeness.
One measurement note in your favour: arangodb's +2.3pp is an artifact, not weak work. Its coverage profile carries 592 functions from mock_*.go, 517 of them at 0.0% — ~6,000 lines of generated gomock in non-test files dominating the denominator. All 8 arangodb tests are functional and one is mutation-verified. That package will never report a sane number until the generated mocks move behind a build tag or into _test.go.
7. Please drop the AI-attribution footer
The body carries a 🤖 Generated with [Claude Code] line. This repo's convention is that merged history attributes only the human author.
8. On the size
119 files and 16,168 lines is not reviewable in one pass, and the #3942 landmine is the proof: it's invisible to CI, invisible to a diff skim, and only surfaces if someone merges the branch and runs the suite. At this size nobody does that — it took three parallel passes here to find it.
What a reviewer can verify cheaply: zero production files changed (I confirmed — git diff --name-only | grep -v _test.go$ is empty, 119 test files, 0 other), gofmt clean, CI green, coverage delta. What they must take on trust is whether ~390 test functions assert anything.
If you're open to splitting, the seam I'd cut along is the Go module boundary, which is already how CI shards it and roughly how your 12 commits group:
- main module — 47 files / 4,324 lines, and where all the collision surfaces live
datasource/pubsub+datasource/file— 36 files / 5,099 lines- the remaining 17 datasource submodules — 36 files / 6,745 lines
To be clear: split for reviewability, don't prune for quality. Cutting to "only the functional tests" would discard about 2% of the lines and isn't worth it.
Things this PR found that aren't its job to fix
Worth separate issues:
CommonFileSystem.{connected,disableRetry}(file/common_fs.go:630-648) is an unsynchronized data race against the background retry goroutine — it's whyfile/ftpandfile/gcsfail under-raceondevelopmenttoday.TestCommonFileSystem_RetryAndConnectedStateexercises those accessors single-threaded, which a reviewer could read as "covered".nats/stream_manager.go:56matchesstrings.Contains(err.Error(), "stream name already in use")rather thanerrors.Is(err, jetstream.ErrStreamNameAlreadyInUse). Verified against the pin —nats.go@v1.53.1/jetstream/errors.go:107defines that exact description, so it works today, but a minor bump that rewords it silently turns a tolerated condition into a hard failure. The new test atstream_manager_test.go:268cements the fragile match.initTest/setupDBcallctrl.Finish()inside themselves incassandra,scylladbanddgraph, so it runs before any expectation is registered.go.uber.org/mock@v0.6.0/gomock/controller.go:268-273then short-circuits thet.Cleanupfinish, and "missing call(s)" is never reported in those packages — every.Times(1)guards only against a wrong call, never against no call.dgraph.go:300-301logsError("dgraph health check failed: ", err)witherr == nilon the empty-response path, so the line reads... : <nil>.TestClient_HealthChecknow pins that.couchbase/wrappers_test.go:66-68asserts a gocb v2.12.4 error string; verified there's no sentinel behind it (cluster_query.go:249is an inlineerrors.New), so the string is the only handle — but it'll break on a gocb bump.
Verified across the whole PR: gofmt -l clean on all 119 files, golangci-lint --new-from-rev clean in the main module, 40/40 CI checks SUCCESS, and every -race/-count=5 failure reproduces identically on development (the websocket.go:47 race, the port-2121 isPortAvailable probe, the 11 redis TestPubSub_* races, cmd/terminal TestSpinner, the remotelogger flake). Nothing introduced.
- run_test: drop the "hook error aborts startup" row. gofr-dev#3942 changes that path to exit the process, so the test would take the binary down once both land; gofr-dev#3942 owns that behavior. The canceled-hook case stays. - container: TestExpectSelect_MismatchCases pinned sql_mock.go's response-before-validation order and its swapped/%d format arguments. Keep only the argument-count mismatch, asserted with exact arguments. - Remove tests that only padded coverage: metrics/exporters exporter_test.go and the TestCustomError_Error addition to testutil/error_test.go (both target files were already fully covered by other tests). - pubsub/google: alias the grpc status import as grpcstatus so the local status variables no longer shadow it (gocritic importShadow). - pubsub/kafka: share one expected-broker helper between the two health tests so the repeated literals no longer trip goconst on health.go.
- gcs: TestStartRetryConnect_ExitsImmediately now fails if startRetryConnect has not returned within 2s. Without its early return it previously still passed, just 60s slower. - eventhub: TestMessage_Commit had two rows running the same line; keep one. - nats: replace three always-true Contains(stdout, "") checks with an assertion that output appears only when expected.
- Stop pinning defects: drop surrealdb's incomplete-credentials Connect case (it certified a silent unauthenticated "connected" state), opentsdb's invalid-datapoints row (wrong sentinel), and dgraph's exact "<nil>" health-check log text. - influxdb: the wrapper fakes returned the same value from every method, so calling the wrong upstream passed. Per-method recorders now assert the exact upstream method, its arguments, and the passed-through value/error. - Remove weak tests: the kv-store/nats MockKeyValueEntry test (a test double asserting its own literals), the cassandra/scylladb iterator tests (a closed session's Iter is always zero-valued, so they could not tell a real wrapper from constants), and opentsdb's nine span-name-only cases (collapsed to the nil-tracer case).
|
Thanks for going through all of it, and for the mutation runs. That's much more than I expected on a PR this size. Pushed three commits on top: bf2b864 (main module), f6def5f (pubsub + file) and d514258 (the other datasources). They're grouped along the same module boundaries you suggested. 1. 2. Tests pinning defects. Removed all three, plus the dgraph one from your list:
I re-ran your examples: adding the error log plus 3. influxdb. Replaced the fakes with per-method recorders. Each method records its name and args and returns a value (and error) unique to that method. Every row asserts the exact upstream call, its args, and the pass-through. Both of your mutations ( 4. Lint.
5. Weak tests. Everything from the table is done:
6. Coverage basis. Thanks for re-measuring. Under CI's method (mocks excluded) I get 90.67% → 95.65%, essentially your figure. The description now leads with that. It says the per-file table uses the narrower per-package basis, gives your container/common.go examples, drops the two 100%→100% rows, and lists 7. Footer. Removed. 8. Size. I'd like to keep it as one PR if that's OK. It's test-only, and the commits are grouped by area (12 plus these 3 review commits), so it can be read commit by commit. If you'd still prefer a split once you've looked at this round, I'll do it along the module boundaries you listed. Local check before pushing: CI-style runs pass for the main module and all touched submodules, For the things you found that aren't this PR's job, I'll open separate issues unless you'd rather track them differently:
|
aryanmehrotra
left a comment
aryanmehrotra
left a comment
There was a problem hiding this comment.
All eight addressed. I checked the delta rather than taking the summary on trust, and the shape of it is the thing I most want to call out: 17 files, every one a _test.go, net −78 lines (332 added, 410 removed). You deleted padding and rewrote the weak fake instead of adding more tests on top. That is the right direction and it is rarer than it should be.
Item by item:
- The
os.Exitlandmine is gone. The"hook error aborts startup"subtest is removed; onlyTestApp_Run_StartupHookCanceledremains, which exercises the path that returns rather than exits. That was the one blocking item — it would have taken the wholepkg/gofrtest binary down with no--- FAILmarker once #3942 lands, killing every test after it. Nothing left for either of us to carry. - surrealdb's
incomplete credentialscase is gone, soConnect()'s silent-failure branch is no longer certified as correct behavior. Please do file that one separately — a client that reports itself connected with unauthenticated credentials and emits zero log lines is worth its own fix. - opentsdb's wrong-sentinel case is gone.
- The influxdb wrappers are properly rebuilt.
calls []upstreamCallwith arecord(method, args...)helper — per-method recording, which is exactly thedgraph/interfaces_test.gopattern I pointed at. Those 154 lines can now actually catch a wrapper calling the wrong upstream, which was the whole problem. importShadowfixed via thegrpcstatusalias.exporter_test.godeleted.- The coverage-baseline footnote is in, so the 84.9% figure can't be mistaken for CI's
-coverpkgnumber. - Attribution footer gone.
One correction from me: I asked you to drop testutil/error_test.go as padding. That was wrong — this PR doesn't touch that file at all; it pre-exists on development. My reviewer mis-attributed it and I passed it on without checking the diff. Sorry for the noise.
I also confirmed nothing else came along for the ride: no go.mod, go.sum, go.work, .github/ or .golangci.yml changes, and zero non-test files across the whole PR.
For the record on what the review established before the fixes, because it should be visible on the PR: across three separate passes covering all 119 files — roughly 17,400 lines read and 43 mutations planted in production code — ~93% of the added lines were already functional, and 39 of 43 mutations were caught. The concern with a large coverage PR is tests written to move a number, and that is not what this is. The fakes are real protocol fakes: an httptest CBOR RPC server driving the actual SurrealDB client (22.2% → 92.3%), a real gRPC api.DgraphServer, a hand-rolled NATS TCP handshake, an embedded BadgerDB, miniredis, pstest.NewServer, sftp.NewRequestServer over net.Pipe() with no sockets at all. Zero t.Skip, zero os.Getenv, zero network dependencies — the whole thing runs with no Docker.
And Test_APIKeyAuthMiddleware, which was literally t.Logf("Test_APIKeyAuthMiddleware"), is now a real table asserting status and the context value. Deleting a vacuous test is the opposite of farming coverage.
Two things I'd still encourage, neither blocking:
Splitting. 119 files is more than one review pass can honestly cover — the landmine above was invisible to CI, invisible to a diff skim, and only surfaced because one pass merged the branch and ran the suite. If you're open to it, the seam is the Go module boundary, which is already how CI shards this: main module (47 files), pubsub + file (36), the remaining 17 datasource submodules (36). Split for reviewability, not to prune — the functional ratio doesn't justify cutting anything.
Follow-up issues worth filing from what this turned up, none of them yours to fix here:
file/common_fs.go:630-648—connectedanddisableRetryare unsynchronized against the background retry goroutine. This is a real production data race and it's whyfile/ftpandfile/gcsfail under-raceondevelopmenttoday.initTest/setupDBcallctrl.Finish()inside themselves in cassandra, scylladb and dgraph, so it runs before any expectation is registered and gomock never reports a missing call. Every.Times(1)in those packages guards only against a wrong call, never against no call.nats/stream_manager.go:56matches an error string rather thanerrors.Is(err, jetstream.ErrStreamNameAlreadyInUse). It works against the pinnednats.go@v1.53.1, but a bump that rewords the description turns a tolerated condition into a hard failure.arangodb's coverage will keep reading artificially low until its ~6,000 lines of generated gomock move behind a build tag or into_test.go— 517 of its 592 profiled functions are mocks at 0%.
Thanks for turning this around quickly and for taking the substantive asks rather than arguing the easy ones.
|
Following up after actually running this at What I ran: 27 Go modules × 5 modes (build, vet, Result: zero failures attributable to this PR. Every failure reproduces identically on
24 of 26 submodules are a clean sweep on all five modes. Every Coverage: not a single package went down. 45 packages measured, all up — surrealdb +69.9, cloudsql +44.1, solr +32.5, eventhub +26.1, The influxdb rewrite works. Both mutations that survived the old fakes are now killed, in both the
Nothing was orphaned by the deletions — Two items, both mine to carry: 1. There is a second collision with #3942, and it is my PR's fault, not yoursYour new The merge itself is clean — no conflicts, and the suite completes with a normal Cause: on I'm taking it. Your test asserts today's behaviour correctly; my PR is the one changing the behaviour, so my PR carries the test update. Nothing for you to change, and please don't pre-adapt to an unmerged branch of mine. 2. Deleting that subtest left
|
|
Thanks for merging, and for the full run across all the modules. No worries about Filed the follow-ups from this review:
|
Resolve the run_test.go conflict with gofr-dev#4274: keep development's new tests and add this PR's runCMD exit-code and cleanup-order tests and the onClose hook on closeRecordingLogger.
Resolve conflicts with gofr-dev#4274 in the elasticsearch and opentsdb tests by keeping both sides' tests, and move development's new opentsdb health-check test from the removed dialTimeout seam to dialContext.
…tests compiling Rebased onto development, which brought in two untagged tests that do not hold under a tag, plus akshat-kumar-singhal's review of the CI step. **The "Default build links the same packages as before" step is removed.** It diffed `go list -deps ./...` against the merge base across the whole root module, and this job runs on every pull request -- so it meant "no PR may add a package or a dependency, ever". That is not a rule this repo has, and it would have gone red on changes with nothing to do with build tags. What it was protecting is already covered, and better, by the step above it: `grep -Eq "$pattern" /tmp/deps.default` fails when a tagged-out package is missing from the DEFAULT build, so the per-tag checks cannot pass vacuously and an untagged build cannot quietly stop linking them. A comment says so, and asks that the broad diff not be re-added. **Two tests from development needed a home under the tags.** TestApp_setupGraphQL_MissingSchema (#4274) names errSchemaMissing, which graphql.go compiles out; it moves from the untagged gofr_test.go into graphql_test.go, which already carries !gofr_nographql. TestContainer_createKafkaPubSub_InvalidConfigs asserts what kafka.New's own validation logs, and under gofr_nopubsub there is no kafka.New -- it takes the pubsubBackendsLinked skip the file already uses for three other tests. Both failed `go vet`/`go test` under their tag while the non-test build stayed green, which is the tagged-build blind spot this PR's CI job exists for: it caught them. **The doc now states where its counts were measured** -- darwin/arm64, Go 1.26.3 -- and that they are platform-dependent, so a linux/amd64 reader seeing roughly twenty fewer knows why the absolute numbers differ and the deltas do not. Verified across all five tag configurations: build, vet and test clean. typos clean, gofmt clean, go.yml parses. golangci-lint reports 5 issues against 7 on development, none new.
2 participants
Description:
This PR adds tests only. It raises statement coverage for every file that was below 90%, across the main module and the
pkg/gofr/datasource/*/metrics/exporters/gcpsubmodules.-coverpkg=./pkg/gofr/..., generatedmock_*.goexcluded): 90.67% → 95.65% (+4.98pp).container/container.gois 79.3% under CI's method, not 67.6%;grpc/middleware/common.gois 58.3%, not 50.0%).go.modorgo.sumare touched.opentsdb/observability.go74.3%,cassandra/internal.go81.2%,scylladb/internal.go92.0%,kv-store/nats/interface.go14.3% (the uncovered part is a test double shipped in that file).Changes after review
Connect, opentsdb wrong-sentinel row, sql_mock order/argument checks, dgraph<nil>health log text.metrics/exporters/exporter_test.goand the added case intestutil/error_test.go. Both target files were already at 100% under CI's measurement. Also removed the kv-store/nats mock-entry test and the cassandra/scylladb iterator tests, and collapsed the opentsdb span-name cases.Contains(stdout, "")checks in nats.importShadowand the kafkagoconstregressions. The reviewer's golangci-lint 2.12.2 shows the same issue set asdevelopmentfor those packages.How the tests are written
expErr,expStatus, per-rowsetupMocks/mockCall), and test bodies don't branch on what to assert.foo.gogo into the existingfoo_test.go. A new_test.gofile is created only where that source file had none (28 new files, for examplehttp/capture_test.go,kafka/conn_test.go,sftp/file_test.go).httptest, pstest, in-memory SFTP overnet.Pipe, and loopback listeners on127.0.0.1:0. There are no sleeps for timing and no real external services. Environment changes go throught.Setenvand working-directory changes throught.Chdir. Tests uset.Context()and package-level sentinel errors.One test fix included
TestMQTT_SubscribeSuccess(existing test) could hang until Go's 10-minute test timeout, and it also does so ondevelopment.subscriptions["test/topic"]beforeSubscribehad registered it, then sent on the zero value's nil channel.-race.Verification (local, run the way CI runs it)
APP_ENV=test go test -covermode=atomic -coverpkg=./pkg/gofr/... ./pkg/gofr/...passes in 58 s. Every submodule'sgo test ./...passes, and none takes more than 60 s, well inside the 5-minute retry budget.golangci-lint run --new-from-rev=origin/developmentreports 0 issues for the main module and every touched submodule.gofmtis clean.Still below 90%, with the reason
graphql.go,datasource/file/local_fs.gopubsub/eventhub/{eventhub,helper,message}.gocouchbase/wrappers.gofile/{azure,ftp,gcs}/fs.goredis/redis.go,kafka/helper.go,mqtt/default_client.gosftp/file.godbresolver/circuit_breaker.gogrpc.go,metrics/exporters/telemetry.goFatalf/exit paths, and telemetry that posts to the real gofr.dev endpointNot covered on purpose:
pkg/gofr/responder.goandpkg/gofr/traces/exporters/config.gocontain only no-op declarations with no executable statements, so there is nothing meaningful to assert.Possible production bugs found (not fixed here, since this PR is tests only; happy to open issues)
datasource/oracle/oracle.go:88builds the DSN with hard-coded local paths (libDir="/Users/zopdev/oracle-client/lib",wallet_location="/Users/zopdev/wallet").dbresolver/circuit_breaker.goallowRequest:CompareAndSwap(&openState, &halfOpenState)compares against a freshly allocated pointer, so an open breaker never moves to half-open after its timeout.container.goKafka config: an invalidKAFKA_BATCH_SIZE,KAFKA_BATCH_BYTESorKAFKA_BATCH_TIMEOUT, orPUBSUB_OFFSET, logs "using default …" but passes the failedAtoiresult (0) on.sftpcreateJSONObjectReaderreopens the remote file name withos.Openon the local filesystem. Separately,peekJSONTokencopies the decoder, so the JSON array reader has already consumed the stream.websocket.go: a response that can't be serialized isn't skipped, so an empty frame is written to the client. There is also a data race atwebsocket.go:47, which shows up in three existing tests under-race.-racefailures ondevelopment, which this PR doesn't touch: redis PubSub tests (redismock concurrency withmonitorConnection),cmd/terminalspinner, natsTestClient_SubscribeWithHandler(flaky), and the gcs/ftp retry-state tests.Breaking Changes (if applicable):
None. The PR adds tests only.
Additional Information:
No new dependencies. Every test helper used (redismock, miniredis, pstest, surrealcbor,
pkg/sftpin-memory server, and so on) is already in the respectivego.mod.Per-file coverage for every file that was below 90% (click to expand)
ai/llm/wire.gocmd/terminal/progress.gocontainer/container.gocontainer/metrics_exporter.gocontext.gocrud_handlers.godatasource/arangodb/arango.godatasource/arangodb/arango_document.godatasource/arangodb/arango_graph.godatasource/arangodb/logger.godatasource/cassandra/internal.godatasource/clickhouse/clickhouse.godatasource/cloudsql/cloudsql.godatasource/couchbase/couchbase.godatasource/couchbase/logger.godatasource/couchbase/wrappers.godatasource/dbresolver/circuit_breaker.godatasource/dbresolver/factory.godatasource/dbresolver/resolver.godatasource/dgraph/interfaces.godatasource/dgraph/metrics.godatasource/elasticsearch/elasticsearch.godatasource/file/azure/fs.godatasource/file/azure/storage_adapter.godatasource/file/common_file.godatasource/file/common_fs.godatasource/file/ftp/fs.godatasource/file/ftp/storage_adapter.godatasource/file/gcs/fs.godatasource/file/gcs/storage_adapter.godatasource/file/interface.godatasource/file/local_fs.godatasource/file/logger.godatasource/file/row_reader.godatasource/file/s3/fs.godatasource/file/s3/fs_dir.godatasource/file/sftp/file.godatasource/file/sftp/fs.godatasource/influxdb/influxdb.godatasource/influxdb/internal.godatasource/influxdb/logger.godatasource/influxdb/metrics_logger.godatasource/kv-store/badger/badger.godatasource/kv-store/dynamodb/logger.godatasource/kv-store/nats/interface.godatasource/kv-store/nats/logger.godatasource/kv-store/nats/nats.godatasource/opentsdb/observability.godatasource/opentsdb/opentsdb.godatasource/opentsdb/preprocess.godatasource/opentsdb/response.godatasource/oracle/oracle.godatasource/pubsub/eventhub/eventhub.godatasource/pubsub/eventhub/helper.godatasource/pubsub/eventhub/message.godatasource/pubsub/google/google.godatasource/pubsub/kafka/conn.godatasource/pubsub/kafka/health.godatasource/pubsub/kafka/helper.godatasource/pubsub/log.godatasource/pubsub/mqtt/default_client.godatasource/pubsub/nats/client.godatasource/pubsub/nats/client_helper.godatasource/pubsub/nats/config.godatasource/pubsub/nats/connection_manager.godatasource/pubsub/nats/pubsub_wrapper.godatasource/pubsub/nats/stream_manager.godatasource/pubsub/nats/subscription_manager.godatasource/pubsub/sqs/sqs.godatasource/redis/health.godatasource/redis/pubsub.godatasource/redis/redis.godatasource/scylladb/internal.godatasource/solr/logger.godatasource/solr/solr.godatasource/sql/sql.godatasource/surrealdb/surrealdb.godatasource/surrealdb/utils.godatasource/surrealdb/wrapper.goexporter.goexternal_db.gofile/zip.gogofr.gographql.gogrpc.gogrpc/middleware/common.gogrpc/middleware/oauth.gohttp/capture.gohttp/errors.gohttp/form_data_binder.gohttp/middleware/apikey_auth.gohttp/middleware/basic_auth.gohttp/middleware/errors.gohttp/trie_router.gohttp_server.gologging/logger.gometrics/exporters/gcp/gcp.gometrics/exporters/provider.gometrics/exporters/telemetry.gometrics/register.gomigration/arango.gomigration/cassandra.gomigration/datasource.gomigration/dgraph.gomigration/elasticsearch.gomigration/opentsdb.gomigration/redis.gomigration/scylla_db.gomigration/sql.gomigration/surreal_db.gorest.gorun.goservice/auth.goservice/logger.goservice/rate_limiter.goservice/rate_limiter_store.gosubscriber.gotelemetry.gowebsocket.goChecklist:
goimportandgolangci-lint.