Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 49 additions & 8 deletions .github/workflows/e2e-pull_request.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ name: E2E Screenshot Tests Pull Request

on:
pull_request:
branches:
- main
types: [opened, synchronize, closed]

permissions:
Expand All @@ -10,7 +12,8 @@ permissions:
jobs:
test:
if: github.event.action != 'closed'
timeout-minutes: 10
# ios27 uses 10, but this PR's large visual diff + 2 retries exceeds that.
timeout-minutes: 20
Comment on lines +15 to +16

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 タイムアウト理由が一時的な文脈に依存する

永続設定のコメントが「this PR」の差分を根拠にしており、マージ後は文脈が失われます。20分の恒久的な根拠へ直すか確認してください。

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

runs-on: ubuntu-latest
permissions:
contents: read
Expand Down Expand Up @@ -48,7 +51,12 @@ jobs:
working-directory: './demo'

- name: Run Playwright tests
run: PLAYWRIGHT_JSON_OUTPUT_NAME=e2e/screenshot.spec.ts-ionic${{ matrix.ionic-major }}.json npm run test:e2e -- --reporter=json,html
run: |
if [ "${{ matrix.ionic-major }}" = "8" ]; then
PLAYWRIGHT_JSON_OUTPUT_NAME=e2e/screenshot.spec.ts-ionic8.json npm run test:e2e:visual -- --reporter=json,html
else
PLAYWRIGHT_JSON_OUTPUT_NAME=e2e/screenshot.spec.ts-ionic9.json npm run test:e2e -- --reporter=json,html
fi
Comment on lines +54 to +59

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Ionic 8のRTL回帰テストが対象外になる

Ionic 8は視覚テストだけを実行するため、RTL回帰テスト はIonic 9限定になります。このカバレッジ縮小が意図どおりか確認してください。

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

working-directory: ./demo
env:
IONIC_MAJOR: ${{ matrix.ionic-major }}
Expand Down Expand Up @@ -89,6 +97,7 @@ jobs:
needs: test
if: >-
always() &&
!cancelled() &&
github.event.action != 'closed' &&
github.event.pull_request.head.repo.full_name == github.repository
runs-on: ubuntu-latest
Expand All @@ -110,8 +119,12 @@ jobs:

deploy-report:
needs: test
# Serialize gh-pages pushes for the same PR and avoid deploying on cancellations.
concurrency:
group: playwright-report-${{ github.event.pull_request.number }}
cancel-in-progress: false
Comment on lines +124 to +125

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 PR別のグループでは共有ブランチへのpushが競合する

異なるPRのデプロイやクリーンアップは、同じ gh-pages ブランチへ同時にpushできます。競合した実行は失敗し、対象レポートが更新または削除されません。

Learn more

すべてのレポートは一つの gh-pages ブランチへ書き込まれます。このグループ名にはPR番号が含まれるため、PR #10とPR #11のデプロイは互いを待ちません。cleanup-report は同じブランチへpushしますが、どのconcurrencyグループにも属しません。各実行が別々にチェックアウトしたブランチを公開するため、同時pushでは少なくとも一方が競合して失敗し得ます。

Example: PR #10とPR #11のテストが同時に完了すると、グループ名はそれぞれ playwright-report-10 と playwright-report-11 です。両方が同じ gh-pages の先端から別コミットを作り、後からpushしたジョブが競合します。

Recommended fix: deploy-report と cleanup-report の両方に、PR番号を含まない共通concurrencyグループを設定してください。gh-pages を更新する他のワークフローがある場合も同じグループへ参加させてください。

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +124 to +125

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 古い実行による最新レポートの上書き

連続pushで新しいテストが先に完了すると、古い実行が後から公開できます。レポートとコメントが現在のPR先端ではなく古い結果へ戻ります。

Learn more

このconcurrency制御はデプロイ開始後にだけ適用され、同じPRの古いテスト実行をキャンセルしません。cancel-in-progress: false なので、古い実行もグループを取得できれば必ず公開を試みます。テスト完了順はコミット順と一致しないため、古い成果物が最新成果物の後に公開されます。コメント本文もその古いworkflow runへのリンクで更新されます。

