The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: westonruter <[email protected]>
Co-authored-by: thelovekesh <[email protected]>
Co-authored-by: felixarntz <[email protected]>
Co-authored-by: mukeshpanchal27 <[email protected]>
Co-authored-by: swissspidy <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@felixarntz The use case is very common for me. To prevent getting blocked and to avoid overly-large pull requests, I open sub-PRs. In this case, it is very useful to have all checks run as if the PR was opened against trunk or a feature branch.

What other random PRs do you have in mind?

@westonruter

@felixarntz The use case is very common for me. To prevent getting blocked and to avoid overly-large pull requests, I open sub-PRs. In this case, it is very useful to have all checks run as if the PR was opened against trunk or a feature branch.

Makes sense. In an ideal world, such sub PRs wouldn't be necessary, but I agree they're sometimes better than getting blocked. When you say "sub-PR", this still means the PR will eventually be reviewed against the "real" base branch though correct? As far as I understand, you only create those PRs to be able to continue working while the PRs that have to be merged first are still waiting for review/approvals. Is that correct? Something I'd like to avoid for instance is that those "sub-PRs" would be the foundation for actual review processes even before the previous PR gets merged. That could get messy very easily. For review, it's always better to have the previous work merged so that the actual changes are obvious to review.

What other random PRs do you have in mind?

Nothing really, with "random PRs" I just meant any PRs that are not against trunk or feature branches :)

When you say "sub-PR", this still means the PR will eventually be reviewed against the "real" base branch though correct? As far as I understand, you only create those PRs to be able to continue working while the PRs that have to be merged first are still waiting for review/approvals. Is that correct?

That is correct.

In terms of the workflows, I think some workflows are better suited for once the PR is actually ready for review (e.g. the label/milestone check, I don't see its value on a PR that's effectively not ready for review anyway). As in:

• Let's say you have PR branch a against feature/something. Good to review.
• While waiting for feedback on a, you create branch b and open a PR from b against a. At this point, having a few basic things like linting and unit testing is probably still helpful. Broader things like standalone plugin testing or correct label usage though seems overkill.
• At some point the branch from a against feature/something gets merged.
• This also means the PR for b can be updated to be against feature/something. At this point it's ready for full review, and the complete suite of workflows makes sense to run.

LMKWYT.

@felixarntz OK, I've reverted the changes to the pr-validation and php-test-standalone-plugins workflow.

TBH, I hadn't before looked deeply at npm run test-php versus npm run test-php-plugins, except that I ran the latter once when I intended to run the former. I'm not totally clear yet on what standalone plugin testing is doing. Is it truly just doing this one single assertion for each plugin?

It seems there would be a much simpler way to go about testing for a fatal, and that would be to not use PHPUnit at all but rather to:

1. Install all of the plugins
2. Activate each plugin one-by-one using WP-CLI
3. When each plugin is active, do a curl on the homepage to make sure it returns a 200 status code and </html>.

In the future, it would be better to use Playwright to do some actual E2E testing rather than a simple curl, like @swissspidy has blogged about.

As it stands right now, the existing standalone tests appear to be broken. As seen in this job:

Error copying plugin "undefined": ENOENT: no such file or directory, lstat './plugins/undefined/'
Error copying plugin "undefined": ENOENT: no such file or directory, lstat './plugins/undefined/'
Error copying plugin "undefined": ENOENT: no such file or directory, lstat './plugins/undefined/'
Error copying plugin "undefined": ENOENT: no such file or directory, lstat './plugins/undefined/'

Nevertheless, the jobs are passing. So the php-test-standalone-plugins workflow is currently wasting resources.

Thanks, @westonruter, for the PR. I had it on my list to open a PR, but you beat me to it.

The standalone tests break after the changes made in #1011. We should change the condition here https://github.com/WordPress/performance/blob/trunk/bin/plugin/commands/build-plugins.js#L29 so that it with skip the module part if we don't have module lists.

I will open a PR soon to address this issue. The tests should not work for modules but should work for plugins only. Both scripts should be updated accordingly.

It seems there would be a much simpler way to go about testing for a fatal, and that would be to not use PHPUnit at all but rather to:

@westonruter I think that test is just a dummy so that we have at least 1 test there. It's not really trying to detect fatal errors or anything.

I think that test is just a dummy so that we have at least 1 test there. It's not really trying to detect fatal errors or anything.

@swissspidy Right, I didn't mean that somehow assertTrue( true ) could possibly fail the assertion. Indeed, PHPStan flags it as being an error:

phpstan: Call to method PHPUnit\Framework\Assert::assertTrue() with true will always evaluate to true.

It seems PHPUnit isn't the right tool for the job here if we're just wanting to check for a plugin fatal error.

It seems PHPUnit isn't the right tool for the job here if we're just wanting to check for a plugin fatal error.

Filed: #1051