-
Notifications
You must be signed in to change notification settings - Fork 35
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.
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
Introduce integration tests and run them in GH actions #313
Conversation
ecee076
to
056ea17
Compare
Pull Request Test Coverage Report for Build 6339936357
💛 - Coveralls |
056ea17
to
3aa161e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I haven't tried it or done a detailed review. This is just some feedback based on a quick glance over.
- GH Actions workflow: for integration tests you should also provide the WP version to test against (otherwise the
continue-on-failure
doesn't make sense and the WP install won't work correctly without a version either).
Probably also a good idea to mention the WP version in the buildname
.
And the WP version should also be part of theCOVERALLS_FLAG_NAME
.
Have a look at the integraton test workflow for WPSEO as an example, if you like. - You may want to add the new config file to
.gitattributes
and a potential overload to the.gitignore
.
You may also want to check that theconfig
and thetests
directory areexport-ignore
d completely already and if not, add those to.gitattributes
too.
I won't have time for a detailed review for a while, but the first glance check looks good (with some small remarks).
@jrfnl Thanks for the feedback! I think I addressed all your suggestions. |
c7b1bcd
to
73a4db9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @enricobattocchi, so I've finally taken some time to do a detailed second review.
Aside from the inline remarks I've left, here are my other findings:
- In light of our recent discussion about this, can the
tests/integration
directory (and the namespace used) be updated totests/wp
? - The
phpunit-integration.xml.dist
file still needs to be added to the.gitattributes
file.
You may want to rebase first as I've just merged a few changes to the.gitattributes
file, which make it more readable and should actually make it easier to add the missing entry. - As the workflows in this repo use an exclusion based approach, the
paths-ignore
entries for most workflows will need to be updated to account for the new files.cs.yml
workflow: thephpunit-integration.xml.dist
file needs to be added (x2).deploy.yml
workflow: no changes needed.lint.yml
workflow: thephpunit-integration.xml.dist
file needs to be added (x2).test.yml
workflow: theconfig/scripts/install-wp-tests.sh
file needs to be unexcluded as theconfig/**
entry means that the workflow would not be triggered to run when theconfig/scripts/install-wp-tests.sh
file changes, while it should (x2).
- I've verified that no changes were needed in the
config/grunt/task-config/copy.js
file (AFAICS). ✔️
✅ I've also ran the test locally for both single-site and multi-site and found it working (providing the bootstrap is changed a little, see inline notes, including notes about the local phpunit-integration.xml
file).
.phpcs.xml.dist
Outdated
<!-- Valid usage: For testing purposes, some non-prefixed globals are being created, which is fine. --> | ||
<rule ref="WordPress.NamingConventions.PrefixAllGlobals"> | ||
<exclude-pattern>/tests/*/bootstrap\.php$</exclude-pattern> | ||
</rule> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this entry really needed ? Or does the entry on line 123-125 already cover this ?
If it's about the function, what about adding the namespace to the bootstrap file too ? Or maybe use a closure instead as it's a one-liner anyway ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't just the function (now moved to a closure) but the variables as well, but I was able to remove them after applying the other suggested changes.
tests/integration/bootstrap.php
Outdated
$_wp_tests_dir = WPIntegration\get_path_to_wp_test_dir(); | ||
if ( $_wp_tests_dir === false ) { | ||
$_wp_tests_dir = YOAST_DUPLICATE_POST_TEST_ROOT_DIR . '../../../../tests/phpunit/'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an anti-pattern as it looks like you are basically trying to do something which the WP Test Utils are supposed to do for you and it included a presumption about the local setup of the dev-user which is likely only correct for your own computer....
Why is this needed ? Are you having trouble running the tests locally ? In that case, try setting either of the following env variables in a local phpunit-integration.xml
overload file:
<php>
<env name="WP_TESTS_DIR" value="path/to/WP-Core/tests/phpunit/"/>
<env name="WP_DEVELOP_DIR" value="path/to/WP-Core/"/>
</php>
Also see: https://github.com/Yoast/wp-test-utils#bootstrap-utility-functions-and-custom-autoloader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if was encountering problems in the beginning, but by removing the if
everything works as expected. Pushed.
tests/integration/bootstrap.php
Outdated
if ( ! defined( 'YOAST_DUPLICATE_POST_TEST_ROOT_DIR' ) ) { | ||
define( 'YOAST_DUPLICATE_POST_TEST_ROOT_DIR', __DIR__ . '/' ); // Includes trailing slash. | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this constant needed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, it turned out it was not needed.
.github/workflows/test.yml
Outdated
- name: Run integration tests | ||
if: ${{ matrix.coverage == false }} | ||
run: composer integration-test | ||
|
||
- name: Run integration tests with code coverage | ||
if: ${{ matrix.coverage == true }} | ||
run: composer integration-coverage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These need to be doubled to have a setup to run the tests for multi-site (presuming you want that as multisite
is an entry in the matrix above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
.github/workflows/test.yml
Outdated
- name: Upload coverage results to Coveralls | ||
if: ${{ success() && matrix.coverage == true }} | ||
env: | ||
COVERALLS_REPO_TOKEN: ${{ secrets.COVERALLS_TOKEN }} | ||
COVERALLS_PARALLEL: true | ||
COVERALLS_FLAG_NAME: intgr-php-${{ matrix.php_version }} | ||
run: php-coveralls -v -x build/logs/clover-integration.xml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one will also need to be doubled, so the code coverage results for multi-site are also uploaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, coveralls now displays the multisite results.
I also added the -wp-${{ matrix.wp_version }}
part
tests/integration/bootstrap.php
Outdated
$GLOBALS['wp_tests_options'] = [ | ||
'active_plugins' => [ | ||
'duplicate-post/duplicate-post', | ||
], | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be needed if the _manually_load_plugin()
function is defined and hooked in.
You seem to be combining two different bootstrap pattern, resulting in unnecessary bootstrap steps/duplication.
See: https://github.com/Yoast/wp-test-utils#using-the-bootstrap-utilities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
Co-authored-by: Juliette <[email protected]>
63e5f82
to
d78fd53
Compare
This commit combines a number of fixes addressing Juliette's feedback, which couldn't be committed separately since they are pretty intertwined: - rename `phpunit-integration.xml.dist` to `phpunit-wp.xml.dist` and adjust references - add `phpunit-wp.xml(.dist)` to `.gitattributes` and `.gitignore` - move from Integration to WP namespace and folder - remove some parts of `bootstrap.php` which are not needed at all, and move `_manually_load_plugin` to a closure
…es to `bootstrap.php`
…to Caoveralls Also includes a renaming of the flag for the single site case so the WP version is also included.
…er CS checks and linting when changed.
0e92504
to
4111485
Compare
Done .
Done.
Done. |
Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
tests/unit
, also adapting the namespace of the classesbootstrap.php
Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
Relevant test scenarios
Test instructions for QA when the code is in the RC
QA can test this PR by following these steps:
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
UI changes
Documentation
Quality assurance
Innovation
innovation
label and noted the work hours.Fixes #