Example: コミットAのテスト中にコミットBをpushし、Bのテストが先に終わるとBのレポートが公開されます。その後Aが終わるとAのレポートと実行リンクで上書きされますが、PRの先端はBのままです。

Recommended fix: PR単位で古いworkflow run全体をキャンセルするか、公開直前に実行対象SHAが現在のPR head SHAと一致することを確認してください。共有ブランチのpush直列化とは別に、古い成果物を公開しない仕組みが必要です。

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

if: >-
always() &&
!cancelled() &&
github.event.action != 'closed' &&
github.event.pull_request.head.repo.full_name == github.repository
runs-on: ubuntu-latest
Expand Down Expand Up @@ -154,6 +167,11 @@ jobs:
user_email: 'github-actions[bot]@users.noreply.github.com'
commit_message: 'Update test report for PR #${{ github.event.pull_request.number }}'

- name: Verify published report URL
env:
REPORT_URL: https://${{ github.repository_owner }}.github.io/${{ github.event.repository.name }}/pr-${{ github.event.pull_request.number }}/
run: curl --fail --silent --show-error --location --retry 6 --retry-delay 10 --retry-all-errors --max-time 20 --output /dev/null "$REPORT_URL"
Comment on lines +170 to +173

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 失敗した公開確認の後にもレポートリンクが投稿される

URL確認が失敗しても、後続コメントは always() により実行されます。未公開または到達不能なレポートへのリンクがPRへ投稿されます。

Learn more

このステップはcurlの終了コードで公開URLを検証します。しかし、直後のコメントステップには if: always() があるため、このステップが失敗してもコメント処理が実行されます。その結果、追加した検証はジョブを失敗させるだけで、リンク投稿を止めません。

Example: Pagesの反映が全リトライ後も404ならcurlは失敗します。それでもコメントは作成または更新され、利用者には404のURLが案内されます。

Recommended fix: コメントステップを検証成功時だけ実行してください。例えば検証ステップへIDを付けて成功を条件にするか、コメントステップの if: always() を通常の成功条件へ変更します。デプロイ失敗時にも既存コメントを整理したい場合は、成功用と失敗用の処理を分けてください。

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


- name: Comment PR with report link
uses: actions/github-script@v7
if: always()
Expand All @@ -162,12 +180,35 @@ jobs:
const prNumber = context.payload.pull_request.number;
const reportUrl = `https://${context.repo.owner}.github.io/${context.repo.repo}/pr-${prNumber}/`;

await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: prNumber,
body: `📊 **Ionic 9 Playwright Test Report**\n\nView the detailed Ionic 9 report: ${reportUrl}\n\nIonic 8 runs against the same screenshots in a separate matrix job. View both results in the [workflow run](${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}).`
const marker = '<!-- ionic9-playwright-report -->';
const body = `${marker}\n` + `📊 **Ionic 9 Playwright Test Report**\n\nView the detailed Ionic 9 report: ${reportUrl}\n\nIonic 8 runs against the same screenshots in a separate matrix job. View both results in the [workflow run](${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}).`;
const comments = await github.paginate(github.rest.issues.listComments, {
...context.repo,
issue_number: prNumber
});
const reports = comments.filter(comment =>
comment.user?.login === 'github-actions[bot]' &&
(comment.body?.includes(marker) || comment.body?.startsWith('📊 **Ionic 9 Playwright Test Report**'))
);
if (reports.length > 0) {
await github.rest.issues.updateComment({
...context.repo,
comment_id: reports[0].id,
body
});
for (const duplicate of reports.slice(1)) {
await github.rest.issues.deleteComment({
...context.repo,
comment_id: duplicate.id
});
}
} else {
await github.rest.issues.createComment({
...context.repo,
issue_number: prNumber,
body
});
}

cleanup-report:
if: >-
Expand Down
1 change: 1 addition & 0 deletions demo/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
"pretest": "npm run docs:generate",
"test": "ng test",
"test:e2e": "playwright test",
"test:e2e:visual": "playwright test e2e/screenshot.spec.ts",
"test:e2e:ui": "playwright test --ui",
"test:e2e:debug": "playwright test --debug",
"test:e2e:update": "playwright test --update-snapshots",
Expand Down
Loading