-
Notifications
You must be signed in to change notification settings - Fork 72
[E2E] Ensure version coverage for WooCommerce and PHP #11002
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
[E2E] Ensure version coverage for WooCommerce and PHP #11002
Conversation
This reverts commit 733ae96.
…nstead of WC_VERSIONS
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
Size Change: +45 B (0%) Total Size: 870 kB
ℹ️ View Unchanged
|
…age-for-woocommerce-and-php
Hi @adimoldovan I'd appreciate your review when you have a moment. Any feedback is welcomed, but I'm specially interested on your thoughts about the WooCommerce and PHP versions. Also, I don't have much experience working with the Github Actions so I may be doing something wierd 😅 Thanks in advance! |
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.
Pull Request Overview
This PR enhances E2E testing coverage by addressing gaps in PHP and WooCommerce version testing while introducing a dynamic version resolution strategy.
- Adds support for PHP 8.3 and 8.4 testing alongside existing PHP 7.3 support
- Introduces L-1 WooCommerce version testing to align with the L-1 support policy
- Implements dynamic version resolution using a new script that fetches current versions from WordPress.org API
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
.github/workflows/e2e-test.yml |
Updated to use dynamic matrix generation with selective PHP version testing strategy |
.github/workflows/e2e-pull-request.yml |
Enhanced to test both L-1 and latest WC versions with matrix generation |
.github/scripts/generate-wc-matrix.sh |
New script for dynamic WooCommerce version resolution and matrix generation |
.github/scripts/README.md |
Documentation for the new matrix generation strategy and scripts |
.github/actions/setup-php/action.yml |
Added support for PHP version override via E2E_PHP_VERSION environment variable |
tests/e2e/env/setup.sh |
Removed inline beta/RC version resolution logic (moved to dedicated script) |
changelog/dev-woopmnt-5249-e2e-ensure-version-coverage-for-woocommerce-and-php |
Added changelog entry |
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.
code lgtm
Code looks good, but I'm curious that there is a mention of WooCommerce 7.7.0 in the script that generates the matrix, but I don't see this version in the list of the E2E tests for this pull request. |
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.
Thanks for addressing the coverage gaps @mgascam.
I do have some suggestions on the entire approach, but this is something that could be addressed down the line in follow-ups, none of these should block this PR.
- I'm not seeing a good reason to have more e2e-test* yml files. Everything could be consolidated in a single workflow. In the end, they set up an environment and run some tests based on some env variables.
- I liked that you extracted part of bash logic into a script. This script could be extended though, and return a full matrix with all the required information for the test jobs to run.
- It's hard and confusing to figure out what runs and when. I suggest having a json config for all jobs that should, either separate in .github, or in package.json. This config should be parsed and the matrix should be generated from it. Here's an example for WooCommerce monorepo using package.json, and one in Jetpack monorepo, using a single config. Both offer a good view of what should run, and an easy way to maintain the matrix.
I'll be happy to help with all of the above, please reach out if you need.
…age-for-woocommerce-and-php
7ed41b6
to
76a9f98
Compare
76a9f98
to
5dca86d
Compare
…age-for-woocommerce-and-php
Hi @Automattic/gamma folks 👋 , requesting a new review after the latest updates:
Thanks! |
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.
@mgascam - I would like to review this PR as I worked on a few E2E fixes last week. However, this PR is blowing up its original scope, focusing on ensuring the coverage for Woo and PHP versions. There are even changes in the dispute
production component, which should not be here. All of these make a proper review for this PR difficult.
I suggest this PR should narrow down its focus back to the original scope, and then you can have other PRs for these new changes #11002 (comment)
Thanks for the feedback, @htdat. You’re right about the scope growing beyond the original intent. For context, my priority was to avoid introducing changes that could harm the stability of the pipeline, especially now that it’s running more smoothly in
Agreed 🤝 Now that I’m more confident about how to fix the flaky tests (including the disputes one you mentioned), I’m happy to split and reorganize these changes as you suggested. The only trade-off is that there may be some “noise” from failing CI runs until the follow-up PRs addressing the flakiness are merged. |
…age-for-woocommerce-and-php
I split this PR as mentioned in the above comment:
Please make sure to add any further reviews there. Thanks in advance. |
Fixes #WOOPMNT-5249
After reviewing the state of E2E testing (pcreKM-3yi-p2) we identified some version coverage gaps:
This change aims to cover those gaps by introducing new test jobs, while keeping in mind the impact to the time it takes to run the workflows.
PHP versions
The current E2E testing strategy is updated to address PHP version coverage gaps:
Please note that we won't be testing each WC version using every PHP version because that would introduce too many jobs, hindering the time it takes to run them. Instead I'm using a "smart" approach to keep the number of jobs to a minimal without sacrificing too much testing coverage.
WooCommerce versions
I'm adding new jobs to test with L-1 WooCommerce version, to align with this recent announcement pcShBQ-3pc-p2#comment-5118. This is the only change affecting the WC versions used in the tests.
About "beta" and "rc" versions
Please note that "beta" and "rc" versions are only included in the matrix when available. While working with the script to fetch the versions from
https://api.wordpress.org/plugins/info/1.0/woocommerce.json
I noticed there is no "beta" version currently available for the next version 10.1.0. I believe this is expected but thought it was worth mentioning it. Also I realized that the current workflow is using a 9.9.0-beta.1 but I think the correct beta to use should be 10.1.0-beta.x (if it exists). This inconsistency would be fixed by this PR.Job matrix comparison
Changes proposed in this Pull Request
.github/workflows/e2e-pull-request.yml
: Added L-1 testing to PR workflow and include PHP in matrix..github/actions/setup-php/action.yml
: supportE2E_PHP_VERSION
override..github/scripts/generate-wc-matrix.sh
: New dynamic version resolution script. This is heavily inspired by @marcinbot's E2E - fix pulling non-latest WC versions, set up tests with rc #10990.github/workflows/e2e-test.yml
: The main branch workflow is updated to:.github/actions/e2e/run-log-tests/action.yml
: fix “re-run failed specs” logicTesting instructions
npm run changelog
to add a changelog file, choosepatch
to leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge