Conversation
Contributor
|
Droid finished @gregns1's task —— View job This is a straightforward test refactor. The main remaining risk is test-only helpers diverging from production behavior, so it would help to fail fast on |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
New test helpers in db/util_testing.go have correctness gaps (not failing on feed entry errors and a query test double ignoring start/end bounds) that can mask real failures and diverge from production behaviour.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR reorganizes Sync Gateway’s cache and changes-feed tests into dedicated sub-packages (e.g. db/changesfeedtest, db/changecachetest, db/channelcachetest, rest/changestest) to isolate coverage and reduce reliance on in-package access to unexported symbols. To support out-of-package tests, it introduces a set of thin “for test” bridges and helpers in db/util_testing.go.
Changes:
- Moved changes-feed, change-cache, and channel-cache tests into new sub-packages with their own
TestMainbootstrapping. - Added exported test helpers/bridges in
db/util_testing.gofor accessing unexported internals from out-of-package tests. - Updated many existing
db/*_test.gofiles to use the new exported setup/helpers (e.g.SetupTestDB,GetChangesForTest), and kept a small set of tests in-package where locks/private methods must be held/called directly.
File summaries
| File | Description |
|---|---|
| rest/changestest/changes_test.go | Moved REST changes tests into changestest and updated references to rest.* helpers. |
| rest/changes_api_test.go | Added back in-package handler test for readChangesOptionsFromJSON. |
| db/util_testing.go | Added new exported test helpers/bridges and a query handler test double for out-of-package tests. |
| db/users_test.go | Switched to exported SetupTestDB. |
| db/sg_replicate_cfg_test.go | Switched to exported SetupTestDB. |
| db/revtree_test.go | Switched to exported SetupTestDB. |
| db/revision_cache_test.go | Switched to exported SetupTestDB. |
| db/query_test.go | Switched to exported SetupTestDB. |
| db/import_test.go | Switched to exported SetupTestDB / SetupTestLeakyDBWithCacheOptions. |
| db/hybrid_logical_vector_test.go | Switched to exported SetupTestDB. |
| db/document_test.go | Switched to exported SetupTestDB. |
| db/delta_cache_test.go | Switched to exported SetupTestDB. |
| db/database_test.go | Removed now-exported local setup helpers and updated call sites to exported versions. |
| db/crud_test.go | Switched to exported SetupTestDB and exported changes helpers. |
| db/channelcachetest/main_test.go | New subpackage TestMain bootstrapping for channel cache tests. |
| db/changesfeedtest/main_test.go | New subpackage TestMain bootstrapping for changes feed tests. |
| db/changesfeedtest/sequence_id_test.go | Moved sequence ID tests into changesfeedtest and updated to use exported APIs. |
| db/changesfeedtest/changes_test.go | Moved changes feed tests into changesfeedtest and updated to use exported bridges/helpers. |
| db/changecachetest/main_test.go | New subpackage TestMain bootstrapping for change cache tests. |
| db/changecachetest/skipped_sequence_test.go | Moved skipped-sequence tests into changecachetest and updated to use exported bridges. |
| db/changecachetest/change_listener_test.go | Moved change-listener tests into changecachetest and updated to use exported bridges. |
| db/changecachetest/change_listener_dual_metadata_test.go | Moved caching-feed collections test into changecachetest and updated to use exported bridge. |
| db/change_cache_pending_test.go | Kept lock-sensitive pending-log test in-package db. |
| db/blip_test.go | Switched to exported SetupTestDB. |
| db/background_mgr_resync_dcp_test.go | Switched to exported SetupTestDB. |
| db/background_mgr_attachment_migration_test.go | Switched to exported SetupTestDB. |
| db/attachment_test.go | Switched to exported SetupTestDB / SetupTestLeakyDBWithCacheOptions. |
| db/attachment_compaction_test.go | Switched to exported SetupTestDB / SetupTestDBDefaultCollection. |
| db/access_test.go | Switched to exported SetupTestDB. |
Review details
Suppressed comments (1)
rest/changestest/changes_test.go:407
- The second status assertion re-checks the previous PUT response (res) rather than the non-winning branch write. It’s redundant (PutNewEditsFalse already asserts 201) and makes the test intent unclear.
- Files reviewed: 32/33 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1319
to
+1323
| var changes = make([]*ChangeEntry, 0, 50) | ||
| for entry := range feed { | ||
| changes = append(changes, entry) | ||
| } | ||
| return changes |
Contributor
Author
There was a problem hiding this comment.
TestChannelQueryCancellation fails with this change. Have removed the comment about the error instead
![factory-droid[bot]](https://avatars.githubusercontent.com/in/358017?s=60&v=4){kind=small}
Comment on lines
+1320
to
+1322
| for entry := range feed { | ||
| changes = append(changes, entry) | ||
| } |
Contributor
There was a problem hiding this comment.
[P1] Fail tests when a changes feed emits an error
MultiChangesFeed can signal failures by sending a ChangeEntry with Err set (for example base.ErrChannelFeed) before closing. GetChangesForTest currently appends entries without checking entry.Err, which can let tests treat an error sentinel as a valid change and pass incorrectly.
Suggested change
| for entry := range feed { | |
| changes = append(changes, entry) | |
| } | |
| for entry := range feed { | |
| require.NotNil(t, entry) | |
| require.NoError(t, entry.Err) | |
| changes = append(changes, entry) | |
| } |
Contributor
Author
There was a problem hiding this comment.
See comment above on copilot review
The seven helpers used by tests across package db were defined in change_cache_test.go, which is about to move to db/changecachetest. Moving the file would have taken them with it and broken 177 call sites in the tests that stay behind. Relocates the six phase-2 delegations to db/change_cache_helpers_test.go, and exports the one real helper among them - the tombstoned-entry builder et - as MakeDeletedTestLogEntry in util_testing.go, since it has consumers on both sides of the boundary. Also adds db/changesfeedtest/main_test.go, which was left untracked when the changes feed tests moved, leaving that package with no TestMain and no bucket pool. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TestAddPendingLogs cannot move to db/changecachetest. It acquires changeCache.lock and, while holding it, pushes directly onto the pendingLogs heap and calls _pushRangeToPending and _addPendingLogs - underscore-prefixed methods whose contract is that the caller already holds the lock. A held lock cannot cross a package boundary, and an accessor that took the lock itself would stop exercising the sequence-buffering path the test exists to pin down. Moves it, with the four helpers only it uses, to change_cache_pending_test.go so the remainder of change_cache_test.go can move as a whole file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds the 24 accessors db/changecachetest needs to drive changeCache, SkippedSequenceSkiplist, singleChannelCacheImpl and lateLogEntry from outside package db. Derived by attempting the move and letting the compiler enumerate, rather than by grep - a single pass finds only a third of them, because an unnameable type masks every method call on values of that type. All are thin pass-throughs. Two read fields without taking changeCache.lock - NextSequence and GetMaxStableCached - because the tests they replace did the same; production's only caller of _getMaxStableCached holds the read lock, so the lock discipline there is worth revisiting on its own rather than as a side effect of relocating tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Moves change_cache_test.go, skipped_sequence_test.go, change_listener_test.go
and change_listener_dual_metadata_test.go out of package db, so the change
cache can be tested and its coverage measured on its own.
54 tests and 10 benchmarks move; TestAddPendingLogs stays behind. Test names
are unchanged - 65 before, 65 after, verified by name rather than by count.
The move is mechanical apart from four edits the compiler cannot make safely:
the three ChangeEntry literals that set collectionID are hoisted so the
existing SetCollectionID accessor can be used; the seven user assignments are
restructured with a non-colliding variable name, since the obvious rewrite
reassigns a user already in scope; the two &Database{user: ...} literals keep
the literal and call SetUserForTest; and the two notifyChangeFunc assignments
become SetNotifyChangeFunc calls. The assertion message in verifyCacheSequences
no longer names singleChannelCacheImpl, which is not nameable from here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n shims Phase 2 exported each shared test helper and left a one-line unexported delegation behind, to avoid renaming call sites in package db. The delegations have no purpose: they sit in the same package as the exported implementations, so the tests can call those directly. Removes eleven of them - the four log-entry builders, the tombstoned-entry helper, shortWaitCache, getChanges, and the four setup wrappers - and renames their 220 call sites. Signatures were identical in every case, so this is a pure rename; the diff is 178 changed lines against 178, and no comment or string literal was touched. Removing logEntry also resolves a name that meant both a helper function and a local variable in the same package. setupTestDB is left for a separate commit - 157 call sites across 20 files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The final phase-2 delegation. 157 call sites across 20 files, all a pure rename to the exported name in the same package - no comment or string literal changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Gives every accessor added for db/changecachetest a leading `_ testing.TB` and a ForTest suffix, so a call from production code reads as obviously wrong and the whole set is greppable by name. The parameter is a signal, not a barrier: nil satisfies testing.TB, so production code can still call these if it insists. The usual secondary deterrent - that it forces a testing import - does not apply here either, since db/util_testing.go, db/database.go and base/logging.go already make testing a dependency of the production binary. Enforcement would need a ruleguard rule; this commit does not add one. The suffix is applied uniformly rather than case by case, so there is no judgement call about which names look sufficiently like production API. It also resolves a real collision: changeCache.NextSequence sat alongside the production DatabaseContext.NextSequence. Two benchmark helpers in skipped_sequence_test.go gained a testing.TB parameter so they can reach the accessors they set up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds the accessors db/channelcachetest needs to drive channelCacheImpl, singleChannelCacheImpl and the bypass cache from outside package db. Derived by attempting the move and compiling to closure - three probe rounds, then three more accessors surfaced during the move itself, because newSingleChannelCache and newChannelCache return unnameable types and mask every method call on their results. A single pass finds six of them. Notable shapes: singleChannelCacheImpl.options is already a *ChannelCacheOptions, so OptionsForTest is a plain getter and the twelve sites that adjust cache caps in place keep working. addChannelCache returns the concrete cache and a capacity flag rather than an error, so AddChannelCacheForTest mirrors that. WaitForChannelCacheCompactionForTest absorbs the waitForCompaction helper from channel_cache_test.go, whose *channelCacheImpl parameter cannot be named from another package. Two exceptions to the accessor-only rule, both because test code has to name a type rather than merely hold it - a type assertion, and a closure whose result type is the concrete cache. ChannelCacheImplForTest and SingleChannelCacheImplForTest are aliases for exactly those cases; no accessor can stand in for a result type. AddListenerToNewestLateLogForTest is the one accessor that is not a bare pass-through: it takes lateLogLock for precisely the operation TestLateLogsSpikeForcePrunedBoundsLateLogsAndForcesRollback performed inline, so the critical section is unchanged. Encapsulating it avoids stranding that test in package db the way TestAddPendingLogs is. No wrapper struct: an exported constructor returning an unexported type is usable by inference, so the 78 addToCache call sites need one exported method rather than a parallel type. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Moves channel_cache_test.go, channel_cache_single_test.go and channel_cache_latelog_test.go out of package db, so the channel cache can be tested and its coverage measured on its own. All 37 tests, 7 benchmarks and the fuzz target move; nothing stays behind. Unlike the change cache, these three files are self-contained - compiling db's remaining tests without them reports no undefined identifiers - so no helper relocation was needed first. Test names are unchanged, 45 before and 45 after, verified by name rather than by count. The move is mechanical apart from edits the compiler cannot make safely: the DatabaseCollectionWithUser literal carrying the unexported user field is restructured around the existing setter, the eight user assignments use it too, the change cache is reached through the database rather than the collection router, and the two cache implementations are named through the aliases added in the previous commit. The waitForCompaction helper is deleted in favour of the accessor that absorbed it, which is why "time" leaves channel_cache_test.go's imports - the only string literal that changes across all three files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
QueryHandlerForTest.getChangesInChannelFromQuery filtered on channel, activeOnly and limit but ignored the sequence bounds, so it could return entries outside the range the caller asked for. singleChannelCacheImpl.GetChanges feeds that result to prependChanges with startSeq/resultValidTo as the cached range, and then concatenates it with the cache result behind a de-dup that only drops a single overlapping entry - so out-of-range rows could both poison the cached range and duplicate. Pre-existing behaviour, carried over from testQueryHandler, but worth fixing now the double is exported and will be reached for by new tests. The bounds follow DatabaseCollection.QueryChannels: both inclusive (the N1QL statement uses BETWEEN [$channelName, $startSeq] AND [$channelName, $endSeq], and the views path uses startkey/endkey with the default inclusive_end), and an endSeq of 0 means unbounded, which query.go turns into N1QLMaxInt64. They are applied before the limit, so entries outside the range no longer consume it. No existing test depended on either behaviour: instrumenting the double to compute both results across db, db/channelcachetest, db/changecachetest and db/changesfeedtest recorded 6250 calls and zero divergences. Every call used startSeq=1 with endSeq of either 2 or MaxUint64, limit=0 and activeOnly=false, so the bounds could not exclude anything - and the limit and activeOnly filters were not exercised either. Adds db/util_testing_test.go, which pins the filter contract directly. It has to live in package db because getChangesInChannelFromQuery is unexported. The test fails against an exclusive startSeq, an exclusive endSeq, a missing endSeq==0 sentinel, and a limit applied before the bounds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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.
CBG-5784
Moving tests into sub-packages for channel cache, change cache and changes feed.
Things to note:
type ChannelCacheImplForTest = channelCacheImpltype SingleChannelCacheImplForTest = singleChannelCacheImplThis is to support type assertion for unexported types in db package in tests that have moved.TestAddPendingLogsdid not move because it holdschangeCache.lockfor_pushRangeToPending/_addPendingLogsTestReadChangesOptionsFromJSONdid not move also because it uses private method on private typeChannelQueryHandler(its only method is unexported)NOTE: decision to be made on whether this change is still wanted. With extra accessor work made etc.
Pre-review checklist
base.UD(docID),base.MD(dbName))docs/apiDependencies (if applicable)
Integration Tests