test: Improve code coverage of tests by josephschorr · Pull Request #3063 · authzed/spicedb

josephschorr · 2026-04-21T17:42:03Z

Adds unit tests for various packages that had insufficient coverage

Covers every constructor, accessor, MarshalZerologObject, and DetailsMetadata in the schema error types, plus asTypeError, TypeError.Unwrap, NewTypeWithSourceError (with and without source position), and backtickNames. Lifts pkg/schema to 80.7% coverage.

Covers every constructor, accessor, MarshalZerologObject, and DetailsMetadata in the namespace error types.

Covers every constructor, Unwrap, GRPCStatus, MarshalZerologObject, and accessor method in the graph error types.

Covers every constructor, GRPCStatus, and MarshalZerologObject in the v1 services error types, plus the defaultIfZero helper. Exercises full and minimal precondition/filter paths for PreconditionFailedError and CouldNotTransactionallyDeleteError so all branches in their GRPCStatus details are hit.

Adds tests for the previously-uncovered caveat, query, reverse-query, count, and lookup-counter wrappers on the checking replicated reader, the no-replicas degenerate path, and the multi-replica round-robin selection via selectReplica.

Exercises the previously-uncovered caveat/namespace/count/lookup wrappers on strictReadReplicatedReader, the replica-without-strict- read-mode rejection path in NewStrictReplicatedDatastore, and the primary-fallback branch in LegacyLookupNamespacesWithNames.

Adds error-path coverage for NewRelationshipIntegrityProxy (empty and malformed key configs, duplicate key IDs, non-expiring expired keys, current-key-with-expiration panic), full pass-through coverage for the trivial datastore methods (MetricsID, UniqueID, CheckRevision, Close, Features, OfflineFeatures, OptimizedRevision, ReadyState, RevisionFromString, Statistics, Unwrap), reader pass-throughs (CountRelationships, caveat/namespace reads, LookupCounters), ReverseQueryRelationships hash-validation failure, and BulkLoad both in the happy path and with a pre-hashed relationship that triggers the bug-assertion panic.

Adds tests for the previously-uncovered public walker wrappers: WalkCaveat, WalkCaveatWithOptions, WalkBaseRelation, WalkBaseRelationWithOptions, WalkFlattenedSchema, the value- receiver WalkOptions.WithStrategy, and WalkOptionsBuilder.MustBuild on an errored builder. Also covers FlattenSchemaWithOptions with nil input.

Adds direct unit tests for the two previously-uncovered public functions in pkg/query: the NewSchemaArrow constructor (asserts it marks the arrow as a schema arrow and configures left/right and direction) and DatastoreIterator.ReplaceSubiterators (asserts it panics since the datastore iterator is a leaf).

The pkg/datalayer package previously had no test files; any coverage came from downstream consumers. Adds direct tests that wrap an in-memory datastore and exercise: - defaultDataLayer pass-throughs (ReadyState, Features, Stats, etc.) - countingDataLayer query/reverse-query counters on both the snapshot reader and the RW transaction - countingDataLayer pass-throughs and DeleteRelationships - Unary and Stream counting interceptors, with and without exporters - NewReadOnlyDataLayer write rejection and reader pass-throughs - ReadWriteTransaction methods: WriteSchema, LegacySchemaWriter and each legacy writer method, BulkLoad, RegisterCounter / StoreCounterValue / UnregisterCounter

Extends combined_test.go to exercise the previously-uncovered option setters (RelationshipChunkCacheConfig, RemoteDispatchTimeout, CaveatTypeSet, Cache, ConcurrencyLimits, GrpcDialOpts, RelationshipChunkCache) and the upstream-dispatch branch of NewDispatcher. Adds error-path tests for invalid secondary upstream hedging delays (parse failure and non-positive value), invalid secondary dispatch expressions, and missing upstream CA certificate files.

codecov · 2026-04-21T17:46:10Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.56%. Comparing base (a813585) to head (47545b7).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3063      +/-   ##
==========================================
+ Coverage   75.68%   76.56%   +0.89%     
==========================================
  Files         486      486              
  Lines       59433    59433              
==========================================
+ Hits        44973    45500     +527     
+ Misses      11191    10664     -527     
  Partials     3269     3269

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

tstirrat15

See comments, otherwise LGTM

tstirrat15 · 2026-04-22T19:05:16Z

+	dispatcher, err := NewDispatcher(
+		MetricsEnabled(true),
+		PrometheusSubsystem("test_subsystem"),
+		DispatchChunkSize(50),
+		RelationshipChunkCacheConfig(cfg),
+		CaveatTypeSet(caveattypes.Default.TypeSet),
+		Cache(dispatchCache),
+		ConcurrencyLimits(graph.ConcurrencyLimits{Check: 10, LookupResources: 5}),
+	)
+	require.NoError(t, err)
+	require.NotNil(t, dispatcher)


How does this assert that the options were applied? Or are we just saying that they don't cause errors when provided as inputs?

This comment goes for every test in this file

tstirrat15 · 2026-04-22T19:40:07Z

+	err := func() PreconditionFailedError {
+		var target PreconditionFailedError
+		_ = errors.As(NewPreconditionFailedErr(precondition), &target)
+		return target
+	}()


Is this necessary because there isn't a New* constructor for this error type?

tstirrat15 · 2026-04-22T19:44:25Z

