Skip to content
Open
Changes from 13 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
40 changes: 36 additions & 4 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ jobs:
test-php:
name: Test PHP ${{ matrix.php }} ${{ matrix.wp != '' && format( ' (WP {0}) ', matrix.wp ) || '' }}
runs-on: ubuntu-24.04
permissions:
contents: read
id-token: write
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

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

id-token: write is granted at the job level, which means every matrix run (and every step in those runs) can mint OIDC tokens. Since only the Codecov upload needs OIDC, consider moving coverage upload into a separate job (or otherwise restricting when it runs, e.g., only on trusted push events) so untrusted PR code paths don’t receive id-token: write unnecessarily.

Suggested change
id-token: write

Copilot uses AI. Check for mistakes.
strategy:
matrix:
php:
Expand Down Expand Up @@ -120,21 +123,50 @@ jobs:
- name: Composer install
run: |
rm composer.lock || true # We need to install fresh.
npm run composer install
# The composer.json platform override (php: 7.2.24) installs PHPUnit 8.5, which does
# not support code coverage on PHP 8. Use --ignore-platform-req=php on PHP 8+ so
# Composer installs PHPUnit 9.6, which supports coverage on PHP 8.x.
if [[ "${WP_ENV_PHP_VERSION}" == 8.* ]]; then
npm run composer -- install --ignore-platform-req=php
else
npm run composer install
fi
Comment on lines +126 to +133
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

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

The --ignore-platform-req=php install path will run for all PHP 8.x matrix entries, changing the dependency set relative to the PHP 7.x runs (and relative to what the config.platform override is intended to enforce). If the goal is specifically to enable coverage generation for the single Codecov upload build, consider narrowing this condition to that build (e.g., the same matrix.php == '8.3' && matrix.wp == 'latest' gate) to reduce variability across the test matrix.

Copilot uses AI. Check for mistakes.

- name: Versions
run: |
npm run env run cli php -- -v
npm run env run cli wp core version

- name: Test
run: npm run test
run: |
npm run env run tests-cli --env-cwd=wp-content/plugins/two-factor -- mkdir -p tests/logs
npm run test

- name: Upload code coverage report
- name: Retrieve coverage report from container
if: ${{ matrix.php == '8.3' && matrix.wp == 'latest' }}
run: |
# PHPUnit writes clover.xml into the Docker volume shared between tests-cli and
# tests-wordpress. Use docker cp (not 'docker exec cat > file'): docker cp is an
# atomic API call that avoids a shell redirect pre-truncating the destination file,
# which would zero it out before cat can read it (self-overwrite).
CONTAINER=$(docker ps --filter "name=tests-wordpress" --format "{{.Names}}" | head -1)
if [ -z "$CONTAINER" ]; then
echo "Error: tests-wordpress container not found"
exit 1
fi
mkdir -p tests/logs
docker cp "$CONTAINER:/var/www/html/wp-content/plugins/two-factor/tests/logs/clover.xml" tests/logs/clover.xml
if [ ! -s tests/logs/clover.xml ]; then
echo "Error: clover.xml is empty — coverage was not generated"
exit 1
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

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

The “Retrieve coverage report from container” step fails the job if the container isn’t found or if clover.xml is empty. That makes CI more brittle even though the subsequent Codecov upload is configured with fail_ci_if_error: false. If the intent is to keep coverage/upload best-effort (badge-only), consider turning these hard failures into a warning + skip upload (or gating the failure to pushes on the default branch only).

Suggested change
echo "Error: tests-wordpress container not found"
exit 1
fi
mkdir -p tests/logs
docker cp "$CONTAINER:/var/www/html/wp-content/plugins/two-factor/tests/logs/clover.xml" tests/logs/clover.xml
if [ ! -s tests/logs/clover.xml ]; then
echo "Error: clover.xml is empty — coverage was not generated"
exit 1
echo "Warning: tests-wordpress container not found; skipping coverage retrieval"
exit 0
fi
mkdir -p tests/logs
if ! docker cp "$CONTAINER:/var/www/html/wp-content/plugins/two-factor/tests/logs/clover.xml" tests/logs/clover.xml; then
echo "Warning: Failed to copy clover.xml from container; skipping coverage upload"
exit 0
fi
if [ ! -s tests/logs/clover.xml ]; then
echo "Warning: clover.xml is empty — coverage was not generated; skipping coverage upload"
exit 0

Copilot uses AI. Check for mistakes.
fi

- name: Upload code coverage report
if: ${{ matrix.php == '8.3' && matrix.wp == 'latest' && hashFiles('tests/logs/clover.xml') != '' }}
uses: codecov/codecov-action@671740ac38dd9b0130fbe1cec585b89eea48d3de
with:
file: tests/logs/clover.xml
use_oidc: true
files: tests/logs/clover.xml
flags: phpunit
fail_ci_if_error: false

Expand Down
Loading