Add Test Infrastructure #44
Conversation
jjgrainger
added
[Type] Enhancement
mukeshpanchal27
left a comment
mukeshpanchal27
left a comment
There was a problem hiding this comment.
Thanks @jjgrainger Great start. The PR feedbacks address below mention issue.
- One difference compared to the Performance Lab plugin is I had to include a
composer installstep in thephp-test.ymlworkflow file. When not included, there is aError: The PHPUnit Polyfills library is a requirement for running the WP test suite.(see action logs
In Performance Lab, It was introduce on WordPress/performance@5a8a5c2 commit.
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@jjgrainger Left a few minor comments, mostly nit-picks. It would be good to add a multisite test setup though.
.wp-env.json
Outdated
| "phpVersion": "7.4" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I think the indentation here is incorrect, it needs to follow https://github.com/10up/plugin-check/blob/trunk/.editorconfig#L23.
package.json
Outdated
| "wp-env": "wp-env", | ||
| "lint-php": "wp-env run composer run-script lint", | ||
| "pretest-php": "wp-env run composer 'install --no-interaction'", | ||
| "test-php": "wp-env run phpunit 'phpunit -c /var/www/html/wp-content/plugins/plugin-check/phpunit.xml.dist --verbose'" |
There was a problem hiding this comment.
Should we add a multisite configuration as well (see e.g. https://github.com/WordPress/performance/blob/trunk/package.json#L30)? I would think this is just as relevant here.
tests/basic-tests.php
Outdated
| * | ||
| * @test | ||
| */ | ||
| public function it_should_assert_true() { |
There was a problem hiding this comment.
Nit-pick to keep in mind for the future:
- WordPress core uses the
test_*method prefix as a convention. - At some point in Performance Lab we started doing this
it_should_*convention, and in that plugin it is for now mixed between the two conventions. However I would advise we stick with the core approach. - Sticking with that convention also means we can avoid the
@testannotation which is redudant. Usually test methods don't need a doc-block, unless they use a@dataProvideror benefit from any extra explanation. Descriptive method names plus inline comments within the test method are usually sufficient to explain what's going on.
This isn't a big issue here since this is obviously just a dummy method, but wanted to point out for future reference. Since there are a few minor other changes to make, maybe let's also rename this to e.g. test_true() and remove the doc block?
There was a problem hiding this comment.
Agreed. I have raise the issue in PL GH about it WordPress/performance#514
mukeshpanchal27
left a comment
mukeshpanchal27
left a comment
There was a problem hiding this comment.
Thanks @jjgrainger Left one nit-pick feedback and question.
mukeshpanchal27
left a comment
mukeshpanchal27
left a comment
There was a problem hiding this comment.
Great work @jjgrainger, LGTM.
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@jjgrainger Thanks, looks great!
Add initial test infrastructure and workflows.
composer installstep in thephp-test.ymlworkflow file. When not included, there is aError: The PHPUnit Polyfills library is a requirement for running the WP test suite.(see action logs)Closes #2