Repository navigation
Offline test for a cloud-only series manifest (SyncFiles=false shape) (#986) - #987
Merged
stevevanhooser merged 2 commits intoSep 14, 2026
Merged
Conversation
DID-matlab#201 (merged as PR #202) opened the read path for a series whose manifest bytes are only remote: seriesMemberPath now offers the manifest's file_info location to the customFileHandler, caches the result at filecachepath/<manifestUid>, and proceeds. That path had no NDI-side coverage: FileSeriesRoundTripTest requires NDI Cloud credentials and only covers SyncFiles=true (the manifest arrives with its bytes), and TestDownloadedSeriesManifestKeepsItsCloudLocation checks only the file_info shape without touching the database. Add TestSeriesWithCloudOnlyManifestResolvesThroughHandler: an offline suite that builds a demoNDISeries with ndi.document.addFileSeries, reshapes its file_info into the SyncFiles=false shape via ndi.cloud.sync.internal.updateFileInfoForRemoteFiles, adds the doc to a DID sqlite database, and opens a member with a uid-keyed mock handler. Four tests: * The handler is called for the manifest first (ctx.uid = manifest's own uid, seriesName='', mode='open', sourcePath = the ndic:// URL verbatim), and the member reads back byte-for-byte. * The manifest lands at filecachepath/<manifestUid> so the next cached_path_for_uid on that uid is a hit. * Three different member opens share ONE manifest handler call, the 28,000-member-level guarantee. * A handler that serves nothing for the manifest still raises DID:SQLITEDB:open with the "not on this machine" message shape -- the identifier and phrase callers already pattern-match on. The test uses did.implementations.sqlitedb directly rather than ndi.dataset.database_openbinarydoc: NDI's didsqlite hard-codes @download_file_from_cloud at open time (right at runtime, wrong here), and ndi.session.database is not publicly reachable to swap. The document is still built through ndi.document / addFileSeries and reshaped through updateFileInfoForRemoteFiles, so the NDI-side pieces this test exists to cover are still exercised. Deferred to a follow-up on #986: retiring the cloud-reference workaround in updateFileInfoForLocalFiles.m:96-101. That change needs verification against the existing FileSeriesRoundTripTest, which requires NDI Cloud credentials.
Contributor
3 tasks
… helper
Two of the four new tests failed on CI because they read back
`calls{1}` as empty even though the member reads succeeded --
proving the handler had run but the recorded calls list was not
what the caller saw.
Root cause: the previous `uidKeyedHandler` helper defined the mock
as a nested function inside the helper and returned `callsHandle`
as an output. MATLAB nested functions share the enclosing
function's workspace, so writes went into the helper's own
`callsHandle` variable -- but a cell array returned from a
function is a value copy. The caller's `calls` was a snapshot
taken at return time and never updated afterwards. Every call to
the handler kept updating the helper's captured workspace, which
the test could not read.
The fix mirrors DID-matlab's own convention (see
TestSeriesMemberFetch.testHandlerGetsTheSeriesNameAndTheMemberUid):
define the mock as a nested function inside each test method that
needs to inspect `calls`. The closure then writes into the test
method's own workspace, which the assertions read directly. No
helper return-by-value, no lost updates.
testTheManifestLandsAtFilecachepath and testDifferentMembersShareOneManifestFetch
also inline their handlers for consistency, and the unused
uidKeyedHandler helper is removed.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #987 +/- ##
==========================================
+ Coverage 29.69% 29.71% +0.02%
==========================================
Files 857 857
Lines 43641 43641
==========================================
+ Hits 12960 12970 +10
+ Misses 30681 30671 -10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
stevewds
approved these changes
Sep 14, 2026
stevevanhooser
deleted the
claude/ndi-986-offline-syncfiles-false-test
branch
September 14, 2026 18:42
This was referenced Sep 14, 2026
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
3 participants
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
Addresses the test half of #986. Adds an offline unit test that pins the path VH-Lab/DID-matlab#201 (merged as PR #202) opened: a series whose manifest bytes are only remote — the shape
SyncFiles=falseproduces — now resolves through thecustomFileHandler, andTestSeriesWithCloudOnlyManifestResolvesThroughHandlerproves that end-to-end from anndi.documentconstruction all the way through DID'sopen_doc.What the test covers
Four scenarios, using
demoNDISeriesand a uid-keyed mock handler (no cloud credentials needed):testTheManifestIsFetchedThroughTheHandler— the acceptance case. The mock receivesctx.uid = manifest's own uid,ctx.seriesName = '',ctx.mode = 'open', andsourcePath = 'ndic://<datasetId>/<manifestUid>'verbatim; the member reads back byte-for-byte.testTheManifestLandsAtFilecachepath— the manifest is cached at<filecachepath>/<manifestUid>after the first open, so the nextcached_path_for_uidon that uid is a hit.testDifferentMembersShareOneManifestFetch— three different member opens result in one manifest handler call. The 28,000-member-level guarantee.testAHandlerThatServesNothingRaisesManifestNotLocal— the handler-refused case still raisesDID:SQLITEDB:openwith the "not on this machine" message shape, so callers pattern-matching on it keep working.Design notes
did.implementations.sqlitedbdirectly rather thanndi.dataset.database_openbinarydoc. NDI'sdidsqlitehard-codes@download_file_from_cloudas the customFileHandler at both add and open time (right at runtime, wrong for an offline test), andndi.session.databaseis not publicly reachable to swap. The document is still built throughndi.document.addFileSeries, and reshaped throughndi.cloud.sync.internal.updateFileInfoForRemoteFiles— the two NDI-side pieces the test exists to cover.Validate=falseonadd_docs: the schema-lookup pathsndi.documentvalidates against are not set up by the isolated working folder, and every question this test asks is about the read path, not about schema validation.did.ido.unique_id()so a lingering entry is harmless in isolation, but this suite asserts the cache placement itself — leaving the entry behind would let a broken setup pass on the next run by finding what a previous run wrote.Not this PR — the follow-up on #986
The other half of #986 — retiring the cloud-reference workaround at
updateFileInfoForLocalFiles.m:96–101— is deferred. That change needs verification against the existingndi.unittest.cloud.FileSeriesRoundTripTest, which requires NDI Cloud credentials. This PR stops short of that; issue #986 stays open for a maintainer with cloud access to close it.The workaround is now "belt and suspenders" rather than strictly required:
SyncFiles=trueshape: DID-matlab#191 already made a local-file manifest reachable by the handler.SyncFiles=falseshape: DID-matlab#201 (this test's subject) added the missing case.But removing it without running the round-trip test against production would risk a regression NDI-matlab CI can't catch.
Related
Test plan
ndi.unittest.TestSeriesWithCloudOnlyManifestResolvesThroughHandler— the new suite (offline, no cloud credentials needed).ndi.unittest.TestDownloadedSeriesManifestKeepsItsCloudLocation— the sibling offline test should keep passing.Cannot run MATLAB locally in this session — please review and validate before merging.
🤖 Generated with Claude Code
https://claude.ai/code/session_015dKpDXCo8VTS4AHiibNnCp
Generated by Claude Code