Skip to content

Try adding codecov #51

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aaronjorbin wants to merge 5 commits into
base: trunk
Choose a base branch
from
Open

Try adding codecov #51

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4956624
Try adding codecov
aaronjorbin Feb 4, 2026
b6b08ee
Fix typo
aaronjorbin Feb 4, 2026
a9fa82e
Include coverage in name of action
aaronjorbin Feb 4, 2026
1053c53
file is deprecated in favor of files
aaronjorbin Feb 4, 2026
eb7fd5a
Experimental is default, so it's not necessary
aaronjorbin Feb 4, 2026
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
28 changes: 25 additions & 3 deletions .github/workflows/php-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ concurrency:

jobs:
php-lint:
name: PHP ${{ matrix.php }} - WP ${{ matrix.wordpress }} - ${{ matrix.multisite && 'Multisite' || 'Single site' }}${{ matrix.experimental && ' (experimental)' || '' }}
name: PHP ${{ matrix.php }} - WP ${{ matrix.wordpress }} - ${{ matrix.multisite && 'Multisite' || 'Single site' }}${{ matrix.experimental && ' (experimental)' || '' }} ${{ matrix.coverage && ' (coverage)' || '' }}
Copy link
Member

@desrosj desrosj Feb 13, 2026

Choose a reason for hiding this comment

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

Suggested change
name: PHP ${{ matrix.php }} - WP ${{ matrix.wordpress }} - ${{ matrix.multisite && 'Multisite' || 'Single site' }}${{ matrix.experimental && ' (experimental)' || '' }} ${{ matrix.coverage && ' (coverage)' || '' }}
name: PHP ${{ matrix.php }} - WP ${{ matrix.wordpress }} - ${{ matrix.multisite && 'Multisite' || 'Single site' }}${{ matrix.experimental && ' (experimental)' || '' }} ${{ matrix.coverage && ' (with coverage)' || '' }}

runs-on: ubuntu-latest
timeout-minutes: 20
strategy:
Expand All @@ -47,6 +47,7 @@ jobs:
wordpress: [ 'trunk' ]
multisite: [ false, true ]
experimental: [false]
coverage: [false]
include:
- php: '8.4'
wordpress: 'trunk'
Expand All @@ -56,6 +57,14 @@ jobs:
wordpress: 'trunk'
multisite: true
experimental: true
- php: '8.4'
Copy link

@justlevine justlevine Feb 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Someone double check me (1am on my phone) but I think all these can be deduped to a single

- php: '8.4'
  wordpress: 'trunk'
  coverage: true
  multisite: [true, false]
  experimental: [true, false]

(Semantically: run coverage on the latest branch against both single/multisite and with/without experimental )

Copy link
Member Author

@aaronjorbin aaronjorbin Feb 10, 2026

Choose a reason for hiding this comment

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

I didn't include experimental in the coverage runs since I figured that the barriers for something being experimental should be as low as possible and that includes affected the automated test coverage.

Copy link

@justlevine justlevine Feb 10, 2026

Choose a reason for hiding this comment

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

Oh, interesting. TBH I'm not entirely sure what experimental is supposed to capture in this plugin. Seems all it does is allow continue-on-error, but I'm not seeing it map to an env var, feature flag, test group, cli param... 🤷

Copy link
Member Author

@aaronjorbin aaronjorbin Feb 10, 2026

Choose a reason for hiding this comment

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

Interesting. I wonder if the idea was to do more, but that never got implemented?

@felixarntz, it looks like the experimental flag was added when you first setup tests here. Is there more that any context we are missing or anything you think we should consider here?

This is feeling like it might be a tangent that is separate from the goal of understanding test coverage. Maybe a followup ticket related to the experimental flag would make sense?

wordpress: 'trunk'
multisite: false
coverage: true
- php: '8.4'
wordpress: 'trunk'
multisite: true
coverage: true
env:
WP_ENV_PHP_VERSION: ${{ matrix.php }}
WP_ENV_CORE: ${{ matrix.wordpress == 'trunk' && 'WordPress/WordPress' || format( 'https://wordpress.org/wordpress-{0}.zip', matrix.wordpress ) }}
Expand Down Expand Up @@ -85,7 +94,20 @@ jobs:
run: npm ci

