Improve mocks for localStorage, Worker, and matchMedia#11682
Improve mocks for localStorage, Worker, and matchMedia#11682Riteshhh08 wants to merge 5 commits intoant-design:masterfrom
Conversation
Refactor localStorage and Worker mocks for better functionality. Update matchMedia polyfill and error handling in console.
dosubot
Bot
added
the
size:XS
|
⚡️ Deploying PR Preview... |
步骤说明更新了测试环境中的全局对象模拟和多边形垫片，包括完善 localStorage、Worker、MessageChannel、matchMedia 和 ResizeObserver 的实现，以及调整控制台错误拦截机制，改进测试隔离性。 变更
诗
代码审查工作量评估🎯 2 (简单) | ⏱️ ~12 分钟 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the test setup by migrating from global to globalThis and enhancing several mocks, including localStorage, Worker, MessageChannel, and matchMedia. The localStorage mock now maintains an internal state, and the matchMedia mock includes additional standard properties and methods. Feedback suggests further improving the MockWorker implementation by using jest.fn() for its methods and implementing a more robust event listener system to better support the EventTarget interface and allow for verification in tests.
| constructor(stringUrl) { | ||
| this.url = stringUrl; | ||
| this.onmessage = () => {}; | ||
| this.onmessage = null; | ||
| } | ||
|
|
||
| postMessage(msg) { | ||
| this.onmessage(msg); | ||
| if (typeof this.onmessage === 'function') { | ||
| this.onmessage({ data: msg }); | ||
| } | ||
| } | ||
|
|
||
| terminate() {} | ||
| addEventListener() {} | ||
| removeEventListener() {} |
There was a problem hiding this comment.
To improve the functionality of the MockWorker and make it consistent with the matchMedia mock, its methods should be jest.fn(). This allows tests to verify that these methods were called. Additionally, postMessage should ideally support listeners added via addEventListener to provide a more complete mock for code that uses the EventTarget interface.
constructor(stringUrl) {
this.url = stringUrl;
this.onmessage = null;
this._listeners = new Set();
}
postMessage = jest.fn((msg) => {
const event = { data: msg };
if (typeof this.onmessage === 'function') {
this.onmessage(event);
}
this._listeners.forEach((listener) => listener(event));
});
terminate = jest.fn();
addEventListener = jest.fn((type, listener) => {
if (type === 'message') this._listeners.add(listener);
});
removeEventListener = jest.fn((type, listener) => {
if (type === 'message') this._listeners.delete(listener);
});![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/setupTests.jsx (1)
33-41:MockWorker为未使用的代码，完整性改进是可选重构。验证显示
MockWorker在整个测试套件中从未被实例化，因此其不完整的事件模型（缺少transfer/ports和addEventListener实现）对测试行为没有实际影响。实际使用的是MessageChannelmock（PolyMessageChannel），其已正确实现port1/port2的消息传递机制，应对src/global.tsx中的 Service Worker 通信场景。若保留
MockWorker用于未来使用，建议补完其事件模型；或如无后续需求，可直接移除以简化 setup 代码。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/setupTests.jsx` around lines 33 - 41, MockWorker is never instantiated in tests and provides an incomplete event model; either remove the MockWorker declaration entirely from setupTests.jsx to simplify setup, or complete its implementation by fully implementing postMessage semantics (including transfer/ports), addEventListener/removeEventListener behavior, and proper terminate handling so it mirrors MessageChannel behavior used by PolyMessageChannel and the Service Worker communication in src/global.tsx; update or remove references accordingly so tests use the existing PolyMessageChannel mock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/setupTests.jsx`:
- Around line 5-17: store is a plain object which can return inherited prototype
properties (so getItem('toString') can be wrong) and the clear implementation
uses a concise arrow that returns the delete expression (which trips Biome); fix
by making store a prototype-less object (e.g., use Object.create(null)) or use a
Map, change localStorageMock.getItem to check ownership (use
Object.prototype.hasOwnProperty.call(store, key) ? store[key] : null), and
rewrite clear to use a block arrow that performs deletion as a statement (e.g.,
Object.keys(store).forEach(key => { delete store[key]; })). Ensure references:
store, localStorageMock.getItem, localStorageMock.clear,
localStorageMock.setItem, localStorageMock.removeItem.
---
Nitpick comments:
In `@tests/setupTests.jsx`:
- Around line 33-41: MockWorker is never instantiated in tests and provides an
incomplete event model; either remove the MockWorker declaration entirely from
setupTests.jsx to simplify setup, or complete its implementation by fully
implementing postMessage semantics (including transfer/ports),
addEventListener/removeEventListener behavior, and proper terminate handling so
it mirrors MessageChannel behavior used by PolyMessageChannel and the Service
Worker communication in src/global.tsx; update or remove references accordingly
so tests use the existing PolyMessageChannel mock.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| const store = {}; | ||
|
|
||
| const localStorageMock = { | ||
| getItem: jest.fn(), | ||
| setItem: jest.fn(), | ||
| removeItem: jest.fn(), | ||
| clear: jest.fn(), | ||
| getItem: jest.fn((key) => store[key] ?? null), | ||
| setItem: jest.fn((key, value) => { | ||
| store[key] = String(value); | ||
| }), | ||
| removeItem: jest.fn((key) => { | ||
| delete store[key]; | ||
| }), | ||
| clear: jest.fn(() => { | ||
| Object.keys(store).forEach((key) => delete store[key]); | ||
| }), |
There was a problem hiding this comment.
localStorage mock 在 Line 8 会误读原型链属性，且 Line 16 会触发 Biome 报错。
store 使用普通对象时，getItem('toString') 这类键会返回继承属性而不是 null，与真实 localStorage 语义不一致；另外 Line 16 的 forEach 回调返回了 delete 结果。
建议修复
-const store = {};
+const store = Object.create(null);
const localStorageMock = {
- getItem: jest.fn((key) => store[key] ?? null),
+ getItem: jest.fn((key) =>
+ Object.prototype.hasOwnProperty.call(store, key) ? store[key] : null,
+ ),
setItem: jest.fn((key, value) => {
- store[key] = String(value);
+ store[String(key)] = String(value);
}),
removeItem: jest.fn((key) => {
- delete store[key];
+ delete store[String(key)];
}),
clear: jest.fn(() => {
- Object.keys(store).forEach((key) => delete store[key]);
+ Object.keys(store).forEach((key) => {
+ delete store[key];
+ });
}),
};🧰 Tools
🪛 Biome (2.4.11)
[error] 16-16: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/setupTests.jsx` around lines 5 - 17, store is a plain object which can
return inherited prototype properties (so getItem('toString') can be wrong) and
the clear implementation uses a concise arrow that returns the delete expression
(which trips Biome); fix by making store a prototype-less object (e.g., use
Object.create(null)) or use a Map, change localStorageMock.getItem to check
ownership (use Object.prototype.hasOwnProperty.call(store, key) ? store[key] :
null), and rewrite clear to use a block arrow that performs deletion as a
statement (e.g., Object.keys(store).forEach(key => { delete store[key]; })).
Ensure references: store, localStorageMock.getItem, localStorageMock.clear,
localStorageMock.setItem, localStorageMock.removeItem.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Failed to clone repository into sandbox. Please try again. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11682 +/- ##
=======================================
Coverage 38.72% 38.72%
=======================================
Files 187 187
Lines 2298 2298
Branches 384 384
=======================================
Hits 890 890
- Misses 1403 1407 +4
+ Partials 5 1 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
2 participants
Refactor localStorage and Worker mocks for better functionality. Update matchMedia polyfill and error handling in console.
Summary by CodeRabbit
发布说明