Conversation
There was a problem hiding this comment.
Pull request overview
Updates the project for the @patternslib/dev v4 and Jest v30 upgrade by adjusting dependency versions and updating several Jest test suites to be compatible with newer JSDOM/Jest behavior.
Changes:
- Bump runtime/dev dependency versions (notably
@patternslib/devand related packages). - Update multiple tests to avoid Jest 30 matcher deprecations and JSDOM navigation limitations.
- Adjust
Makefilebundling hook to compose with the parent@patternslib/devMakefile target.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pat/zoom/zoom.test.js | Uses jQuery .css("zoom") assertion instead of reading style.zoom directly. |
| src/pat/navigation/navigation.test.js | Reworks URL/base URL handling to avoid JSDOM navigation issues; adds prototype mocking for base_url. |
| src/pat/inject/inject.test.js | Updates mocks and event dispatch to be more JSDOM/Jest 30 friendly; adjusts error-handler disabling strategy. |
| src/pat/gallery/gallery.test.js | Mocks initialize_gallery to avoid initializing PhotoSwipe during unit tests. |
| src/lib/depends_parse.test.js | Replaces deprecated toThrowError with toThrow. |
| src/core/base.test.js | Replaces deprecated toThrowError with toThrow. |
| package.json | Dependency upgrades (incl. @patternslib/dev v4 and other packages). |
| Makefile | Changes bundle-pre to a double-colon rule to compose with the parent Makefile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Mock the base_url method at the prototype level to avoid JSDOM navigation issues | ||
| const originalBaseUrl = Pattern.prototype.base_url; | ||
| Pattern.prototype.base_url = function () { | ||
| return "https://patternslib.com/"; | ||
| }; |
There was a problem hiding this comment.
Same concern here: Pattern.prototype.base_url is overridden and restored only at the end of the test. If the test fails early, the override can leak. Restore via afterEach or try/finally to guarantee cleanup.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Done in 5d23509. Moved Pattern.prototype.base_url save/override to beforeEach and the restore to afterEach on the describe block, so cleanup is guaranteed regardless of test failures.
| // Mock URLSearchParams to return the parameter that disables error handling | ||
| const originalURLSearchParams = global.URLSearchParams; | ||
| global.URLSearchParams = jest.fn().mockImplementation(() => ({ | ||
| get: (key) => (key === "pat-inject-errorhandler.off" ? "" : null), | ||
| })); |
There was a problem hiding this comment.
This test replaces global.URLSearchParams and restores it only at the end of the test. If the test throws/fails before restoration, the override can leak into later tests (and jest.restoreAllMocks() won’t revert this assignment). Move the restore into an afterEach for this describe block or use try/finally inside the test.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Fixed in cacb872. Wrapped the test body in try/finally so global.URLSearchParams is always restored even when an assertion throws.
| const set_url = (url, portal_url) => { | ||
| global.window.location = { | ||
| // Mock the location by creating a mock object | ||
| const urlObj = new URL(url); | ||
| const mockLocation = { | ||
| href: url, | ||
| protocol: urlObj.protocol, | ||
| host: urlObj.host, | ||
| hostname: urlObj.hostname, | ||
| port: urlObj.port, | ||
| pathname: urlObj.pathname, | ||
| search: urlObj.search, | ||
| hash: urlObj.hash, | ||
| origin: urlObj.origin, | ||
| }; | ||
|
|
||
| // Replace the window.location object | ||
| global.window.location = mockLocation; | ||
|
|
There was a problem hiding this comment.
set_url is no longer called in this describe block, but beforeEach still deletes global.window.location. That leaves window.location undefined during the test run and can break any code-path that reads it (e.g. inject URL rebasing). Either remove the delete global.window.location setup, or call set_url(...) (or otherwise define window.location) in the test(s).
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Fixed in cacb872. Removed delete global.window.location from beforeEach in describe "2" — since set_url is no longer called in that block, window.location no longer needs to be replaced.
| // Mock the base_url method at the prototype level to avoid JSDOM navigation issues | ||
| const originalBaseUrl = Pattern.prototype.base_url; | ||
| Pattern.prototype.base_url = function () { | ||
| return "https://patternslib.com/path/to/test"; | ||
| }; |
There was a problem hiding this comment.
This test mutates Pattern.prototype.base_url and restores it only at the end of the test body. If an assertion throws before the restore, the mocked method can leak into subsequent tests. Prefer restoring in an afterEach for this describe block or via try/finally.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Fixed in cacb872. Moved Pattern.prototype.base_url save/override to beforeEach and restore to afterEach in describe "2", so cleanup is guaranteed regardless of test failures.
No description provided.