Fix: Flaky tests.

[ujiro99]

No description provided.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 25.66%. Comparing base (c54c6f1) to head (1fe3e8e).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #354   +/-   ##
=======================================
  Coverage   25.66%   25.66%           
=======================================
  Files         309      309           
  Lines       31240    31240           
  Branches     1546     1546           
=======================================
  Hits         8017     8017           
  Misses      23223    23223           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

E2E テストのフレーク低減を目的に、外部サービス依存のテストを CI でスキップし、固定待機を条件待ちへ置き換える方向の改善を行う PR です。

Changes:

• 外部サービスに依存する E2E を CI では test.skip で実行しないように変更
• Hub 関連のテストで固定 waitForTimeout を expect.poll / waitFor に置換して安定化
• OptionsPage のリロード後待機時間を短縮

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
packages/extension/e2e/url-status.spec.ts	以前は常時 skip だった URL ステータス確認を、CI のみ skip に変更
packages/extension/e2e/pages/OptionsPage.ts	reset 後の固定待機を 500ms→100ms に短縮
packages/extension/e2e/page-action.spec.ts	Amazon 等の外部サイト依存テストを CI で skip
packages/extension/e2e/hub.spec.ts	固定待機を expect.poll と可視化待ちに置換し、フレーク低減

[Copilot]

resetSettings() の末尾が waitForTimeout(100) になっていますが、固定スリープは環境差で再度フレークしやすいです（importSettings() は expect.poll で getCommands の反映を待っています）。

• domcontentloaded 後に「設定が既定値へ反映されたこと」（例: getCommands の内容が変化/安定したこと、または特定UI要素の表示）を条件待ちする形に寄せてください。

[claude]

PR レビュー: Fix: Flaky tests

このPRはフレーキーなE2Eテストの修正を目的としており、アプローチは全体的に正しい方向性です。

良い点

1. expect.poll() の活用 (hub.spec.ts:121-130)

固定の waitForTimeout(500) を削除し、expect.poll() でポーリングに変更したことは、フレーキーテスト修正のベストプラクティスです。

2. waitFor({ state: 'visible' }) の使用 (hub.spec.ts:143-145)

isVisible() の即時評価から waitFor() を使った明示的な待機に変更したことで、要素の表示タイミングへの依存がなくなりました。

3. 外部サービステストのCIスキップ (page-action.spec.ts:257, url-status.spec.ts:12)

process.env.CI を使って外部サービスへのアクセスをCI環境でスキップする対応は適切です。

---

問題点・改善提案

[重要] E2E-90, E2E-91 に残存する waitForTimeout (hub.spec.ts:45, hub.spec.ts:86)

今回修正されたE2E-92と同様に、E2E-90とE2E-91も await page.waitForTimeout(500) を使っており、フレーキーテストの原因になり得ます。expect.poll() に変更することを推奨します。

// 推奨パターン
await downloadButton.click()
await expect.poll(
  async () => {
    const commands = await getCommands()
    return (commands?.length ?? 0) > countBefore
  },
  { timeout: 5000 },
).toBe(true)

[軽微] waitForTimeout(100) の残存 (OptionsPage.ts:241)

500ms から 100ms への短縮は改善ですが、固定待機であることに変わりありません。resetSettings 完了を示すDOMの変化を待機条件にできれば、より安定します。適切な条件がない場合は100msで許容範囲です。

[確認] url-status.spec.ts のスキップ方法の変更 (url-status.spec.ts:12-13)

• 変更前: test.describe.skip() → 常にスキップ
• 変更後: CI環境のみスキップ、ローカルでは外部URLへのリクエストが発生

page-action.spec.ts と同じパターンで統一されており一貫性があります。意図的な変更であれば問題ありません。

---

まとめ

ファイル	評価
hub.spec.ts (E2E-92)	OK: 修正方法が適切
hub.spec.ts (E2E-90, E2E-91)	要確認: waitForTimeout(500) が残存
page-action.spec.ts	OK: 適切なCIスキップ
OptionsPage.ts	OK: 短縮済み
url-status.spec.ts	OK: 意図的な変更なら問題なし

E2E-90 と E2E-91 の waitForTimeout(500) も同様に修正することをお勧めします。

[claude]

コードレビュー

PR #354「Fix: Flaky tests.」のレビューを行いました。フレーキーなE2Eテストの修正として全体的に良い方向性ですが、いくつか指摘事項があります。

---

警告: バグ・潜在的な問題

1. waitFor() の後の isVisible() チェックが機能しない

ファイル: packages/extension/e2e/hub.spec.ts
行: 60–67、105–112、146–152

waitFor({ state: "visible", timeout: 5000 }) は、要素が5秒以内に表示されない場合に 例外をスロー します。そのため、直後の isVisible() チェックが false になることはなく、グレースフルスキップ（test.skip()）が意図通りに機能しません。

元のコードはハブページにインストール可能なコマンドが存在しない場合にテストをスキップする設計でしたが、新しいコードでは waitFor がタイムアウトエラーで失敗するため、スキップではなくテスト失敗になります。

修正案 (hub.spec.ts 60行目付近):

try {
  await downloadButton.waitFor({ state: 'visible', timeout: 5000 })
} catch {
  test.skip(true, 'No installable PageAction commands found on hub page')
  return
}

---

確認: 気になる点

2. tryGetCommandId() の返り値が捨てられている

ファイル: packages/extension/e2e/hub.spec.ts
行: 113–114

2番目のテスト（コマンドのダウンロード確認）では、tryGetCommandId(commandData) の返り値を使用していません。バリデーション目的であれば意図通りですが、元のコードにあった expect(commandId).toBeTruthy() が削除されたことでアサーションが減っています。意図的であれば、コメントで明示すると読みやすくなります。

3. タイムアウトの短縮が別のフレーキーさを生む可能性

ファイル: packages/extension/e2e/pages/OptionsPage.ts
行: 241

waitForTimeout を 500ms → 100ms に短縮しています。domcontentloaded の後にUIの安定を待つ意図があったはずで、100ms では環境によっては不足する可能性があります。固定タイムアウトそのものを使うより、特定のUI要素の状態を待つ waitFor に置き換えられないか検討してください。

---

良い点

• expect.poll() の活用（hub.spec.ts 68–76行目付近）: 固定の waitForTimeout(500) をPollingベースのアサーションに置き換えたのはPlaywrightのベストプラクティスに沿った正しいアプローチです。

• CIスキップの実装（page-action.spec.ts 257行目、url-status.spec.ts 13行目）: 外部サービスへのアクセスを含むテストをCI環境でスキップする対応は適切です。

• ポップアップコマンドへのセレクター改善（hub.spec.ts 146行目付近）: openMode":"popup" による絞り込みで、テスト対象を適切なコマンドタイプに限定できています。

• tryGetCommandId() ヘルパー関数の追加: JSONパースの安全性が向上し、エラーメッセージも明確になりました。

• url-status.spec.ts の .skip 解除: CIスキップ条件付きでローカル実行可能にしたのは良い改善です。

---

まとめ

フレーキーなテストへの対処として概ね適切な修正ですが、waitFor() + isVisible() のパターンはグレースフルスキップを壊している点が主要な問題です。修正をご検討ください。

このレビューは Claude (claude-sonnet-4-6) によって自動生成されました。