test: add files plugin upload + error-handling coverage (50 tests)#319
Merged
Conversation
Adds a focused test file covering FilesPlugin paths that were lightly exercised: the upload streaming path, error mapping in _handleApiError, cache invalidation around mutations, raw/download response headers, shutdown behavior, volume discovery merging, path validation, and the clientConfig surface. Originally proposed in PR #256 alongside the analytics ARROW_STREAM work, but the file shipped with three classes of failures and was removed from that PR to unblock its CI: 1. Missing per-volume policy on test configs. The default policy is publicRead(), which denies upload/mkdir/delete and surfaced as 403 instead of the SDK-error status codes the tests asserted. Fixed by setting policy: policy.allowAll() on every test volume. 2. vi.spyOn(connector, "upload") inferred a stricter signature than the test's three-arg async mock. Switched the mock to spread args with a typed cast. 3. AuthenticationError test asserted "token" in the error message, but _extractUser actually surfaces the missing-x-forwarded-user path. Updated the assertion to match the real behavior. Co-authored-by: Isaac Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
The previous single file name implied upload + write coverage, but the file actually held tests for ~10 separate concerns (download, raw, path validation, shutdown, volume discovery, etc). Split into focused files: - error-handling.test.ts _handleApiError + _sendStatusError - upload.test.ts upload streaming + cache invalidation - raw-endpoint.test.ts /raw security headers - download-endpoint.test.ts /download Content-Disposition - delete.test.ts delete + cache invalidation - mkdir.test.ts mkdir + cache invalidation - shutdown.test.ts shutdown + trackWrite - volume-config.test.ts discoverVolumes + clientConfig - path-validation.test.ts null-byte / 4096-cap / required-path Shared pure helpers (mockReq/mockRes/getRouteHandler/etc) extracted to _test-helpers.ts. The vi.hoisted/vi.mock block stays inlined per file to match the convention in plugin.test.ts. No test logic changes: 50/50 still passing in the files plugin (1242 across appkit). Co-authored-by: Isaac Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
Three test-quality fixes flagged by GPT 5.4 + Gemini 3.1 Pro review: - shutdown.test.ts: assert the shutdown promise stays unresolved while inflightWrites > 0 — the previous test would silently pass even if shutdown() returned immediately (the final inflightWrites === 0 check is trivially true regardless of waiting). - path-validation.test.ts: assert the SDK connector was not called when the handler rejects the input. Previously a regression that hit the SDK *and* returned 400 would still pass. - _test-helpers.ts: lowercase override.headers keys in mockReq so a caller passing "Content-Type" matches the case-insensitive req.header() lookup. Co-authored-by: Isaac Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
atilafassina
approved these changes
May 12, 2026
atilafassina
added a commit
that referenced
this pull request
May 15, 2026
… shape After rebasing onto main, three tests from main's PR #319 expected the pre-hardening error shape (raw `error.message` echoed back), while the plugin now returns generic public bodies per the CWE-209 fix in bd212d7: - _handleApiError 4xx now responds with STATUS_CODES[status] ("Unauthorized", "Not Found", etc.) instead of the upstream message. - Route-level AuthenticationError now returns the generic 401, not the raw "Missing x-forwarded-user" text. Test updates: - "AuthenticationError" handler test now asserts `"Unauthorized"` and was renamed to reflect that the raw message stays server-side. - "ApiError 404" test now asserts `"Not Found"` (STATUS_CODES[404]). - The route-level test was reworked to exercise an OBO volume, which is the only path that still produces 401 from a route (SP fallback is policy-gated now, not unconditional). Signed-off-by: Atila Fassina <atila@fassina.eu>
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
2 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
Adds a focused test file (
packages/appkit/src/plugins/files/tests/upload-and-write.test.ts) covering FilesPlugin paths that are currently lightly exercised:_handleApiError—AuthenticationError→ 401,ApiError4xx/5xx variants, fallback to 500 for non-ApiErrorContent-DispositiontrackWrite(in-flight write tracking, 10s deadline)clientConfigsurface_sendStatusError50 tests across 13 describe blocks, ~1245 lines. Coverage target for the files plugin: 69% → ~89%.
Background
Originally proposed in #256 alongside the analytics ARROW_STREAM work. The file shipped with three classes of failures and was removed from #256 to unblock its CI. This PR resurrects it with the failures fixed:
mainispublicRead(), which denies upload/mkdir/delete and surfaced as 403 instead of the SDK-error status codes the tests asserted. Fixed by settingpolicy: policy.allowAll()on every test volume.vi.spyOn(connector, "upload")mock signature. Inferred a stricter signature than the test's three-arg async mock. Switched the mock to a spread-args function with a typed cast.AuthenticationErrortest asserted "token" in the error message, but_extractUseractually surfaces the missing-x-forwarded-userpath first. Updated the assertion to match real behavior, and the test name to reflect what's actually being tested.Test plan
pnpm exec vitest run packages/appkit/src/plugins/files/tests/upload-and-write.test.ts— 50/50 passingpnpm --filter=@databricks/appkit typecheck— cleanThis pull request was AI-assisted by Isaac.