Open
Conversation
管理画面の主要機能をカバーする25件のE2Eテストを追加。 CI（GitHub Actions）にPlaywright E2Eジョブを追加し、lint後に自動実行されるようにした。 - ナビゲーション（サイドバーメニュー） - 設定画面（閾値変更、通知先設定、バリデーション） - テストメール送信（MailHog連携） - メールテンプレート編集 - コマンド実行（アラート送信、重複防止、在庫回復後の再アラート） - 送信履歴画面 - 異常系（CSRF、エラーメッセージ） また、テストメールのダミー商品にIDを設定し、メールテンプレートで 商品名ヘッダーが正しく出力されるよう修正。 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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)
📝 WalkthroughWalkthroughE2Eテスト基盤が新規追加されました。Playwright設定、MailHogヘルパー、複数のテストスイート（ナビゲーション、設定画面、コマンド実行、テストメール送信など）を統備。GitHub Actionsワークフロー（e2e実行、lint実行）も新規構築。PHP側ではダミーデータのID設定機能を改良しています。 Changes
Sequence Diagram(s)sequenceDiagram
participant Test as E2Eテスト
participant Exec as execStockAlert()
participant PHP as PHPコンソール
participant DB as Database
participant Mail as MailHog API
participant Assert as 検証
Test->>Exec: コマンド実行要求
Exec->>PHP: bin/console実行
PHP->>DB: 在庫チェック<br/>対象商品抽出
PHP->>Mail: アラートメール送信
Mail->>Mail: メッセージ保存
Exec-->>Test: 終了コード・出力返却
Test->>Mail: getMessages()
Mail-->>Test: MailHogメッセージ一覧
Test->>Assert: 件数・内容検証
sequenceDiagram
participant Test as E2Eテスト
participant Browser as Playwright<br/>Browser
participant App as EC-CUBEアプリ
participant Form as フォーム
participant Mail as MailHog API
Test->>Browser: ページ遷移
Browser->>App: GET /admin/plugin/<br/>stock-alert/config
App-->>Browser: 設定画面HTML
Test->>Form: フォーム入力<br/>(threshold, emails等)
Form->>App: POST フォーム送信
App-->>Form: リダイレクト
Note over Browser: フラッシュメッセージ表示確認
Test->>Browser: sendTestMail()実行
Browser->>App: テストメール<br/>送信リクエスト
App->>Mail: アラートメール送信
Mail-->>Browser: 成功レスポンス
Test->>Mail: waitForMessage()
Mail-->>Test: 受信メッセージ検証
Estimated code review effort🎯 4 (Complex) | ⏱️ ~80 minutes 多数のテストファイルと環境設定がありますね。各テストスイートのロジックは個別に確認が必要で、ヘルパーモジュール間の連携、ワークフロー設定との整合性も見ておく必要があると思います。E2E基盤全体としてはまとまった変更量かもしれません。 Poem
…大規模なE2E基盤の追加ですね。テスト構成は論理的にまとまっているようなので、素敵だと思います。ただ、ワークフロー間の依存関係、特に既存のtest.ymlからlintを分離した部分が期待通り動作するかどうか…少し気になるところがあります。各テストスイートも個別に見ると自然な流れなのですが、全体として一貫性があるか、確認したほうがいいかもしれません。 🚥 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 |
![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
🧹 Nitpick comments (16)
e2e/helpers/pages.ts (2)
35-36: ダイアログリスナーの登録をonceに変更することを推奨します。
page.on('dialog', ...)はリスナーを永続的に登録します。同一ページコンテキストでsendTestMailを複数回呼び出すとリスナーが蓄積する可能性があります。page.once('dialog', ...)を使用することで、一度だけ処理されるようになります。♻️ 提案される修正
// confirm ダイアログを自動承認 - page.on('dialog', dialog => dialog.accept()); + page.once('dialog', dialog => dialog.accept());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/pages.ts` around lines 35 - 36, Replace the persistent dialog listener registration page.on('dialog', dialog => dialog.accept()) with a one-time listener so the handler is removed after it fires (use page.once('dialog', ...) instead); update the place where sendTestMail triggers dialogs to register the one-shot listener before the action to prevent listener accumulation across multiple sendTestMail calls and ensure each dialog is handled exactly once.
38-38: セレクタをより具体的にすることを検討してください。
button.btn-outline-primaryは汎用的なセレクタです。UIの変更でテストが不安定になる可能性があります。可能であれば、data-testid属性の追加やより具体的なセレクタの使用を検討してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/pages.ts` at line 38, The test uses a fragile generic selector page.locator('button.btn-outline-primary').click() — replace it with a stable data-testid-based selector: add a specific data-testid attribute to the target component (e.g., data-testid="confirm-button") and update the test to locate that element (use page.getByTestId or page.locator('[data-testid="..."]') and call click()); ensure the test and the UI component that renders the button (the component/class that currently renders button.btn-outline-primary) are both updated so the test targets the new data-testid.e2e/helpers/mailhog.ts (1)
23-32: fetch呼び出しにエラーハンドリングの追加を検討してください。
deleteAllMessagesとgetMessagesはHTTPエラー（4xx/5xx）やネットワークエラーをチェックしていません。MailHogが利用不可の場合、エラーメッセージが不明確になる可能性があります。♻️ 提案される修正
/** 全メールを削除する */ export async function deleteAllMessages(): Promise<void> { - await fetch(`${MAILHOG_API}/v1/messages`, { method: 'DELETE' }); + const res = await fetch(`${MAILHOG_API}/v1/messages`, { method: 'DELETE' }); + if (!res.ok) { + throw new Error(`MailHog API error: ${res.status} ${res.statusText}`); + } } /** 全メールを取得する（新しい順） */ export async function getMessages(): Promise<MailhogMessage[]> { const res = await fetch(`${MAILHOG_API}/v2/messages`); + if (!res.ok) { + throw new Error(`MailHog API error: ${res.status} ${res.statusText}`); + } const data: MailhogResponse = await res.json(); return data.items; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/mailhog.ts` around lines 23 - 32, The two helper functions deleteAllMessages and getMessages lack error handling for network and HTTP errors; update both to wrap their fetch calls in try/catch, check response.ok after each fetch, and throw or reject with a descriptive error that includes the HTTP status and response text (or underlying network error) so callers get clear diagnostics when MailHog is unavailable or returns 4xx/5xx responses. Ensure getMessages still parses JSON only after verifying response.ok and that deleteAllMessages reports failure when the DELETE does not succeed.e2e/tests/global-setup.ts (1)
5-6: 認証情報を環境変数から読み込むことを検討してください。ハードコードされた認証情報（
admin/password）はローカル・CI環境では一般的ですが、環境変数から読み込むようにすると柔軟性が向上します。♻️ 提案される修正
+const ADMIN_ID = process.env.ECCUBE_ADMIN_ID || 'admin'; +const ADMIN_PASS = process.env.ECCUBE_ADMIN_PASS || 'password'; + setup('管理画面にログインしてセッションを保存', async ({ page }) => { await page.goto('/admin/login'); - await page.locator('#login_id').fill('admin'); - await page.locator('#password').fill('password'); + await page.locator('#login_id').fill(ADMIN_ID); + await page.locator('#password').fill(ADMIN_PASS); await page.locator('button[type="submit"]').click();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/global-setup.ts` around lines 5 - 6, Replace the hardcoded credentials used with page.locator('#login_id').fill('admin') and page.locator('#password').fill('password') so they read from environment variables (e.g., process.env.E2E_LOGIN and process.env.E2E_PASSWORD); update the global setup logic in e2e/tests/global-setup.ts to pull those vars, provide sensible fallback or throw a clear error when missing, and use the retrieved values when filling the '#login_id' and '#password' locators.e2e/tests/log.spec.ts (1)
42-58: テストの前提条件について確認してください。TC-6.3は
execStockAlert()を実行しますが、TC-6.2も同様に実行します。テストの実行順序によっては、ログの状態が期待と異なる可能性があります。テスト間の独立性を高めるため、各テストで明示的にデータをセットアップすることを検討してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/log.spec.ts` around lines 42 - 58, The test TC-6.3 depends on log state possibly set by TC-6.2 (execStockAlert()), so make the test self-contained by explicitly creating the expected stock-alert log and ensuring the page state before asserting the product link; call or implement a deterministic setup helper (e.g., ensureStockAlert or awaitable execStockAlert) and a teardown/reset helper (e.g., clearLogs) so you can call clearLogs(); await execStockAlert() (or call the new ensureStockAlert API) and wait for the log to appear via gotoLog(page) before querying productLink (the locator 'table a[href*="admin/product/product"]'). If the link is truly absent then fail the test rather than silently skip; update TC-6.3 to use setThreshold(page, 9999), await execStockAlert(), await gotoLog(page), then assert productLink.isVisible() and click it to expect the /admin/product/product/:id/edit URL.e2e/tests/config.spec.ts (2)
44-45:.catch(() => false)は不要です。
isVisible()は例外をスローしないため、.catch()は冗長です。♻️ 修正案
- const hasSuccess = await page.locator('.alert-success').isVisible().catch(() => false); + const hasSuccess = await page.locator('.alert-success').isVisible();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/config.spec.ts` around lines 44 - 45, Remove the unnecessary .catch(() => false) from the isVisible call: change the assignment to use await page.locator('.alert-success').isVisible() directly (the hasSuccess variable and the expect(hasSuccess).toBeFalsy() assertion remain unchanged), since isVisible() does not throw and the .catch is redundant.
65-66: 親要素へのロケーター参照方法をより明示的にしてください。
locator('..')は Playwright で暗黙的に XPath の親軸セレクターとして機能しますが、意図をより明確にするためにlocator('xpath=..')と明示的に指定することをお勧めします。これにより、他の開発者がコードを読む際に XPath セレクターを使用していることがすぐに理解できるようになります。♻️ より明確なロケーターの提案
- const helpText = page.locator('#stock_alert_config_alertEmails') - .locator('..').locator('.form-text'); + const helpText = page.locator('#stock_alert_config_alertEmails') + .locator('xpath=..').locator('.form-text');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/config.spec.ts` around lines 65 - 66, Change the implicit parent locator usage to an explicit XPath expression for clarity: update the helpText locator that chains page.locator('#stock_alert_config_alertEmails').locator('..').locator('.form-text') so the parent reference uses locator('xpath=..') (i.e., locate the parent via XPath) before chaining .locator('.form-text'); keep the same variable name helpText and chaining behavior but replace the implicit '..' with 'xpath=..'.e2e/tests/error-cases.spec.ts (2)
13-14:.catch(() => false)は不要です。Playwright の
isVisible()は要素が存在しない場合でも例外をスローせずfalseを返します。.catch()は冗長であり、削除できます。♻️ 修正案
- const hasSuccess = await page.locator('.alert-success').isVisible().catch(() => false); - const hasError = await page.locator('.alert-danger').isVisible().catch(() => false); + const hasSuccess = await page.locator('.alert-success').isVisible(); + const hasError = await page.locator('.alert-danger').isVisible();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/error-cases.spec.ts` around lines 13 - 14, Remove the redundant .catch(() => false) wrappers around the Playwright visibility checks: replace the uses of page.locator('.alert-success').isVisible().catch(() => false) and page.locator('.alert-danger').isVisible().catch(() => false) with simple awaits of page.locator(...).isVisible() (i.e., assign hasSuccess = await page.locator('.alert-success').isVisible() and hasError = await page.locator('.alert-danger').isVisible()), since isVisible() already returns false for non-existent elements.
28-36: CSRF テストのアサーションが複雑で意図が不明瞭です。現在のアサーションは、ステータスコードが
[301, 302, 403, 422]のいずれか または URL に/configが含まれる場合に成功します。コンテキストスニペットによると、コントローラーは CSRF 失敗時に 302 リダイレクトを返すため、よりシンプルなアサーションで十分です。♻️ より明確なアサーションの提案
test('CSRF トークンなしのテストメール送信は拒否される', async ({ page }) => { // 直接 POST（CSRF トークンなし） - const response = await page.request.post('/admin/plugin/stock-alert/config/send-test'); + const response = await page.request.post('/admin/plugin/stock-alert/config/send-test', { + maxRedirects: 0, + }); // リダイレクト or エラーになること（200 で処理が通らない） - expect([301, 302, 403, 422].some(code => - response.status() === code || response.url().includes('/config'), - )).toBeTruthy(); + // CSRF エラー時は 302 リダイレクトが返される + expect(response.status()).toBe(302); });注:
maxRedirects: 0を使用しない場合は、リダイレクト後の URL を確認する方法も有効です：expect(response.url()).toContain('/config'); expect(response.status()).toBe(200); // リダイレクト後のステータス🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/error-cases.spec.ts` around lines 28 - 36, The current complex assertion in the test "CSRF トークンなしのテストメール送信は拒否される" makes intent unclear; replace the multi-code/URL OR check with a single clear assertion on the response produced by the POST (the response variable) — since the controller returns a 302 on CSRF failure, assert response.status() === 302 (or alternatively assert response.url().includes('/config') if you prefer checking the redirected location); remove the array-of-codes logic and the combined boolean expression to make the test deterministic and readable.e2e/tests/test-mail.spec.ts (1)
43-51: 「メールが送信されない」ことの検証パターンを改善してください。現在の実装には以下の問題があります：
waitForTimeout(2000)は固定遅延であり、環境依存で不安定になりやすいexpect(true).toBe(false)は失敗時のメッセージが不明瞭♻️ より明確な検証パターンの提案
// 少し待ってもメールが届かないことを確認 - await page.waitForTimeout(2000); - try { - await waitForMessage(() => true, 1000); - // ここに到達したらメールが届いてしまっている - expect(true).toBe(false); - } catch { - // メールが届かない = 期待通り - } + // 短いタイムアウトでメッセージが届かないことを確認 + const messages = await getMessages(); + expect(messages).toHaveLength(0);注:
getMessages()をdeleteAllMessages()の後に呼び出しているため、メッセージが 0 件であることを直接確認できます。ダイアログをキャンセルした直後なので、長い待機は不要です。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/test-mail.spec.ts` around lines 43 - 51, Replace the brittle fixed-wait + try/catch pattern (waitForTimeout, waitForMessage, expect(true).toBe(false)) with a direct assertion that the mailbox is empty after the dialog cancellation: after calling deleteAllMessages(), call getMessages() and assert its length is 0 with a clear expectation message; remove the unnecessary page.waitForTimeout(2000) and the try/catch around waitForMessage() so the test deterministically checks getMessages() === 0 instead of relying on timeouts or an inverted expect.e2e/tests/command.spec.ts (2)
47-50:setTimeoutによるハードコード遅延はテストを不安定にします。
new Promise(r => setTimeout(r, 2000))は固定遅延であり、環境によってはタイミング問題が発生する可能性があります。MailHog のメッセージが「ない」ことを確認するには、短いタイムアウトでgetMessages()を呼び出し、空であることをアサートする方が信頼性が高いです。♻️ 改善案
// メールが送信されていないことを確認 - await new Promise(r => setTimeout(r, 2000)); - const messages = await getMessages(); - expect(messages.length).toBe(0); + // 短いポーリング間隔でメッセージがないことを確認 + const checkNoMessages = async (retries = 3, delay = 500) => { + for (let i = 0; i < retries; i++) { + const messages = await getMessages(); + if (messages.length > 0) { + throw new Error(`Unexpected messages found: ${messages.length}`); + } + await new Promise(r => setTimeout(r, delay)); + } + }; + await checkNoMessages();または、
waitForMessageのような既存ヘルパーに「メッセージがないことを確認する」バリアントを追加することも検討してください。Also applies to: 67-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/command.spec.ts` around lines 47 - 50, The test uses a hardcoded delay (new Promise(... setTimeout 2000)) which makes it flaky; replace that sleep with a direct, short-polling check that calls getMessages() (or a small retry loop with a short overall timeout) and assert that the returned array is empty, or add/use a helper variant like waitForMessageAbsent/getMessagesEmpty to encapsulate this check; apply the same fix to the other occurrence around the 67-70 block so both assertions verify emptiness via getMessages() (or the new helper) instead of sleeping.
97-103:test.afterAllがtest.describeブロックの外にあります。
test.afterAllがtest.describe('コマンド実行')の外に配置されているため、このファイル内のすべてのテストに適用されます。意図通りであれば問題ありませんが、スコープを明確にするためにtest.describe内に移動することを推奨します。♻️ スコープを明確にする修正案
test.describe('コマンド実行', () => { test.beforeEach(async () => { await deleteAllMessages(); }); + test.afterAll(async ({ browser }) => { + // 閾値をデフォルトに戻す + const context = await browser.newContext({ storageState: './auth.json' }); + const page = await context.newPage(); + await setThreshold(page, 5); + await context.close(); + }); + test('TC-5.1: アラート対象商品ありでメール送信される', async ({ page }) => { // ... }); // ... }); - -test.afterAll(async ({ browser }) => { - // 閾値をデフォルトに戻す - const context = await browser.newContext({ storageState: './auth.json' }); - const page = await context.newPage(); - await setThreshold(page, 5); - await context.close(); -});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/command.spec.ts` around lines 97 - 103, test.afterAll が test.describe('コマンド実行') ブロックの外にありファイル全体に適用されているため、意図がコマンド実行テストだけに適用するなら test.describe 内に移動してください；具体的には現在の test.afterAll ブロック（browser を受け取り setThreshold(page, 5) を呼ぶ処理）を test.describe('コマンド実行') の末尾に移してスコープを限定し、setThreshold 呼び出しと context/newPage/close の処理がその describe 内の全テスト後にのみ実行されるようにしてください。.github/workflows/ci.yml (2)
197-201: MailHog サービスにヘルスチェックがありません。MySQL サービスと異なり、MailHog にはヘルスチェックが設定されていません。テストの安定性向上のため、
optionsでヘルスチェックを追加することを検討してください。♻️ MailHog ヘルスチェックの追加案
mailhog: image: mailhog/mailhog ports: - 1025:1025 - 8025:8025 + options: >- + --health-cmd="wget -q --spider http://localhost:8025/api/v2/messages || exit 1" + --health-interval=10s + --health-timeout=5s + --health-retries=3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 197 - 201, The MailHog service block (mailhog) lacks a container healthcheck; add an options.healthcheck entry for the mailhog service to improve test stability by probing the web UI or SMTP port (e.g., a command like curl -f http://localhost:8025/ || exit 1), and configure sensible interval, timeout and retries (for example interval: 10s, timeout: 5s, retries: 5) so CI waits for MailHog readiness before running tests.
261-264: PHP サーバーの起動確認が不十分な可能性があります。
sleep 3と単一のcurlチェックでは、サーバー起動のタイミング問題でテストが不安定になる可能性があります。リトライ付きのヘルスチェックを推奨します。♻️ リトライ付きヘルスチェックの提案
run: | php -S 127.0.0.1:8000 -t . & - sleep 3 - curl -sf http://127.0.0.1:8000/admin/login > /dev/null + for i in {1..30}; do + curl -sf http://127.0.0.1:8000/admin/login > /dev/null && break + echo "Waiting for server... ($i)" + sleep 1 + done + curl -sf http://127.0.0.1:8000/admin/login > /dev/null || exit 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 261 - 264, Replace the fragile "sleep 3" + single "curl -sf http://127.0.0.1:8000/admin/login" check with a retrying health-check loop: after launching the PHP built-in server (php -S 127.0.0.1:8000 -t . &), poll the admin URL with curl in a loop (e.g., up to N attempts with a short sleep between attempts), exit successfully when curl -sf returns 0 and exit with non‑zero if the max attempts are reached; update the run step that currently uses "sleep 3" and the single "curl" invocation to use this retry loop to ensure reliable startup before proceeding.e2e/playwright.config.ts (1)
8-14:trace: 'on-first-retry'はretries: 0では無効です。
retries: 0が設定されているため、リトライが発生せずtrace: 'on-first-retry'によるトレース収集は行われません。デバッグ用にトレースを取得したい場合は、trace: 'on'またはtrace: 'retain-on-failure'に変更することを検討してください。♻️ トレース設定の修正案
use: { baseURL: process.env.ECCUBE_URL || 'http://127.0.0.1:8000', screenshot: 'only-on-failure', - trace: 'on-first-retry', + trace: 'retain-on-failure', },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/playwright.config.ts` around lines 8 - 14, The current Playwright config sets retries: 0 so trace: 'on-first-retry' never triggers; update the trace setting inside the use object in e2e/playwright.config.ts (the trace property in the use block) to a mode that captures traces without retries (e.g., change trace to 'on' or 'retain-on-failure'), or alternatively increase retries from 0 if you want to keep 'on-first-retry' behavior; modify the trace value accordingly to ensure traces are actually collected during test runs.e2e/tests/mail-template.spec.ts (1)
54-59:waitForTimeoutの使用はテストを不安定にする可能性があります。Playwright のドキュメントでは
waitForTimeoutの使用は推奨されていません。代わりに、バリデーションエラーメッセージの表示を待つか、フォームの状態変化を検出するアサーションを使用してください。♻️ 改善案
// 送信ボタンをクリック（HTML5 required バリデーションで送信されない場合もある） await page.locator('button.btn-ec-conversion[type="submit"]').click(); // 成功メッセージが表示されないことを確認 - await page.waitForTimeout(1000); - await expect(page.locator('.alert-success')).not.toBeVisible(); + // バリデーションエラーまたはページの状態が安定するまで待機 + await expect(page.locator('.alert-success')).not.toBeVisible({ timeout: 3000 });注:
not.toBeVisible()にtimeoutオプションを指定すると、指定時間内に要素が表示されないことを確認できます。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/mail-template.spec.ts` around lines 54 - 59, Replace the brittle waitForTimeout call with a deterministic assertion: after clicking the submit button (page.locator('button.btn-ec-conversion[type="submit"]').click()), wait for the specific validation/error element to appear or for the success alert to remain absent using Playwright's expect assertions instead of sleep. Concretely, remove waitForTimeout and either assert the validation message locator becomes visible (e.g., wait for your form validation selector) or call expect(page.locator('.alert-success')).not.toBeVisible with an explicit timeout option to assert the alert does not appear within the given time; ensure you reference the same selectors used in this test (the submit button locator and '.alert-success') when making the replacement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/tests/error-cases.spec.ts`:
- Line 6: Two tests share the same ID string "TC-7.2" which will cause duplicate
test identifiers; locate the test call using the title 'TC-7.2:
テストメール送信失敗時にエラーメッセージが表示される（SMTP障害）' and the other test that reuses "TC-7.2" and
rename one to a unique ID (e.g., change one to "TC-7.3" or "TC-7.2a"), and
update any assertions/comments or external references that expect the old ID so
the test reports have distinct identifiers.
In `@e2e/tests/navigation.spec.ts`:
- Line 4: Multiple tests in navigation.spec.ts use the same ID "TC-1.1"; update
each test title string in the three test(...) declarations (e.g., the test
starting with "TC-1.1: サイドバーに在庫アラートメニューが表示される") so each has a unique test ID
like "TC-1.1", "TC-1.2", "TC-1.3" while keeping the rest of the description
intact; ensure you change all occurrences of the duplicated "TC-1.1" test titles
in this file.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 197-201: The MailHog service block (mailhog) lacks a container
healthcheck; add an options.healthcheck entry for the mailhog service to improve
test stability by probing the web UI or SMTP port (e.g., a command like curl -f
http://localhost:8025/ || exit 1), and configure sensible interval, timeout and
retries (for example interval: 10s, timeout: 5s, retries: 5) so CI waits for
MailHog readiness before running tests.
- Around line 261-264: Replace the fragile "sleep 3" + single "curl -sf
http://127.0.0.1:8000/admin/login" check with a retrying health-check loop:
after launching the PHP built-in server (php -S 127.0.0.1:8000 -t . &), poll the
admin URL with curl in a loop (e.g., up to N attempts with a short sleep between
attempts), exit successfully when curl -sf returns 0 and exit with non‑zero if
the max attempts are reached; update the run step that currently uses "sleep 3"
and the single "curl" invocation to use this retry loop to ensure reliable
startup before proceeding.
In `@e2e/helpers/mailhog.ts`:
- Around line 23-32: The two helper functions deleteAllMessages and getMessages
lack error handling for network and HTTP errors; update both to wrap their fetch
calls in try/catch, check response.ok after each fetch, and throw or reject with
a descriptive error that includes the HTTP status and response text (or
underlying network error) so callers get clear diagnostics when MailHog is
unavailable or returns 4xx/5xx responses. Ensure getMessages still parses JSON
only after verifying response.ok and that deleteAllMessages reports failure when
the DELETE does not succeed.
In `@e2e/helpers/pages.ts`:
- Around line 35-36: Replace the persistent dialog listener registration
page.on('dialog', dialog => dialog.accept()) with a one-time listener so the
handler is removed after it fires (use page.once('dialog', ...) instead); update
the place where sendTestMail triggers dialogs to register the one-shot listener
before the action to prevent listener accumulation across multiple sendTestMail
calls and ensure each dialog is handled exactly once.
- Line 38: The test uses a fragile generic selector
page.locator('button.btn-outline-primary').click() — replace it with a stable
data-testid-based selector: add a specific data-testid attribute to the target
component (e.g., data-testid="confirm-button") and update the test to locate
that element (use page.getByTestId or page.locator('[data-testid="..."]') and
call click()); ensure the test and the UI component that renders the button (the
component/class that currently renders button.btn-outline-primary) are both
updated so the test targets the new data-testid.
In `@e2e/playwright.config.ts`:
- Around line 8-14: The current Playwright config sets retries: 0 so trace:
'on-first-retry' never triggers; update the trace setting inside the use object
in e2e/playwright.config.ts (the trace property in the use block) to a mode that
captures traces without retries (e.g., change trace to 'on' or
'retain-on-failure'), or alternatively increase retries from 0 if you want to
keep 'on-first-retry' behavior; modify the trace value accordingly to ensure
traces are actually collected during test runs.
In `@e2e/tests/command.spec.ts`:
- Around line 47-50: The test uses a hardcoded delay (new Promise(... setTimeout
2000)) which makes it flaky; replace that sleep with a direct, short-polling
check that calls getMessages() (or a small retry loop with a short overall
timeout) and assert that the returned array is empty, or add/use a helper
variant like waitForMessageAbsent/getMessagesEmpty to encapsulate this check;
apply the same fix to the other occurrence around the 67-70 block so both
assertions verify emptiness via getMessages() (or the new helper) instead of
sleeping.
- Around line 97-103: test.afterAll が test.describe('コマンド実行')
ブロックの外にありファイル全体に適用されているため、意図がコマンド実行テストだけに適用するなら test.describe
内に移動してください；具体的には現在の test.afterAll ブロック（browser を受け取り setThreshold(page, 5)
を呼ぶ処理）を test.describe('コマンド実行') の末尾に移してスコープを限定し、setThreshold 呼び出しと
context/newPage/close の処理がその describe 内の全テスト後にのみ実行されるようにしてください。
In `@e2e/tests/config.spec.ts`:
- Around line 44-45: Remove the unnecessary .catch(() => false) from the
isVisible call: change the assignment to use await
page.locator('.alert-success').isVisible() directly (the hasSuccess variable and
the expect(hasSuccess).toBeFalsy() assertion remain unchanged), since
isVisible() does not throw and the .catch is redundant.
- Around line 65-66: Change the implicit parent locator usage to an explicit
XPath expression for clarity: update the helpText locator that chains
page.locator('#stock_alert_config_alertEmails').locator('..').locator('.form-text')
so the parent reference uses locator('xpath=..') (i.e., locate the parent via
XPath) before chaining .locator('.form-text'); keep the same variable name
helpText and chaining behavior but replace the implicit '..' with 'xpath=..'.
In `@e2e/tests/error-cases.spec.ts`:
- Around line 13-14: Remove the redundant .catch(() => false) wrappers around
the Playwright visibility checks: replace the uses of
page.locator('.alert-success').isVisible().catch(() => false) and
page.locator('.alert-danger').isVisible().catch(() => false) with simple awaits
of page.locator(...).isVisible() (i.e., assign hasSuccess = await
page.locator('.alert-success').isVisible() and hasError = await
page.locator('.alert-danger').isVisible()), since isVisible() already returns
false for non-existent elements.
- Around line 28-36: The current complex assertion in the test "CSRF
トークンなしのテストメール送信は拒否される" makes intent unclear; replace the multi-code/URL OR check
with a single clear assertion on the response produced by the POST (the response
variable) — since the controller returns a 302 on CSRF failure, assert
response.status() === 302 (or alternatively assert
response.url().includes('/config') if you prefer checking the redirected
location); remove the array-of-codes logic and the combined boolean expression
to make the test deterministic and readable.
In `@e2e/tests/global-setup.ts`:
- Around line 5-6: Replace the hardcoded credentials used with
page.locator('#login_id').fill('admin') and
page.locator('#password').fill('password') so they read from environment
variables (e.g., process.env.E2E_LOGIN and process.env.E2E_PASSWORD); update the
global setup logic in e2e/tests/global-setup.ts to pull those vars, provide
sensible fallback or throw a clear error when missing, and use the retrieved
values when filling the '#login_id' and '#password' locators.
In `@e2e/tests/log.spec.ts`:
- Around line 42-58: The test TC-6.3 depends on log state possibly set by TC-6.2
(execStockAlert()), so make the test self-contained by explicitly creating the
expected stock-alert log and ensuring the page state before asserting the
product link; call or implement a deterministic setup helper (e.g.,
ensureStockAlert or awaitable execStockAlert) and a teardown/reset helper (e.g.,
clearLogs) so you can call clearLogs(); await execStockAlert() (or call the new
ensureStockAlert API) and wait for the log to appear via gotoLog(page) before
querying productLink (the locator 'table a[href*="admin/product/product"]'). If
the link is truly absent then fail the test rather than silently skip; update
TC-6.3 to use setThreshold(page, 9999), await execStockAlert(), await
gotoLog(page), then assert productLink.isVisible() and click it to expect the
/admin/product/product/:id/edit URL.
In `@e2e/tests/mail-template.spec.ts`:
- Around line 54-59: Replace the brittle waitForTimeout call with a
deterministic assertion: after clicking the submit button
(page.locator('button.btn-ec-conversion[type="submit"]').click()), wait for the
specific validation/error element to appear or for the success alert to remain
absent using Playwright's expect assertions instead of sleep. Concretely, remove
waitForTimeout and either assert the validation message locator becomes visible
(e.g., wait for your form validation selector) or call
expect(page.locator('.alert-success')).not.toBeVisible with an explicit timeout
option to assert the alert does not appear within the given time; ensure you
reference the same selectors used in this test (the submit button locator and
'.alert-success') when making the replacement.
In `@e2e/tests/test-mail.spec.ts`:
- Around line 43-51: Replace the brittle fixed-wait + try/catch pattern
(waitForTimeout, waitForMessage, expect(true).toBe(false)) with a direct
assertion that the mailbox is empty after the dialog cancellation: after calling
deleteAllMessages(), call getMessages() and assert its length is 0 with a clear
expectation message; remove the unnecessary page.waitForTimeout(2000) and the
try/catch around waitForMessage() so the test deterministically checks
getMessages() === 0 instead of relying on timeouts or an inverted expect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 592f4966-b86f-48e7-9368-3f713d9f083f
⛔ Files ignored due to path filters (1)
e2e/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
.github/workflows/ci.ymlService/StockAlertMailBuilder.phpe2e/.gitignoree2e/helpers/command.tse2e/helpers/mailhog.tse2e/helpers/pages.tse2e/package.jsone2e/playwright.config.tse2e/tests/command.spec.tse2e/tests/config.spec.tse2e/tests/error-cases.spec.tse2e/tests/global-setup.tse2e/tests/log.spec.tse2e/tests/mail-template.spec.tse2e/tests/navigation.spec.tse2e/tests/test-mail.spec.ts
| import { deleteAllMessages } from '../helpers/mailhog'; | ||
|
|
||
| test.describe('異常系・エッジケース', () => { | ||
| test('TC-7.2: テストメール送信失敗時にエラーメッセージが表示される（SMTP障害）', async ({ page }) => { |
There was a problem hiding this comment.
テスト ID が重複しています。
Line 6 と Line 20 の両方で TC-7.2 が使用されています。テストレポートでの識別を容易にするため、ユニークな ID を付与してください（例: TC-7.2a / TC-7.2b、または TC-7.2 / TC-7.3）。
Also applies to: 20-20
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/error-cases.spec.ts` at line 6, Two tests share the same ID string
"TC-7.2" which will cause duplicate test identifiers; locate the test call using
the title 'TC-7.2: テストメール送信失敗時にエラーメッセージが表示される（SMTP障害）' and the other test that
reuses "TC-7.2" and rename one to a unique ID (e.g., change one to "TC-7.3" or
"TC-7.2a"), and update any assertions/comments or external references that
expect the old ID so the test reports have distinct identifiers.
| import { test, expect } from '@playwright/test'; | ||
|
|
||
| test.describe('ナビゲーション', () => { | ||
| test('TC-1.1: サイドバーに在庫アラートメニューが表示される', async ({ page }) => { |
There was a problem hiding this comment.
テスト ID が重複しています。
3つのテストすべてが TC-1.1 を使用しています。テストレポートでの識別を容易にするため、各テストにユニークな ID を付与してください（例: TC-1.1、TC-1.2、TC-1.3）。
♻️ 修正案
- test('TC-1.1: サイドバーに在庫アラートメニューが表示される', async ({ page }) => {
+ test('TC-1.1: サイドバーに在庫アラートメニューが表示される', async ({ page }) => {
...
- test('TC-1.1: サブメニュー「設定」から設定画面に遷移する', async ({ page }) => {
+ test('TC-1.2: サブメニュー「設定」から設定画面に遷移する', async ({ page }) => {
...
- test('TC-1.1: サブメニュー「送信履歴」から送信履歴画面に遷移する', async ({ page }) => {
+ test('TC-1.3: サブメニュー「送信履歴」から送信履歴画面に遷移する', async ({ page }) => {Also applies to: 12-12, 27-27
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/navigation.spec.ts` at line 4, Multiple tests in navigation.spec.ts
use the same ID "TC-1.1"; update each test title string in the three test(...)
declarations (e.g., the test starting with "TC-1.1: サイドバーに在庫アラートメニューが表示される") so
each has a unique test ID like "TC-1.1", "TC-1.2", "TC-1.3" while keeping the
rest of the description intact; ensure you change all occurrences of the
duplicated "TC-1.1" test titles in this file.
EC-CUBEは.htaccessのRewriteRuleでindex.phpにルーティングしているが、 PHPビルトインサーバーは.htaccessを読まないため500エラーになっていた。 ルータースクリプトを生成して静的ファイル以外をindex.phpにルーティングする。 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
272-275: 固定sleep 3はフレークの原因になりやすいかもしれません起動完了前に
curlが走ると、環境負荷次第で不安定になりやすいので、リトライ付きの待機にしておくほうが安全だと思います。差分案
- php -S 127.0.0.1:8000 router.php & - sleep 3 - curl -sf http://127.0.0.1:8000/admin/login > /dev/null + php -S 127.0.0.1:8000 router.php >/tmp/eccube-php-server.log 2>&1 & + for i in {1..30}; do + if curl -sf http://127.0.0.1:8000/admin/login > /dev/null; then + break + fi + sleep 1 + done + curl -sf http://127.0.0.1:8000/admin/login > /dev/null || { + echo "EC-CUBE サーバー起動確認に失敗しました" + tail -n 200 /tmp/eccube-php-server.log || true + exit 1 + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 272 - 275, Replace the fixed sleep before the curl healthcheck with a retry loop that polls the admin login endpoint until it becomes available or a timeout is reached: run the PHP built-in server (router.php) in background as before, then repeatedly attempt the curl -sf http://127.0.0.1:8000/admin/login request (with short sleeps/backoff between attempts) and exit CI with failure if the endpoint does not respond within a reasonable timeout; this removes the brittle fixed "sleep 3" and makes the startup check resilient to variable startup times.
178-182: E2Eジョブにtimeout-minutesを入れておくと運用が楽ですねPlaywright や外部サービス待ちでハングしたときにランナーを長時間占有しやすいので、ジョブ全体のタイムアウトを付けておくのが無難だと思います。
差分案
e2e: name: Playwright E2E Tests needs: lint runs-on: ubuntu-latest + timeout-minutes: 30Also applies to: 287-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 178 - 182, Add a global job timeout for the Playwright E2E job by setting the timeout-minutes property on the e2e job definition (the job named "Playwright E2E Tests" / identifier "e2e") so the runner is not indefinitely locked by hangs; apply the same change to the other E2E job block mentioned (the second Playwright E2E job around the other occurrence) and choose a sensible timeout value (e.g., 30–60 minutes) appropriate for your test suite.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 272-275: Replace the fixed sleep before the curl healthcheck with
a retry loop that polls the admin login endpoint until it becomes available or a
timeout is reached: run the PHP built-in server (router.php) in background as
before, then repeatedly attempt the curl -sf http://127.0.0.1:8000/admin/login
request (with short sleeps/backoff between attempts) and exit CI with failure if
the endpoint does not respond within a reasonable timeout; this removes the
brittle fixed "sleep 3" and makes the startup check resilient to variable
startup times.
- Around line 178-182: Add a global job timeout for the Playwright E2E job by
setting the timeout-minutes property on the e2e job definition (the job named
"Playwright E2E Tests" / identifier "e2e") so the runner is not indefinitely
locked by hangs; apply the same change to the other E2E job block mentioned (the
second Playwright E2E job around the other occurrence) and choose a sensible
timeout value (e.g., 30–60 minutes) appropriate for your test suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60ed0e89-bf24-4e98-9df5-cd56d3d05627
📒 Files selected for processing (1)
.github/workflows/ci.yml
heredocとYAMLインデントの相性問題を回避するため、 router.phpをリポジトリに含める形に変更。 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PHP-CS-FixerによるEC-CUBEヘッダーコメント規約への準拠。 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ci.ymlの3ジョブを独立したワークフローに分離。 - lint.yml: PHP-CS-Fixer & PHPStan - test.yml: PHPUnit（e2e変更時はスキップ） - e2e.yml: Playwright E2E Tests 各ワークフローが独立して並列実行される。 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
プラグインディレクトリ内のPHPファイルはSymfonyのcache:clearで スキャンされるため、router.phpがe2e/内にあるとエラーになる。 CIのe2e.ymlでprintfにより動的生成する方式に変更。 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/lint.yml (1)
9-15: …少し確認したいところがあります。e2e/**パスの除外が必要かもしれません。
test.ymlではe2e/**を除外していますが、こちらのワークフローでは含まれたままですね。E2E テストファイル（TypeScript など）の変更時に lint ワークフローが実行される必要があるかどうか、意図通りか確認いただければと思います。PHP-CS-Fixer と PHPStan は PHP ファイルのみを対象としているので、E2E ディレクトリの変更で実行する必要はないかもしれません。
🔧 必要であれば除外を追加
paths: - '**' - '!*.md' + - '!e2e/**' pull_request: paths: - '**' - '!*.md' + - '!e2e/**'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/lint.yml around lines 9 - 15, lint ワークフロー (.github/workflows/lint.yml) が e2e/** を除外していないため、E2E テストディレクトリの変更で不要に lint が走る可能性があります；push と pull_request のそれぞれの paths 配列（現在の paths: - '**' - '!*.md' ブロック）に '!e2e/**' を追加して test.yml と同様に E2E ファイルを除外するよう更新してください。.github/workflows/e2e.yml (1)
36-40: …少し気になるところがあります。MailHog サービスにヘルスチェックがないですね。MySQL サービスにはヘルスチェックが設定されていますが、MailHog にはありません。MailHog の起動が遅れた場合、テストメール送信のテストが不安定になる可能性があるかもしれません。
🔧 ヘルスチェックの追加案
mailhog: image: mailhog/mailhog ports: - 1025:1025 - 8025:8025 + options: >- + --health-cmd="wget -q --spider http://localhost:8025/api/v2/messages || exit 1" + --health-interval=10s + --health-timeout=5s + --health-retries=3ただ、MailHog は比較的軽量なので、実際には問題が起きない可能性も高いです。もし E2E テストでメール関連のフレーキーなテストが発生した場合に検討していただければと思います。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e.yml around lines 36 - 40, Add a Docker healthcheck for the mailhog service to avoid flakey E2E tests when MailHog is slow to start: update the mailhog service block (service name "mailhog") to include a healthcheck that probes the HTTP UI on port 8025 (e.g., run a curl -f or similar CMD test against http://localhost:8025/), and set sensible interval/timeout/retries (e.g., short interval and a few retries) so the workflow waits until MailHog is ready before running tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/router.php`:
- Around line 14-22: This router is being loaded outside the PHP built-in web
server causing undefined $_SERVER['REQUEST_URI'] and missing
__DIR__.'/index.php' when run via CLI; add a guard at the top of this router.php
that ensures it only runs under the built-in server (check PHP_SAPI ===
'cli-server') and that $_SERVER['REQUEST_URI'] is set, and if not, return/stop
early; also before require __DIR__.'/index.php' verify that
file_exists(__DIR__.'/index.php') (or avoid requiring when not present) so the
script never attempts to parse URLs or include index.php in non-server contexts
(references: $_SERVER['REQUEST_URI'], parse_url, is_file, preg_match,
__DIR__.'/index.php').
---
Nitpick comments:
In @.github/workflows/e2e.yml:
- Around line 36-40: Add a Docker healthcheck for the mailhog service to avoid
flakey E2E tests when MailHog is slow to start: update the mailhog service block
(service name "mailhog") to include a healthcheck that probes the HTTP UI on
port 8025 (e.g., run a curl -f or similar CMD test against
http://localhost:8025/), and set sensible interval/timeout/retries (e.g., short
interval and a few retries) so the workflow waits until MailHog is ready before
running tests.
In @.github/workflows/lint.yml:
- Around line 9-15: lint ワークフロー (.github/workflows/lint.yml) が e2e/**
を除外していないため、E2E テストディレクトリの変更で不要に lint が走る可能性があります；push と pull_request のそれぞれの
paths 配列（現在の paths: - '**' - '!*.md' ブロック）に '!e2e/**' を追加して test.yml と同様に E2E
ファイルを除外するよう更新してください。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84bcf43f-abdb-4945-a326-03c1b65818d4
📒 Files selected for processing (4)
.github/workflows/e2e.yml.github/workflows/lint.yml.github/workflows/test.ymle2e/router.php
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
テストカバレッジ
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
リリースノート
Tests
Chores