From d4a080ddd3ad438f3481167a3a79b897e225d71a Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Fri, 23 Oct 2026 09:46:31 -0400 Subject: [PATCH 1/2] fix(ci): match Python coverage reports to their workflow run Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8b0f8941-4558-474e-9eae-005cb5ee0e47 --- .github/tests/test_coverage_report.js | 187 ++++++++++++++++++ .github/workflows/github-automation-tests.yml | 4 + .../workflows/python-test-coverage-report.yml | 66 +++++-- .github/workflows/python-test-coverage.yml | 7 +- 4 files changed, 239 insertions(+), 25 deletions(-) create mode 100644 .github/tests/test_coverage_report.js diff --git a/.github/tests/test_coverage_report.js b/.github/tests/test_coverage_report.js new file mode 100644 index 000000000000..bd4daed0958f --- /dev/null +++ b/.github/tests/test_coverage_report.js @@ -0,0 +1,187 @@ +// Copyright (c) Microsoft. All rights reserved. + +const { describe, it } = require('node:test'); +const assert = require('node:assert/strict'); +const { readFileSync, mkdtempSync, writeFileSync, rmSync } = require('node:fs'); +const { tmpdir } = require('node:os'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); + +const workflow = readFileSync( + path.join(__dirname, '../workflows/python-test-coverage-report.yml'), + 'utf8', +); +const selectionStep = workflow.split(' - name: Read and set PR number\n')[1].split(' - name:')[0]; +const context = { + repo: { owner: 'microsoft', repo: 'semantic-kernel' }, + payload: { + repository: { id: 1 }, + workflow_run: { + event: 'pull_request', + head_sha: 'abc123', + head_branch: 'topic', + head_repository: { id: 2 }, + pull_requests: [{ number: 42 }], + }, + }, +}; +const pullRequest = { + number: 42, + state: 'open', + base: { repo: { id: 1 } }, + head: { sha: 'abc123', ref: 'topic', repo: { id: 2 } }, +}; + +async function selectPullRequest({ eventContext = context, github = {}, artifactNumber = '99' } = {}) { + const script = selectionStep.match(/ script: \|\n((?: .*\n|\n)+)/); + if (script) { + const outputs = {}; + const warnings = []; + const AsyncFunction = Object.getPrototypeOf(async function () {}).constructor; + await new AsyncFunction('context', 'github', 'core', script[1].replace(/^ /gm, ''))( + eventContext, + github, + { + setOutput: (name, value) => { outputs[name] = value; }, + warning: (message) => { warnings.push(message); }, + setFailed: (message) => { throw new Error(message); }, + }, + ); + return { number: outputs.pr_number, warnings }; + } + + const run = selectionStep.match(/ run: \|\n((?: .*\n|\n)+)/); + assert.ok(run, 'Expected an executable PR-selection step'); + const directory = mkdtempSync(path.join(tmpdir(), 'coverage-report-')); + try { + writeFileSync(path.join(directory, 'pr_number'), artifactNumber); + const result = spawnSync( + process.platform === 'win32' ? 'C:\\Program Files\\Git\\bin\\bash.exe' : 'bash', + ['-e', '-c', run[1].replace(/^ /gm, '')], + { cwd: directory, env: { ...process.env, GITHUB_ENV: 'github-env' }, encoding: 'utf8' }, + ); + assert.ifError(result.error); + assert.equal(result.status, 0, result.stderr); + const number = readFileSync(path.join(directory, 'github-env'), 'utf8').match(/^PR_NUMBER=(.+)$/m); + return { number: number && Number(number[1]), warnings: [] }; + } finally { + rmSync(directory, { recursive: true, force: true }); + } +} + +function githubForPullRequest(pr = pullRequest) { + return { + rest: { + pulls: { + get: async (params) => { + assert.deepEqual(params, { ...context.repo, pull_number: 42 }); + return { data: pr }; + }, + }, + }, + }; +} + +function contextWithRun(changes) { + return { + ...context, + payload: { + ...context.payload, + workflow_run: { ...context.payload.workflow_run, ...changes }, + }, + }; +} + +function githubWithAssociatedPullRequests(prs) { + const github = githubForPullRequest(); + github.rest.repos = { listPullRequestsAssociatedWithCommit: Symbol('associated pull requests') }; + github.paginate = async (method, params) => { + assert.equal(method, github.rest.repos.listPullRequestsAssociatedWithCommit); + assert.deepEqual(params, { ...context.repo, commit_sha: 'abc123', per_page: 100 }); + return prs; + }; + return github; +} + +describe('Coverage report PR selection', () => { + it('selects the triggering run PR independently of the downloaded PR number', async () => { + const result = await selectPullRequest({ github: githubForPullRequest() }); + assert.equal(result.number, 42); + }); + + it('finds the matching fork PR when the run has no PR association', async () => { + const result = await selectPullRequest({ + eventContext: contextWithRun({ pull_requests: [] }), + github: githubWithAssociatedPullRequests([ + { ...pullRequest, number: 43, head: { ...pullRequest.head, repo: { id: 3 } } }, + pullRequest, + ]), + }); + assert.equal(result.number, 42); + }); + + it('skips runs without a unique matching PR and explains why', async () => { + for (const prs of [[], [pullRequest, { ...pullRequest, number: 43 }]]) { + const result = await selectPullRequest({ + eventContext: contextWithRun({ pull_requests: [] }), + github: githubWithAssociatedPullRequests(prs), + }); + assert.equal(result.number, undefined); + assert.match(result.warnings[0], /exactly one matching pull request/); + } + const result = await selectPullRequest({ + eventContext: contextWithRun({ pull_requests: [{ number: 42 }, { number: 43 }] }), + }); + assert.equal(result.number, undefined); + assert.match(result.warnings[0], /exactly one matching pull request/); + }); + + it('skips closed PRs and PRs whose commit, branch, or repository no longer matches', async () => { + for (const pr of [ + { ...pullRequest, state: 'closed' }, + { ...pullRequest, base: { repo: { id: 3 } } }, + { ...pullRequest, head: { ...pullRequest.head, sha: 'def456' } }, + { ...pullRequest, head: { ...pullRequest.head, ref: 'other' } }, + { ...pullRequest, head: { ...pullRequest.head, repo: { id: 3 } } }, + { ...pullRequest, head: { ...pullRequest.head, repo: null } }, + ]) { + const result = await selectPullRequest({ github: githubForPullRequest(pr) }); + assert.equal(result.number, undefined); + assert.match(result.warnings[0], /no longer matches/); + } + }); + + it('does not report on runs for other events', async () => { + const result = await selectPullRequest({ eventContext: contextWithRun({ event: 'push' }) }); + assert.equal(result.number, undefined); + assert.match(result.warnings[0], /pull request workflow run/); + }); + + it('skips runs whose source repository is no longer available', async () => { + const result = await selectPullRequest({ + eventContext: contextWithRun({ head_repository: null }), + github: githubForPullRequest({ ...pullRequest, head: { ...pullRequest.head, repo: null } }), + }); + assert.equal(result.number, undefined); + assert.match(result.warnings[0], /no longer matches/); + }); + + it('propagates API failures instead of selecting a different PR', async () => { + const github = githubForPullRequest(); + github.rest.pulls.get = async () => { throw new Error('API unavailable'); }; + await assert.rejects(selectPullRequest({ github }), /API unavailable/); + }); + + it('uses the selected PR output for commenting and resolves it before downloading reports', () => { + const commentStep = workflow.split(' - name: Pytest coverage comment\n')[1]; + const downloadStep = workflow.split(' - name: Download coverage report\n')[1].split(' - name:')[0]; + assert.match(commentStep, /issue-number: \$\{\{ steps\.pr\.outputs\.pr_number \}\}/); + for (const step of [commentStep, downloadStep]) { + assert.match(step, /if: steps\.pr\.outputs\.pr_number != ''/); + } + assert.ok( + workflow.indexOf(' - name: Read and set PR number\n') < + workflow.indexOf(' - name: Download coverage report\n'), + ); + }); +}); diff --git a/.github/workflows/github-automation-tests.yml b/.github/workflows/github-automation-tests.yml index a7b3e003f38d..6604bd9c6176 100644 --- a/.github/workflows/github-automation-tests.yml +++ b/.github/workflows/github-automation-tests.yml @@ -10,6 +10,8 @@ on: - ".github/workflows/github-automation-tests.yml" - ".github/workflows/label-issues.yml" - ".github/workflows/label-pr.yml" + - ".github/workflows/python-test-coverage.yml" + - ".github/workflows/python-test-coverage-report.yml" push: branches: - main @@ -21,6 +23,8 @@ on: - ".github/workflows/github-automation-tests.yml" - ".github/workflows/label-issues.yml" - ".github/workflows/label-pr.yml" + - ".github/workflows/python-test-coverage.yml" + - ".github/workflows/python-test-coverage-report.yml" permissions: contents: read diff --git a/.github/workflows/python-test-coverage-report.yml b/.github/workflows/python-test-coverage-report.yml index a940675a476c..1d03ad53bfdc 100644 --- a/.github/workflows/python-test-coverage-report.yml +++ b/.github/workflows/python-test-coverage-report.yml @@ -21,37 +21,65 @@ jobs: working-directory: python steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - name: Read and set PR number + id: pr + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + with: + script: | + const run = context.payload.workflow_run; + if (run.event !== 'pull_request') { + core.warning('Coverage reporting requires a pull request workflow run.'); + return; + } + const matchesRun = (pr) => + pr.state === 'open' && + pr.base.repo.id === context.payload.repository.id && + pr.head.sha === run.head_sha && + pr.head.ref === run.head_branch && + pr.head.repo?.id != null && + pr.head.repo.id === run.head_repository?.id; + let candidates = run.pull_requests ?? []; + if (candidates.length === 0) { + candidates = await github.paginate( + github.rest.repos.listPullRequestsAssociatedWithCommit, + { ...context.repo, commit_sha: run.head_sha, per_page: 100 }, + ); + candidates = candidates.filter(matchesRun); + } + if (candidates.length !== 1) { + core.warning('Coverage reporting requires exactly one matching pull request.'); + return; + } + const { data: pr } = await github.rest.pulls.get({ + ...context.repo, + pull_number: candidates[0].number, + }); + if (!matchesRun(pr)) { + core.warning('The pull request no longer matches the coverage run.'); + return; + } + core.setOutput('pr_number', pr.number); - name: Download coverage report + if: steps.pr.outputs.pr_number != '' uses: actions/download-artifact@634f93cb2916e3fdff6788551b99b062d0335ce0 # v5.0.0 with: + name: python-test-coverage github-token: ${{ github.token }} run-id: ${{ github.event.workflow_run.id }} - path: ./python - merge-multiple: true - - name: Display structure of downloaded files - run: ls - - name: Read and set PR number - # Need to read the PR number from the file saved in the previous workflow - # because the workflow_run event does not have access to the PR number - # The PR number is needed to post the comment on the PR - run: | - PR_NUMBER=$(cat pr_number) - if ! [[ "$PR_NUMBER" =~ ^[0-9]+$ ]]; then - echo "::error::PR number file contains invalid content" - exit 1 - fi - echo "PR number: $PR_NUMBER" - echo "PR_NUMBER=$PR_NUMBER" >> $GITHUB_ENV + path: ./python/coverage-report - name: Pytest coverage comment + if: steps.pr.outputs.pr_number != '' id: coverageComment uses: MishaKav/pytest-coverage-comment@1bdbba85fb74a2fdc1ec66383acec1091610f82b # v1.1.57 with: github-token: ${{ github.token }} - issue-number: ${{ env.PR_NUMBER }} - pytest-xml-coverage-path: python/python-coverage.xml + issue-number: ${{ steps.pr.outputs.pr_number }} + pytest-xml-coverage-path: python/coverage-report/python-coverage.xml title: "Python Test Coverage Report" badge-title: "Python Test Coverage" junitxml-title: "Python Unit Test Overview" - junitxml-path: python/pytest.xml + junitxml-path: python/coverage-report/pytest.xml default-branch: "main" report-only-changed-files: true diff --git a/.github/workflows/python-test-coverage.yml b/.github/workflows/python-test-coverage.yml index 78f53d8f0dff..2995ffaca556 100644 --- a/.github/workflows/python-test-coverage.yml +++ b/.github/workflows/python-test-coverage.yml @@ -21,11 +21,6 @@ jobs: UV_PYTHON: "3.10" steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - # Save the PR number to a file since the workflow_run event - # in the coverage report workflow does not have access to it - - name: Save PR number - run: | - echo ${{ github.event.number }} > ./pr_number - name: Set up uv uses: astral-sh/setup-uv@d0cc045d04ccac9d8b7881df0226f9e82c39688e # v6.8.0 with: @@ -40,10 +35,10 @@ jobs: - name: Upload coverage report uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 with: + name: python-test-coverage path: | python/python-coverage.xml python/pytest.xml - python/pr_number overwrite: true retention-days: 1 if-no-files-found: error From e2b1926e4c2e04cbc355a5b9d685252b942325e8 Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Fri, 23 Oct 2026 10:06:05 -0400 Subject: [PATCH 2/2] fix(ci): remove coverage workflow unit tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8b0f8941-4558-474e-9eae-005cb5ee0e47 --- .github/tests/test_coverage_report.js | 187 ------------------ .github/workflows/github-automation-tests.yml | 4 - 2 files changed, 191 deletions(-) delete mode 100644 .github/tests/test_coverage_report.js diff --git a/.github/tests/test_coverage_report.js b/.github/tests/test_coverage_report.js deleted file mode 100644 index bd4daed0958f..000000000000 --- a/.github/tests/test_coverage_report.js +++ /dev/null @@ -1,187 +0,0 @@ -// Copyright (c) Microsoft. All rights reserved. - -const { describe, it } = require('node:test'); -const assert = require('node:assert/strict'); -const { readFileSync, mkdtempSync, writeFileSync, rmSync } = require('node:fs'); -const { tmpdir } = require('node:os'); -const path = require('node:path'); -const { spawnSync } = require('node:child_process'); - -const workflow = readFileSync( - path.join(__dirname, '../workflows/python-test-coverage-report.yml'), - 'utf8', -); -const selectionStep = workflow.split(' - name: Read and set PR number\n')[1].split(' - name:')[0]; -const context = { - repo: { owner: 'microsoft', repo: 'semantic-kernel' }, - payload: { - repository: { id: 1 }, - workflow_run: { - event: 'pull_request', - head_sha: 'abc123', - head_branch: 'topic', - head_repository: { id: 2 }, - pull_requests: [{ number: 42 }], - }, - }, -}; -const pullRequest = { - number: 42, - state: 'open', - base: { repo: { id: 1 } }, - head: { sha: 'abc123', ref: 'topic', repo: { id: 2 } }, -}; - -async function selectPullRequest({ eventContext = context, github = {}, artifactNumber = '99' } = {}) { - const script = selectionStep.match(/ script: \|\n((?: .*\n|\n)+)/); - if (script) { - const outputs = {}; - const warnings = []; - const AsyncFunction = Object.getPrototypeOf(async function () {}).constructor; - await new AsyncFunction('context', 'github', 'core', script[1].replace(/^ /gm, ''))( - eventContext, - github, - { - setOutput: (name, value) => { outputs[name] = value; }, - warning: (message) => { warnings.push(message); }, - setFailed: (message) => { throw new Error(message); }, - }, - ); - return { number: outputs.pr_number, warnings }; - } - - const run = selectionStep.match(/ run: \|\n((?: .*\n|\n)+)/); - assert.ok(run, 'Expected an executable PR-selection step'); - const directory = mkdtempSync(path.join(tmpdir(), 'coverage-report-')); - try { - writeFileSync(path.join(directory, 'pr_number'), artifactNumber); - const result = spawnSync( - process.platform === 'win32' ? 'C:\\Program Files\\Git\\bin\\bash.exe' : 'bash', - ['-e', '-c', run[1].replace(/^ /gm, '')], - { cwd: directory, env: { ...process.env, GITHUB_ENV: 'github-env' }, encoding: 'utf8' }, - ); - assert.ifError(result.error); - assert.equal(result.status, 0, result.stderr); - const number = readFileSync(path.join(directory, 'github-env'), 'utf8').match(/^PR_NUMBER=(.+)$/m); - return { number: number && Number(number[1]), warnings: [] }; - } finally { - rmSync(directory, { recursive: true, force: true }); - } -} - -function githubForPullRequest(pr = pullRequest) { - return { - rest: { - pulls: { - get: async (params) => { - assert.deepEqual(params, { ...context.repo, pull_number: 42 }); - return { data: pr }; - }, - }, - }, - }; -} - -function contextWithRun(changes) { - return { - ...context, - payload: { - ...context.payload, - workflow_run: { ...context.payload.workflow_run, ...changes }, - }, - }; -} - -function githubWithAssociatedPullRequests(prs) { - const github = githubForPullRequest(); - github.rest.repos = { listPullRequestsAssociatedWithCommit: Symbol('associated pull requests') }; - github.paginate = async (method, params) => { - assert.equal(method, github.rest.repos.listPullRequestsAssociatedWithCommit); - assert.deepEqual(params, { ...context.repo, commit_sha: 'abc123', per_page: 100 }); - return prs; - }; - return github; -} - -describe('Coverage report PR selection', () => { - it('selects the triggering run PR independently of the downloaded PR number', async () => { - const result = await selectPullRequest({ github: githubForPullRequest() }); - assert.equal(result.number, 42); - }); - - it('finds the matching fork PR when the run has no PR association', async () => { - const result = await selectPullRequest({ - eventContext: contextWithRun({ pull_requests: [] }), - github: githubWithAssociatedPullRequests([ - { ...pullRequest, number: 43, head: { ...pullRequest.head, repo: { id: 3 } } }, - pullRequest, - ]), - }); - assert.equal(result.number, 42); - }); - - it('skips runs without a unique matching PR and explains why', async () => { - for (const prs of [[], [pullRequest, { ...pullRequest, number: 43 }]]) { - const result = await selectPullRequest({ - eventContext: contextWithRun({ pull_requests: [] }), - github: githubWithAssociatedPullRequests(prs), - }); - assert.equal(result.number, undefined); - assert.match(result.warnings[0], /exactly one matching pull request/); - } - const result = await selectPullRequest({ - eventContext: contextWithRun({ pull_requests: [{ number: 42 }, { number: 43 }] }), - }); - assert.equal(result.number, undefined); - assert.match(result.warnings[0], /exactly one matching pull request/); - }); - - it('skips closed PRs and PRs whose commit, branch, or repository no longer matches', async () => { - for (const pr of [ - { ...pullRequest, state: 'closed' }, - { ...pullRequest, base: { repo: { id: 3 } } }, - { ...pullRequest, head: { ...pullRequest.head, sha: 'def456' } }, - { ...pullRequest, head: { ...pullRequest.head, ref: 'other' } }, - { ...pullRequest, head: { ...pullRequest.head, repo: { id: 3 } } }, - { ...pullRequest, head: { ...pullRequest.head, repo: null } }, - ]) { - const result = await selectPullRequest({ github: githubForPullRequest(pr) }); - assert.equal(result.number, undefined); - assert.match(result.warnings[0], /no longer matches/); - } - }); - - it('does not report on runs for other events', async () => { - const result = await selectPullRequest({ eventContext: contextWithRun({ event: 'push' }) }); - assert.equal(result.number, undefined); - assert.match(result.warnings[0], /pull request workflow run/); - }); - - it('skips runs whose source repository is no longer available', async () => { - const result = await selectPullRequest({ - eventContext: contextWithRun({ head_repository: null }), - github: githubForPullRequest({ ...pullRequest, head: { ...pullRequest.head, repo: null } }), - }); - assert.equal(result.number, undefined); - assert.match(result.warnings[0], /no longer matches/); - }); - - it('propagates API failures instead of selecting a different PR', async () => { - const github = githubForPullRequest(); - github.rest.pulls.get = async () => { throw new Error('API unavailable'); }; - await assert.rejects(selectPullRequest({ github }), /API unavailable/); - }); - - it('uses the selected PR output for commenting and resolves it before downloading reports', () => { - const commentStep = workflow.split(' - name: Pytest coverage comment\n')[1]; - const downloadStep = workflow.split(' - name: Download coverage report\n')[1].split(' - name:')[0]; - assert.match(commentStep, /issue-number: \$\{\{ steps\.pr\.outputs\.pr_number \}\}/); - for (const step of [commentStep, downloadStep]) { - assert.match(step, /if: steps\.pr\.outputs\.pr_number != ''/); - } - assert.ok( - workflow.indexOf(' - name: Read and set PR number\n') < - workflow.indexOf(' - name: Download coverage report\n'), - ); - }); -}); diff --git a/.github/workflows/github-automation-tests.yml b/.github/workflows/github-automation-tests.yml index 6604bd9c6176..a7b3e003f38d 100644 --- a/.github/workflows/github-automation-tests.yml +++ b/.github/workflows/github-automation-tests.yml @@ -10,8 +10,6 @@ on: - ".github/workflows/github-automation-tests.yml" - ".github/workflows/label-issues.yml" - ".github/workflows/label-pr.yml" - - ".github/workflows/python-test-coverage.yml" - - ".github/workflows/python-test-coverage-report.yml" push: branches: - main @@ -23,8 +21,6 @@ on: - ".github/workflows/github-automation-tests.yml" - ".github/workflows/label-issues.yml" - ".github/workflows/label-pr.yml" - - ".github/workflows/python-test-coverage.yml" - - ".github/workflows/python-test-coverage-report.yml" permissions: contents: read