chore: Migrate unit tests from mocha to vitest#478
Open
yuiseki wants to merge 4 commits intogeolonia:masterfrom
Open
chore: Migrate unit tests from mocha to vitest#478yuiseki wants to merge 4 commits intogeolonia:masterfrom
yuiseki wants to merge 4 commits intogeolonia:masterfrom
Conversation
- Replace mocha + ts-node + jsdom-global with vitest (jsdom environment) - Convert all assert.* to vitest expect() - Convert mocha before/after to vitest beforeAll/afterAll - Add vitest.config.ts with jsdom environment - Add tsconfig.build.json to exclude test files from webpack build - Update webpack.config.js to use tsconfig.build.json - Remove .mocharc.js - All 50 tests pass on Node 22
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMochaの設定ファイルを削除してVitestへ移行しました。npmの Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Poem
🚥 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 |
yuiseki
requested review from
bougan1160,
madmaid,
naogify,
niryuu,
sanak and
smellman
yuiseki
added
Priority: Middle
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/util.test.ts (1)
330-360:⚠️ Potential issue | 🔴 CriticalVitest は
doneコールバックをサポートしていません。これが CI 失敗の原因です。Line 330 と Line 350 の
it(..., (done) => ...)パターンは Vitest では完全にサポートされていない（単なる非推奨ではなく、未実装）ため、テストはdone()の呼び出しを待たずに終了し、その後の Promise 拒否が未処理となります。async/awaitまたは Promise return で置き換えてください。修正案（async/await へ移行）
- it('should call the callback with response data when the promise resolves', (done) => { + it('should call the callback with response data when the promise resolves', async () => { // モックされた成功した promise const mockResponse = { data: new Image(), cacheControl: 'public, max-age=3600', expires: '1609459200', }; const promise = Promise.resolve(mockResponse); - loadImageCompatibility(promise, (error, data, expiry) => { - expect(error).toEqual(null); - expect(data).toEqual(mockResponse.data); - expect(expiry).toEqual({ - cacheControl: mockResponse.cacheControl, - expires: mockResponse.expires, - }); - done(); - }); + await new Promise<void>((resolve, reject) => { + loadImageCompatibility(promise, (error, data, expiry) => { + try { + expect(error).toEqual(null); + expect(data).toEqual(mockResponse.data); + expect(expiry).toEqual({ + cacheControl: mockResponse.cacheControl, + expires: mockResponse.expires, + }); + resolve(); + } catch (e) { + reject(e); + } + }); + }); }); - it('should call the callback with error when the promise rejects', (done) => { + it('should call the callback with error when the promise rejects', async () => { // モックされた失敗した promise const mockError = new Error('Failed to load image'); const promise = Promise.reject(mockError); - loadImageCompatibility(promise, (error, data, expiry) => { - expect(error).toEqual(mockError); - expect(data).toBe(undefined); - expect(expiry).toBe(undefined); - done(); - }); + await new Promise<void>((resolve, reject) => { + loadImageCompatibility(promise, (error, data, expiry) => { + try { + expect(error).toEqual(mockError); + expect(data).toBe(undefined); + expect(expiry).toBe(undefined); + resolve(); + } catch (e) { + reject(e); + } + }); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/util.test.ts` around lines 330 - 360, The tests use the unsupported done callback causing CI failures; update both specs that call loadImageCompatibility to return/await the promise instead of using done: rewrite the first test to await loadImageCompatibility(promise) or return the Promise and then assert the resolved values (error null, data equals mockResponse.data, expiry object matches), and rewrite the rejection test to await/expect the promise to reject or return the rejected Promise and assert the error and that data/expiry are undefined; reference the test cases around loadImageCompatibility and the mock promises used in the two it(...) blocks.src/lib/simplestyle.test.ts (1)
159-185:⚠️ Potential issue | 🟠 Major外部URL依存のユニットテストは不安定です。
Line 159 と Line 218 の実ネットワーク取得に依存しており、CI の証明書状態やネットワークで揺れます（今回の pipeline warning と整合）。この2ケースは
window.fetchをスタブして固定レスポンスにしてください。差分案（固定レスポンス化）
+const sampleRemoteGeoJSON = { + type: 'FeatureCollection', + features: [ + { + type: 'Feature', + properties: {}, + geometry: { + type: 'LineString', + coordinates: [ + [139.6870422363281, 35.73425097869431], + [139.76943969726562, 35.73425097869431], + [139.73922729492188, 35.66399091134812], + [139.70352172851562, 35.698571062054015], + ], + }, + }, + ], +}; + it('should load GeoJSON from url', async () => { const { SimpleStyle } = await import('./simplestyle'); const map = new Map(); - const geojson = - 'https://gist.githubusercontent.com/miya0001/56c3dc174f5cdf1d9565cbca0fbd3c48/raw/c13330036d28ef547a8a87cb6df3fa12de19ddb6/test.geojson'; + const geojson = 'https://example.com/test.geojson'; + window.fetch = async () => + new Response(JSON.stringify(sampleRemoteGeoJSON), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); const ss = new SimpleStyle(geojson); ss.addTo(map).fitBounds(); @@ it('should update GeoJSON from url', async () => { const { SimpleStyle } = await import('./simplestyle'); @@ - const geojson = - 'https://gist.githubusercontent.com/miya0001/56c3dc174f5cdf1d9565cbca0fbd3c48/raw/c13330036d28ef547a8a87cb6df3fa12de19ddb6/test.geojson'; + const geojson = 'https://example.com/test.geojson'; + window.fetch = async () => + new Response(JSON.stringify(sampleRemoteGeoJSON), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); ss.updateData(geojson);Also applies to: 204-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/simplestyle.test.ts` around lines 159 - 185, The test relies on real network fetches for the GeoJSON URL which makes CI flaky; stub window.fetch in the test to return a fixed successful Response containing the expected GeoJSON payload before importing SimpleStyle, then restore the original fetch after the test. Specifically, in the 'should load GeoJSON from url' case (and the similar case around lines 204-240) set window.fetch to a jest.fn() or equivalent that resolves to an object with ok=true and a json() that returns the expected FeatureCollection used to assert map.sources['geolonia-simple-style'].data.features[0].geometry (so _loadingPromise, SimpleStyle(geojson), ss.addTo(map).fitBounds() behave deterministically), and ensure teardown restores the real window.fetch.
🧹 Nitpick comments (1)
src/lib/simplestyle.test.ts (1)
7-13: グローバルの上書きはテスト後に復元してください。
window.requestAnimationFrameを上書きしたままなので、同ファイル内で将来テスト追加時に副作用が出ます。afterAllで元に戻すのが安全です。差分案
-import { describe, it, expect, beforeAll } from 'vitest'; +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { random } from './util'; +const originalRequestAnimationFrame = window.requestAnimationFrame; + beforeAll(() => { window.URL.createObjectURL ||= (_: Blob | MediaSource) => 'dummy'; window.requestAnimationFrame = (cb) => { cb(performance.now()); return random(999999); }; }); + +afterAll(() => { + window.requestAnimationFrame = originalRequestAnimationFrame; +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/simplestyle.test.ts` around lines 7 - 13, The test overwrites globals in beforeAll (window.requestAnimationFrame and window.URL.createObjectURL) but never restores them; add an afterAll that saves the originals before overriding (capture origRequestAnimationFrame and origCreateObjectURL) and restores window.requestAnimationFrame and window.URL.createObjectURL in afterAll to avoid cross-test side effects, keeping the existing beforeAll behavior (random stub and dummy URL) but ensuring cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/keyring.test.ts`:
- Around line 109-112: The test suite currently only covers the negative branch
for relative .json paths; add a positive test that sets/mocks location.href to a
geolonia.com origin and asserts keyring.isGeoloniaStyleCheck('./my-style.json')
returns true to exercise the new URL(style, location.href) branch in keyring.ts.
Specifically, in keyring.test.ts add one test that temporarily stubs
global/location.href (or uses jsdom's location) to a geolonia.com URL, calls
isGeoloniaStyleCheck with a relative .json path, and restores location
afterwards so the code path that resolves relative URLs against location.href
(new URL(style, location.href)) is covered.
In `@src/lib/util.test.ts`:
- Line 1: Remove the unused Vitest hook imports to clean up the test file: in
src/lib/util.test.ts remove beforeEach, afterEach, and afterAll from the
destructured import from 'vitest' (leave only describe, it, expect, and
beforeAll if used) so the import line only includes hooks/identifiers actually
referenced in the file.
---
Outside diff comments:
In `@src/lib/simplestyle.test.ts`:
- Around line 159-185: The test relies on real network fetches for the GeoJSON
URL which makes CI flaky; stub window.fetch in the test to return a fixed
successful Response containing the expected GeoJSON payload before importing
SimpleStyle, then restore the original fetch after the test. Specifically, in
the 'should load GeoJSON from url' case (and the similar case around lines
204-240) set window.fetch to a jest.fn() or equivalent that resolves to an
object with ok=true and a json() that returns the expected FeatureCollection
used to assert map.sources['geolonia-simple-style'].data.features[0].geometry
(so _loadingPromise, SimpleStyle(geojson), ss.addTo(map).fitBounds() behave
deterministically), and ensure teardown restores the real window.fetch.
In `@src/lib/util.test.ts`:
- Around line 330-360: The tests use the unsupported done callback causing CI
failures; update both specs that call loadImageCompatibility to return/await the
promise instead of using done: rewrite the first test to await
loadImageCompatibility(promise) or return the Promise and then assert the
resolved values (error null, data equals mockResponse.data, expiry object
matches), and rewrite the rejection test to await/expect the promise to reject
or return the rejected Promise and assert the error and that data/expiry are
undefined; reference the test cases around loadImageCompatibility and the mock
promises used in the two it(...) blocks.
---
Nitpick comments:
In `@src/lib/simplestyle.test.ts`:
- Around line 7-13: The test overwrites globals in beforeAll
(window.requestAnimationFrame and window.URL.createObjectURL) but never restores
them; add an afterAll that saves the originals before overriding (capture
origRequestAnimationFrame and origCreateObjectURL) and restores
window.requestAnimationFrame and window.URL.createObjectURL in afterAll to avoid
cross-test side effects, keeping the existing beforeAll behavior (random stub
and dummy URL) but ensuring cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9948a9de-1ae6-4d28-9121-1a7291251839
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
.mocharc.jspackage.jsonsrc/lib/keyring.test.tssrc/lib/parse-atts.test.tssrc/lib/simplestyle.test.tssrc/lib/util.test.tssrc/testability.test.tstsconfig.build.jsontsconfig.jsonvitest.config.tswebpack.config.js
💤 Files with no reviewable changes (1)
- .mocharc.js
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/util.test.ts (1)
1-1:⚠️ Potential issue | 🟡 Minor未使用の Vitest フック import を削除してください。
Line 1 で
beforeEach・afterEach・afterAllは未使用で、lint warning の原因になっています。beforeAllのみ残す構成にしてください。修正案
-import { describe, it, expect, beforeEach, afterEach, beforeAll, afterAll } from 'vitest'; +import { describe, it, expect, beforeAll } from 'vitest';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/util.test.ts` at line 1, Remove the unused Vitest hook imports to silence the lint warnings: update the import statement that currently imports beforeEach, afterEach, afterAll, beforeAll to only import beforeAll (retain describe, it, expect), i.e. remove references to beforeEach, afterEach, and afterAll from the import list so only beforeAll remains alongside describe/it/expect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/util.test.ts`:
- Line 1: Remove the unused Vitest hook imports to silence the lint warnings:
update the import statement that currently imports beforeEach, afterEach,
afterAll, beforeAll to only import beforeAll (retain describe, it, expect), i.e.
remove references to beforeEach, afterEach, and afterAll from the import list so
only beforeAll remains alongside describe/it/expect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73d2a819-ffce-4e1c-8bbb-f5eedd1c1146
📒 Files selected for processing (2)
src/lib/keyring.test.tssrc/lib/util.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/keyring.test.ts
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
1 participant
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
mocha + ts-node + jsdom-global → vitest に移行。テストの内容は変更せず、テストランナーとアサーション API のみ変更。
Changes
mocha,ts-node,@types/mocha,jsdom-globalを削除、vitestを追加.mocharc.jsを削除、vitest.config.tsを追加（jsdom environment）assert.*→ vitest のexpect()before/after→ vitest のbeforeAll/afterAlltsconfig.build.jsonを追加（webpack ビルドからテストファイルを除外）webpack.config.jsでtsconfig.build.jsonを参照Motivation
@geolonia/maps-core(ESM) 依存にも対応可能Test plan
npm run lint— 0 errorsnpm run test— 5 files, 50 tests passednpm run build— compiled (warnings only)Summary by CodeRabbit