-
Notifications
You must be signed in to change notification settings - Fork 16
Optional deps #453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Optional deps #453
Changes from 2 commits
47d307c
0d7a9e0
0dd1dbd
28066a0
1b2f1d1
66a6b04
338b0d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,75 @@ | ||||||||||||||||||||||||||||||||||||||||||||
| describe("optional peer dependency errors", () => { | ||||||||||||||||||||||||||||||||||||||||||||
| const makeNotFound = (pkg) => { | ||||||||||||||||||||||||||||||||||||||||||||
| const err = new Error(`Cannot find module '${pkg}'`) | ||||||||||||||||||||||||||||||||||||||||||||
| err.code = "MODULE_NOT_FOUND" | ||||||||||||||||||||||||||||||||||||||||||||
| return err | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| test("aws-s3 throws helpful error when @aws-sdk/client-s3 is missing", () => { | ||||||||||||||||||||||||||||||||||||||||||||
| jest.isolateModules(() => { | ||||||||||||||||||||||||||||||||||||||||||||
| jest.doMock("@aws-sdk/client-s3", () => { | ||||||||||||||||||||||||||||||||||||||||||||
| throw makeNotFound("@aws-sdk/client-s3") | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
| expect(() => require("../../srv/attachments/aws-s3")).toThrow( | ||||||||||||||||||||||||||||||||||||||||||||
| "npm install @aws-sdk/client-s3 @aws-sdk/lib-storage", | ||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| test("aws-s3 throws helpful error when @aws-sdk/lib-storage is missing", () => { | ||||||||||||||||||||||||||||||||||||||||||||
| jest.isolateModules(() => { | ||||||||||||||||||||||||||||||||||||||||||||
| jest.doMock("@aws-sdk/lib-storage", () => { | ||||||||||||||||||||||||||||||||||||||||||||
| throw makeNotFound("@aws-sdk/lib-storage") | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
| expect(() => require("../../srv/attachments/aws-s3")).toThrow( | ||||||||||||||||||||||||||||||||||||||||||||
| "npm install @aws-sdk/client-s3 @aws-sdk/lib-storage", | ||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+19
to
+28
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Same issue as the first test —
Suggested change
Double-check suggestion before committing. Edit this comment for amendments. Please provide feedback on the review comment by checking the appropriate box:
|
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| test("azure-blob-storage throws helpful error when @azure/storage-blob is missing", () => { | ||||||||||||||||||||||||||||||||||||||||||||
| jest.isolateModules(() => { | ||||||||||||||||||||||||||||||||||||||||||||
| jest.doMock("@azure/storage-blob", () => { | ||||||||||||||||||||||||||||||||||||||||||||
| throw makeNotFound("@azure/storage-blob") | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
| expect(() => require("../../srv/attachments/azure-blob-storage")).toThrow( | ||||||||||||||||||||||||||||||||||||||||||||
| "npm install @azure/storage-blob", | ||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+30
to
+39
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: All remaining
Suggested change
Double-check suggestion before committing. Edit this comment for amendments. Please provide feedback on the review comment by checking the appropriate box:
|
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| test("gcp throws helpful error when @google-cloud/storage is missing", () => { | ||||||||||||||||||||||||||||||||||||||||||||
| jest.isolateModules(() => { | ||||||||||||||||||||||||||||||||||||||||||||
| jest.doMock("@google-cloud/storage", () => { | ||||||||||||||||||||||||||||||||||||||||||||
| throw makeNotFound("@google-cloud/storage") | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
| expect(() => require("../../srv/attachments/gcp")).toThrow( | ||||||||||||||||||||||||||||||||||||||||||||
| "npm install @google-cloud/storage", | ||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| test("aws-s3 loads successfully when SDKs are present", () => { | ||||||||||||||||||||||||||||||||||||||||||||
| jest.isolateModules(() => { | ||||||||||||||||||||||||||||||||||||||||||||
| jest.dontMock("@aws-sdk/client-s3") | ||||||||||||||||||||||||||||||||||||||||||||
| jest.dontMock("@aws-sdk/lib-storage") | ||||||||||||||||||||||||||||||||||||||||||||
| expect(() => require("../../srv/attachments/aws-s3")).not.toThrow() | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| test("azure-blob-storage loads successfully when SDK is present", () => { | ||||||||||||||||||||||||||||||||||||||||||||
| jest.isolateModules(() => { | ||||||||||||||||||||||||||||||||||||||||||||
| jest.dontMock("@azure/storage-blob") | ||||||||||||||||||||||||||||||||||||||||||||
| expect(() => | ||||||||||||||||||||||||||||||||||||||||||||
| require("../../srv/attachments/azure-blob-storage"), | ||||||||||||||||||||||||||||||||||||||||||||
| ).not.toThrow() | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| test("gcp loads successfully when SDK is present", () => { | ||||||||||||||||||||||||||||||||||||||||||||
| jest.isolateModules(() => { | ||||||||||||||||||||||||||||||||||||||||||||
| jest.dontMock("@google-cloud/storage") | ||||||||||||||||||||||||||||||||||||||||||||
| expect(() => require("../../srv/attachments/gcp")).not.toThrow() | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug:
expectassertions insidejest.isolateModules()callbacks are silently swallowed on failurejest.isolateModules()executes the callback synchronously and propagates thrown errors, but the callback's return value is discarded. If anexpect(...).toThrow()assertion fails (i.e., no error is thrown),expectitself throws internally — and that will propagate, so failure detection works in that direction. However, forexpect(...).not.toThrow()(lines 56, 65, 72), if the module does throw, the error is thrown inside the callback but Jest may not always reliably associate it with the enclosing test, especially when modules load side-effects or when an unhandled rejection occurs.More critically, these tests that assert successful loading (
.not.toThrow()) load real modules that have real side-effects (e.g.,aws-s3.jscallscds.log(...), requires../../lib/helper, etc.). If those side-effect requires fail, the error propagates, but the isolation is not guaranteed to be clean across tests becausejest.dontMockinsidejest.isolateModulesdoesn't reset the mock registry for modules that were previously mocked in earlier tests.The safest pattern is to return the
jest.isolateModulescall from the test callback, ensuring the module registry is properly scoped:Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box: