Introduce integration tests and run them in GH actions #313

enricobattocchi · 2023-06-29T17:14:52Z

Context

We want to introduce integration tests into the repository and run them on GitHub Actions.

Summary

This PR can be summarized in the following changelog entry:

Adds integration tests to the plugin.

Relevant technical choices:

Moved the unit tests to tests/unit, also adapting the namespace of the classes
PHPCS configuration has been updated to prevent CS errors in bootstrap.php
A very basic integration test has been added as a proof of concept to check that everything is working as expected.

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

Check that the jobs are passing, and that they now include integration tests on all the supported PHP versions.

Relevant test scenarios

Changes should be tested with the browser console open
Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies
Changes should be tested on different editors (Block/Classic/Elementor/other)
Changes should be tested on different browsers
Changes should be tested on multisite

Test instructions for QA when the code is in the RC

QA should use the same steps as above.

QA can test this PR by following these steps:

N/A

Impact check

This PR affects the following parts of the plugin, which may require extra testing:

UI changes

This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Documentation

I have written documentation for this change.

Quality assurance

I have tested this code to the best of my abilities
I have added unittests to verify the code works as intended

Innovation

No innovation project is applicable for this PR.
This PR falls under an innovation project. I have attached the innovation label and noted the work hours.

Fixes #

coveralls · 2023-06-29T17:20:15Z

Pull Request Test Coverage Report for Build 6339936357

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+3.3%) to 48.802%

Totals
Change from base Build 6319529401:	3.3%
Covered Lines:	1100
Relevant Lines:	2254

💛 - Coveralls

jrfnl

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 build name.
And the WP version should also be part of the COVERALLS_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 the config and the tests directory are export-ignored 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).

.phpcs.xml.dist

tests/integration/bootstrap.php

tests/unit/bootstrap.php

tests/integration/post-duplicator-test.php

enricobattocchi · 2023-07-05T21:34:07Z

@jrfnl Thanks for the feedback! I think I addressed all your suggestions.

