fix(driver-utils): address PR #26151 review comments#26204
fix(driver-utils): address PR #26151 review comments#26204anthony-murphy merged 4 commits intomicrosoft:mainfrom
Conversation
- Replace `any` with proper error types in tests using GenericNetworkError and NonRetryableError from network.ts - Add test for non-retryable errors to verify cache behavior (errors without canRetry=true should remain cached, not cleared) - Skip prefetch calls if blobId is already cached to avoid unnecessary async overhead Co-Authored-By: anthony-murphy <anthony.murphy@microsoft.com> Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
/azp run Build - api-markdown-documenter, Build - benchmark-tool, Build - build-common, Build - build-tools, Build - client packages, Build - common-utils, Build - eslint-config-fluid, Build - eslint-plugin-fluid, Build - protocol-definitions, Build - test-tools |
|
/azp run repo-policy-check, server-gitrest, server-gitssh, server-historian, server-routerlicious |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
jason-ha
left a comment
jason-ha
left a comment
There was a problem hiding this comment.
#26151 (comment) (use of any) appears addressed :)
packages/loader/driver-utils/src/test/prefetchDocumentStorageService.spec.ts
Outdated
Show resolved
Hide resolved
…ntity Address review feedback: use non-retryable error in the fire-and-forget prefetch failure test so we can verify the cached error instance identity rather than just checking the message. Co-Authored-By: anthony-murphy <anthony.murphy@microsoft.com> Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
/azp run Build - api-markdown-documenter, Build - benchmark-tool, Build - build-common, Build - build-tools, Build - client packages, Build - common-utils, Build - eslint-config-fluid, Build - eslint-plugin-fluid, Build - protocol-definitions, Build - test-tools |
|
/azp run repo-policy-check, server-gitrest, server-gitssh, server-historian, server-routerlicious |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
packages/loader/driver-utils/src/test/prefetchDocumentStorageService.spec.ts
Show resolved
Hide resolved
Add explicit unhandled rejection tracking to the fire-and-forget prefetch failure tests. This actively verifies no unhandled rejections occur rather than relying on implicit behavior. Co-Authored-By: anthony-murphy <anthony.murphy@microsoft.com> Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
packages/loader/driver-utils/src/test/prefetchDocumentStorageService.spec.ts
Outdated
Show resolved
Hide resolved
Change the first assert.rejects in the non-retryable error test to validate via error instance identity rather than message comparison, for consistency with other tests. Co-Authored-By: anthony-murphy <anthony.murphy@microsoft.com> Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Azure Pipelines commands (copy and post separately, limit of 10 at a time): |
There was a problem hiding this comment.
Pull request overview
This PR addresses review feedback from #26151 by improving type safety in tests, adding test coverage for non-retryable errors, and optimizing prefetch performance.
Changes:
- Replace
anytype usage with proper error types (GenericNetworkErrorandNonRetryableError) - Add comprehensive test for non-retryable error caching behavior
- Optimize prefetch performance by skipping cache lookups for already-cached blobs
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/loader/driver-utils/src/test/prefetchDocumentStorageService.spec.ts | Added typed error helper functions, new non-retryable error test, and improved unhandled rejection tracking in existing tests |
| packages/loader/driver-utils/src/prefetchDocumentStorageService.ts | Added cache checks before prefetch calls to avoid unnecessary async overhead when blobs are already cached |
jason-ha
left a comment
jason-ha
left a comment
There was a problem hiding this comment.
My test concerns were addressed.
For the implementation, would PromiseCache be better to use? Could be for a different follow-up.
|
/azp run Build - api-markdown-documenter, Build - benchmark-tool, Build - build-common, Build - build-tools, Build - client packages, Build - common-utils, Build - eslint-config-fluid, Build - eslint-plugin-fluid, Build - protocol-definitions, Build - test-tools |
|
/azp run repo-policy-check, server-gitrest, server-gitssh, server-historian, server-routerlicious |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
Summary
anywith proper error types in tests usingGenericNetworkErrorandNonRetryableErrorThis is a follow-up PR to address review comments from #26151:
anyandas any- errors should use proper typesreadBlobcall if blobId is already inprefetchCacheTest plan
🤖 Generated with Claude Code
Co-Authored-By: anthony-murphy anthony.murphy@microsoft.com