- name: Install WordPress
run: npm run wp-env start
run: |
if [[ ${{ matrix.coverage == true }} == true ]]; then
npm run wp-env start -- --xdebug=coverage
else
npm run wp-env start
fi

- name: Running ${{ matrix.multisite && 'multisite' || 'single site' }} unit tests
run: npm run test-php${{ matrix.multisite && '-multisite' || '' }}
run: npm run test-php${{ matrix.multisite && '-multisite' || '' }}${{ matrix.coverage && '-coverage' || '' }}

- name: Upload code coverage report
if: ${{ matrix.coverage }}
uses: codecov/codecov-action@671740ac38dd9b0130fbe1cec585b89eea48d3de
Copy link
Member

@desrosj desrosj Feb 13, 2026

Choose a reason for hiding this comment

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

Suggested change
uses: codecov/codecov-action@671740ac38dd9b0130fbe1cec585b89eea48d3de
uses: codecov/codecov-action@671740ac38dd9b0130fbe1cec585b89eea48d3de # v5.2.2

Including the human-readable version number is always helpful.

with:
files: build/logs/*.xml
flags: unit
Copy link
Member

@desrosj desrosj Feb 13, 2026

Choose a reason for hiding this comment

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

Would multisite and single-site (or single) make more sense here? In wordpress-develop we pass single and multisite (the php actually needs to be removed because only 1 flag per submission is recommended).

Since there's only one form of testing being submitted, (and to my knowledge) only PHPUnit tests have been submitted to Codecov from WordPress organization repositories, I don't think we need to segment by unit, integration, etc.

The docs note that you can also use it to segment reports for multiple features. But we also do not do that currently anywhere that I'm aware of.

Suggested change
flags: unit
flags: ${{ matrix.multisite && 'multisite' || 'single' }}

token: ${{ secrets.CODECOV_TOKEN }}
15 changes: 15 additions & 0 deletions codecov.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
codecov:
notify:
require_ci_to_pass: yes
coverage:
status:
project:
default:
target: auto
threshold: 80%
base: auto
informational: true
patch:
default:
threshold: 80%
informational: true
2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@
"lint-js": "wp-scripts lint-js ./src",
"lint-php": "wp-env run cli --env-cwd=wp-content/plugins/$(basename $(pwd)) composer lint",
"test-php": "wp-env run tests-cli --env-cwd=wp-content/plugins/$(basename $(pwd)) vendor/bin/phpunit -c phpunit.xml.dist --verbose",
"test-php-coverage": "wp-env run tests-cli --env-cwd=wp-content/plugins/$(basename $(pwd)) vendor/bin/phpunit -c phpunit.xml.dist --verbose --coverage-clover build/logs/php-coverage.xml",
Copy link

@justlevine justlevine Feb 4, 2026

Choose a reason for hiding this comment

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

Barely matters here since we only need this until the (hopeful) core merge, but to help future folks looking for prior art (and the humans/agents dealing with cognitive load), could we move the configs into an implementation detail inside phpunit.xml.dist instead of an explicit npm command (or two)

Refs from WordPress/ai:

Copy link
Member Author

@aaronjorbin aaronjorbin Feb 10, 2026

Choose a reason for hiding this comment

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

As I mentioned in the description, this uses the same implementation as plugin check, so there shouldn't be any worry about new prior art.

Copy link

@justlevine justlevine Feb 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure I followed. I'm saying that the 2-year-old config used by plugin-check is suboptimal and dated, which is why we went with a different pattern in the other Core AI Building blocks (WordPress/ai is the above example, but MCP Adapter and the now archived abilities-api also follow this pattern.)

In the very nit-scenario where someone is scaffolding a canonical plugin and chooses to use this plugin as prior art, they're not going to notice that you referenced plugin check in the PR description, and they're probably not going to realize that even though the command is recently committed, it's not following the conventions used elsewhere.

(Unless your goal was just to generate the codecov to get an understanding for the merge proposal, and not merge this PR in? In that case ya then it really doesn't matter)

Copy link
Member Author

@aaronjorbin aaronjorbin Feb 10, 2026

Choose a reason for hiding this comment

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

You expressed worry about this adding new prior art that can increase cognitive load, I'm pointing out that this is not new prior art.

I don't think that explicit actions are "suboptimal and dated", can you explain why you think it is?

My goal is to get the coverage in the short term for helping make the decision on the merge proposal and if this repo continues to be used, to surface coverage information on code changes so that can inform decision making in the long term.

Copy link

@justlevine justlevine Feb 11, 2026

Choose a reason for hiding this comment

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

You expressed worry about this adding new prior art that can increase cognitive load, I'm pointing out that this is not new prior art.

Okay so to clarify my concern is recency bias and cross-repo maintainer friction within the Core AI team, not about the novelty of a specific implementation pattern in the WordPress org. This plugin as a whole serves as prior art and despite being "newer" than the other core-ai repos that iterated with intentionality on older patterns, it's reaching for a pattern written 3 years well before guidelines for WordPress/* hosted projects were released. IOW

I don't think that explicit actions are "suboptimal and dated", can you explain why you think it is?

Step before the explicit actions, the first inefficiency is that we a rely on a configuration that isn't codified in config, but passed as an argument. We lose config schema linting, portability/reusability, overloadability via a local .xml or script flags, increased possibility for typos, etc.

Next the specific script implementations:

  • It doesn't follow the conventions of our other tooling/test scripts, that all keep their configs inside the config files.
  • It's misleading: npm run test-php-*coverage won't actually generate coverage unless you've started wp-env with xdebug.
  • It scales poorly at n*2. a 3rd suite becomes 6 commands to juggle between, a 4th 8 etc.

I'm sure there were others discussed at the time we were scaffolding the other repos, but those are what pop to mind. Anecdotally I've seen both AI and human contributors struggle to intuit how to write unit tests.

Whereas:

  1. npm run wp-env start -- --xdebug
  2. `npm run test:php:{suite} -- --{whatever flags }

Is shorter, requires knowledge of fewer commands, more self documenting, overloadable, creates no overhead, and general has fewerr opportunities for user ever.

Now sure, this might not reach "death by 1000 papercuts" levels of inefficiency, or warrant a change on an existing project, but it was an intentional and considered iteration that took into account older patterns from plugin-check, performance, and more recent decisions like WordPress/two-factor#717 (which is what I meant by "dated": internally we have more recent iterations . In lieu of cross-silo communication, recency bias is one of the few reliable ways to communicate change. If project quality well governed - as many parts of WordPress are - it's usually safe to assume that gatekeepers have discussed and considered not just the Chesterton Fences behind previous config decisions, but the cost/benefit of deviating from prior art. Like we're doing now 😅)

if this repo continues to be used

Ironically, in this case I care less about this, because we can course correct in a follow-up PR. My concern here is scoped to this getting merged, and the plugin being archived shortly after, where the files exist in the diff and a 2026 commit date, but you gotta go spelunking to see that this is not the iteration that the team's other projects and canonical plugins are following. Most folks are just likely to replace the includes and reuse the scaffold with a few search-replaces and no second glance.

Copy link
Member

@desrosj desrosj Feb 13, 2026

Choose a reason for hiding this comment

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

I don't have strong opinions here. I do kind of like the idea of putting as much as possible inside the phpunit.xml.dist configuration file.

I seem to recall that there was a reason why specifying these things in the command line was preferable. I looked back in Core Trac but couldn't find what I'm thinking of. In r59356, the local Docker environment was updated to automatically enable xDebug when the intention seemed to be generating a coverage report. This can save contributors time figuring out why their report is not being created, but that does not make use of wp-env.

It's always possible someone uses the wrong code as an example. In my opinion, we should create a template repository for someone to use when building out provider plugins (which I believe is what you suggesting would be created using this plugin as a building block) that has all of the preferred best practices at any given point in time.

"test-php-multisite": "wp-env run tests-cli --env-cwd=wp-content/plugins/$(basename $(pwd)) vendor/bin/phpunit -c tests/phpunit/multisite.xml --verbose",
"test-php-multisite-coverage": "wp-env run tests-cli --env-cwd=wp-content/plugins/$(basename $(pwd)) vendor/bin/phpunit -c tests/phpunit/multisite.xml --verbose --coverage-clover build/logs/php-coverage.xml",
"wp-env": "wp-env"
}
}
Loading