+// TestCountingDataLayer_CountsQueryAndReverseQuery ensures QueryRelationships
+// and ReverseQueryRelationships increment their counters on both the snapshot
+// reader and the RW transaction.
+func TestCountingDataLayer_CountsQueryAndReverseQuery(t *testing.T) {


I thought we already had this test? What codepath is it landing on?

i don't even know why we have the CountingDataLayer 🫠 its only prod use has datalayer.UnaryCountingInterceptor(nil). so what's the point? we're not doing anything with the counts.

is this for future use? and how does it differ from the observableProxy?

if we do need it, can we split this file into counting_test.go, impl_test.go, readonly_test.go, etc?

tstirrat15 · 2026-04-22T19:49:11Z

+	_, err = WalkFlattenedSchema(flattened, visitor, struct{}{})
+	require.NoError(t, err)
+
+	require.NotEmpty(t, visitor.schemas)
+	require.GreaterOrEqual(t, len(visitor.definitions), 2)


Is this just testing that the code doesn't error, more or less?

miparnisari · 2026-04-22T20:57:43Z

 	"github.com/authzed/spicedb/pkg/tuple"
 )

+func TestCheckingReplicatedWithNoReplicasReturnsPrimary(t *testing.T) {


there is a godoc in the prod code that says

// NewCheckingReplicatedDatastore (...) // NOTE: Be *very* careful when using this function. It is not safe to use this function without // knowledge of the layout of the underlying datastore and its replicas. // the replicas *must* point to a *stable* instance of the datastore (not a load balancer).

is it possible to encode this knowledge in a test?

Similarly, for NewStrictReplicatedDatastore:

// NewStrictReplicatedDatastore (...) // This is useful when the read pool points to a load balancer that can transparently handle the request.

can we encode that in a test?

(see #2525 (comment) for more context)

miparnisari · 2026-04-22T21:03:44Z

+	require.Error(t, err)
+}
+
+func TestRelationshipIntegrityReverseQueryValidatesHash(t *testing.T) {


we already have a test for this scenario: TestBasicIntegrityFailureDueToInvalidHashSignature. please remove this one

miparnisari · 2026-04-22T21:34:31Z

+}
+
+func TestRelationNotFoundError(t *testing.T) {
+	err := NewRelationNotFoundErr("document", "viewer")


I cannot find any references to NewRelationNotFoundErr. Might as well remove it and get rid of the test too.

miparnisari

Thanks for doing this. Just a few comments. The suggestion on the additional test can be done in a follow up because i think it's not trivial.

josephschorr added 11 commits April 21, 2026 12:17

test: add coverage for internal/namespace/errors.go

102a627

Covers every constructor, accessor, MarshalZerologObject, and DetailsMetadata in the namespace error types.

test: add coverage for internal/graph/errors.go

49c9ad7

Covers every constructor, Unwrap, GRPCStatus, MarshalZerologObject, and accessor method in the graph error types.

github-actions Bot added area/api v1 Affects the v1 API area/datastore Affects the storage system area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) area/dispatch Affects dispatching of requests labels Apr 21, 2026

josephschorr changed the title ~~Improve code coverage of tests~~ test: Improve code coverage of tests Apr 21, 2026

josephschorr added the Skip-Changelog label Apr 21, 2026

chore: lint fixes

47545b7

josephschorr marked this pull request as ready for review April 21, 2026 18:09

josephschorr requested a review from a team as a code owner April 21, 2026 18:09

josephschorr enabled auto-merge (rebase) April 21, 2026 18:38

tstirrat15 approved these changes Apr 22, 2026

View reviewed changes

miparnisari reviewed Apr 22, 2026

View reviewed changes

miparnisari requested changes Apr 22, 2026

View reviewed changes

miparnisari mentioned this pull request May 1, 2026

Using Read Replica on AWS Aurora Postgres causing errors #2525

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Improve code coverage of tests#3063

test: Improve code coverage of tests#3063
josephschorr wants to merge 12 commits intoauthzed:mainfrom
josephschorr:test-cov-improvement

josephschorr commented Apr 21, 2026

Uh oh!

codecov Bot commented Apr 21, 2026 •

edited

Loading

Uh oh!

tstirrat15 left a comment

Uh oh!

tstirrat15 Apr 22, 2026

Uh oh!

tstirrat15 Apr 22, 2026

Uh oh!

tstirrat15 Apr 22, 2026

Uh oh!

tstirrat15 Apr 22, 2026

Uh oh!

miparnisari Apr 22, 2026 •

edited

Loading

Uh oh!

tstirrat15 Apr 22, 2026

Uh oh!

miparnisari Apr 22, 2026 •

edited

Loading

Uh oh!

miparnisari Apr 22, 2026

Uh oh!

miparnisari Apr 22, 2026

Uh oh!

miparnisari left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

josephschorr commented Apr 21, 2026

Uh oh!

codecov Bot commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tstirrat15 left a comment

Choose a reason for hiding this comment

Uh oh!

tstirrat15 Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

tstirrat15 Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

tstirrat15 Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

tstirrat15 Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

miparnisari Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tstirrat15 Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

miparnisari Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miparnisari Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

miparnisari Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

miparnisari left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Apr 21, 2026 •

edited

Loading

miparnisari Apr 22, 2026 •

edited

Loading

miparnisari Apr 22, 2026 •

edited

Loading