jrfnl

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 to tests/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: the phpunit-integration.xml.dist file needs to be added (x2).
- deploy.yml workflow: no changes needed.
- lint.yml workflow: the phpunit-integration.xml.dist file needs to be added (x2).
- test.yml workflow: the config/scripts/install-wp-tests.sh file needs to be unexcluded as the config/** entry means that the workflow would not be triggered to run when the config/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).

phpunit-integration.xml.dist

.phpcs.xml.dist

jrfnl · 2023-09-27T00:20:19Z

.phpcs.xml.dist

 	<!-- 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>


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 ?

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.

jrfnl · 2023-09-27T00:28:26Z

tests/integration/bootstrap.php

+$_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/';
+}


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

Not sure if was encountering problems in the beginning, but by removing the if everything works as expected. Pushed.

jrfnl · 2023-09-27T00:28:54Z

tests/integration/bootstrap.php

+if ( ! defined( 'YOAST_DUPLICATE_POST_TEST_ROOT_DIR' ) ) {
+	define( 'YOAST_DUPLICATE_POST_TEST_ROOT_DIR', __DIR__ . '/' ); // Includes trailing slash.
+}


Why is this constant needed ?

As above, it turned out it was not needed.

tests/integration/bootstrap.php

tests/integration/post-duplicator-test.php

jrfnl · 2023-09-27T00:50:07Z

.github/workflows/test.yml

+      - 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


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).

Example: https://github.com/Yoast/wordpress-seo/blob/384ec628f4560b0f896c30999276442ed953bb73/.github/workflows/test.yml#L237-L255

jrfnl · 2023-09-27T00:52:32Z

.github/workflows/test.yml

+      - 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


This one will also need to be doubled, so the code coverage results for multi-site are also uploaded.

Example: https://github.com/Yoast/wpseo-news/blob/d9f573d8bfb2dc81fcf885d828f2385b0abe8266/.github/workflows/test.yml#L267-L281

Done, coveralls now displays the multisite results.
I also added the -wp-${{ matrix.wp_version }} part

jrfnl · 2023-09-27T01:07:48Z

tests/integration/bootstrap.php

+$GLOBALS['wp_tests_options'] = [
+	'active_plugins' => [
+		'duplicate-post/duplicate-post',
+	],
+];


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

.phpcs.xml.dist

…o 6.2

Co-authored-by: Juliette <[email protected]>

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.

…ootstrap.php`

enricobattocchi · 2023-09-28T14:02:02Z

In light of our recent discussion about this, can the tests/integration directory (and the namespace used) be updated to tests/wp ?

Done .

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.

Done.

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: the phpunit-integration.xml.dist file needs to be added (x2).

deploy.yml workflow: no changes needed.

lint.yml workflow: the phpunit-integration.xml.dist file needs to be added (x2).

test.yml workflow: the config/scripts/install-wp-tests.sh file needs to be unexcluded as the config/** entry means that the workflow would not be triggered to run when the config/scripts/install-wp-tests.sh file changes, while it should (x2).

Done.

enricobattocchi added the changelog: non-user-facing label Jun 29, 2023

enricobattocchi force-pushed the feature/integration-tests branch from ecee076 to 056ea17 Compare June 29, 2023 17:17

enricobattocchi force-pushed the feature/integration-tests branch from 056ea17 to 3aa161e Compare June 29, 2023 17:32

enricobattocchi changed the title ~~WIP Feature/integration tests~~ Introduce integration tests and run them in GH actions Jun 29, 2023

enricobattocchi marked this pull request as ready for review June 29, 2023 18:50

enricobattocchi requested a review from jrfnl June 29, 2023 18:50

jrfnl requested changes Jun 30, 2023

View reviewed changes

.phpcs.xml.dist Outdated Show resolved Hide resolved

tests/integration/bootstrap.php Outdated Show resolved Hide resolved

tests/unit/bootstrap.php Outdated Show resolved Hide resolved

tests/integration/post-duplicator-test.php Outdated Show resolved Hide resolved

enricobattocchi force-pushed the feature/integration-tests branch from c7b1bcd to 73a4db9 Compare August 30, 2023 20:40

jrfnl requested changes Sep 27, 2023

View reviewed changes

enricobattocchi self-assigned this Sep 28, 2023

enricobattocchi commented Sep 28, 2023

View reviewed changes

.phpcs.xml.dist Outdated Show resolved Hide resolved

enricobattocchi and others added 12 commits September 28, 2023 10:26

Move unit tests to unit folder

f9a90c4

Add integration tests folder with a proof-of-concept test

a46895a

Introduce composer commands for integration tests

f555197

Add integration tests to GH actions

5db1668

Include WP version in GH actions matrix

7d3eb40

Add integration tests config to .gitignore

c3f84a7

Fine-tune the exclusions in CS config

278c1d2

Use a cleaner syntax to travel 2 dirs up

2d56c0a

Remove unnecessary leftovers from Video SEO bootstrap

e8af90a

Increase coverage by testing the case where we copy the post date

0efcb90

Adapt integration tests for minimum supported WordPress version set t…

279d68d

…o 6.2

Apply suggestions from code review

d78fd53

Co-authored-by: Juliette <[email protected]>

enricobattocchi force-pushed the feature/integration-tests branch from 63e5f82 to d78fd53 Compare September 28, 2023 08:27

enricobattocchi added 4 commits September 28, 2023 13:34

Remove exclusion for the InvalidFunctionsFileName sniff after chang…

1ba2495

…es to `bootstrap.php`

Run integration tests for multisite in GH actions and upload results …

b4455cd

…to Caoveralls Also includes a renaming of the flag for the single site case so the WP version is also included.

Add phpunit-wp.xml.dist to the lists of files that should not trigg…

011edb9

…er CS checks and linting when changed.

Enforce running tests when the install-wp-tests.sh script changes

4111485

enricobattocchi force-pushed the feature/integration-tests branch from 0e92504 to 4111485 Compare September 28, 2023 13:39

Remove exclusion for the PrefixAllGlobals sniff after changes to `b…

4c62e97

…ootstrap.php`

jrfnl approved these changes Oct 4, 2023

View reviewed changes

jrfnl merged commit 4642e1c into trunk Oct 4, 2023
42 checks passed

jrfnl deleted the feature/integration-tests branch October 4, 2023 08:04

This was referenced Oct 4, 2023

PHPCS: minor tweak #324

Merged

Tests: consistency in terminology #325

Merged

GH Actions: only run "coveralls finished" once all tests have finished running #326

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce integration tests and run them in GH actions #313

Introduce integration tests and run them in GH actions #313

enricobattocchi commented Jun 29, 2023 •

edited

Loading

coveralls commented Jun 29, 2023 •

edited

Loading

jrfnl left a comment

enricobattocchi commented Jul 5, 2023

jrfnl left a comment •

edited

Loading

jrfnl Sep 27, 2023

enricobattocchi Sep 28, 2023

jrfnl Sep 27, 2023

enricobattocchi Sep 28, 2023

jrfnl Sep 27, 2023

enricobattocchi Sep 28, 2023

jrfnl Sep 27, 2023

enricobattocchi Sep 28, 2023

jrfnl Sep 27, 2023

enricobattocchi Sep 28, 2023

jrfnl Sep 27, 2023

enricobattocchi Sep 28, 2023

enricobattocchi commented Sep 28, 2023

Introduce integration tests and run them in GH actions #313

Introduce integration tests and run them in GH actions #313

Conversation

enricobattocchi commented Jun 29, 2023 • edited Loading

Context

Summary

Relevant technical choices:

Test instructions

Test instructions for the acceptance test before the PR gets merged

Relevant test scenarios

Test instructions for QA when the code is in the RC

Impact check

UI changes

Documentation

Quality assurance

Innovation

coveralls commented Jun 29, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6339936357

💛 - Coveralls

jrfnl left a comment

Choose a reason for hiding this comment

enricobattocchi commented Jul 5, 2023

jrfnl left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enricobattocchi commented Sep 28, 2023

enricobattocchi commented Jun 29, 2023 •

edited

Loading

coveralls commented Jun 29, 2023 •

edited

Loading

jrfnl left a comment •

edited

Loading