-
Notifications
You must be signed in to change notification settings - Fork 68
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
Add CI workflow that runs only changed E2E tests, and succeeds when no tests have changed #10233
base: develop
Are you sure you want to change the base?
Conversation
…o tests have changed
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
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 working on this @tpaksu !
I asked some questions related to the changes.
Also, are we going to consider running the actual E2E tests only if the only-changed
tests passed (similar to how smoke tests work) or we would consider that a b-plan?
@@ -41,6 +41,7 @@ | |||
"test:e2e-pw-update-snapshots": "npm run test:e2e-pw -- --update-snapshots", | |||
"test:e2e-pw-ui": "./tests/e2e-pw/test-e2e-pw-ui.sh", | |||
"test:e2e-pw-ci": "npx playwright test --config=tests/e2e-pw/playwright.config.ts --grep-invert @todo", | |||
"test:e2e-pw-ci-only-changed": "npx playwright test --only-changed=origin/develop --pass-with-no-tests --config=tests/e2e-pw/playwright.config.ts --grep-invert @todo", |
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.
The only-changed
flag points to origin/develop
, but the workflow will run when the PR targets develop
or trunk
. Should we keep 2 test:e2e-pw-ci-only-changed
npm scripts, one for trunk and the other one for develop so that we could choose one or another depending on the branch targeted by the PR?
E2E_WP_VERSION: 'latest' | ||
E2E_WC_VERSION: 'latest' |
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.
The E2E tests - All
workflow runs tests against different versions of WP and WC (7.7.0, latest and beta, for example). Would we want to consider this in this workflow? There will be times when tests will pass against latest but fail against beta, like with the Coming Soon mode introduced in the latest WC release.
@@ -6,6 +6,9 @@ on: | |||
- develop | |||
- trunk | |||
workflow_dispatch: | |||
pull_request_review: | |||
types: | |||
- submitted |
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.
What would happen if E2E tests are run as usual and then someone adds a new review? Would the tests trigger again?
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.
Yes, you're right, pull_request.ready_for_review
action seems like the correct event that should trigger this.
Edit: It ran again when I added an answer on the main thread :D Confirmed this.
@alopezari thanks for the review, I was focused on another issue, so I couldn't put my eyes on this earlier.
I think this is a good candidate to be in this PR, but there's one thing that makes me have second thoughts of doing that way; when I changed something in the dependent file, it ran 114 tests out of 123, and it made me wonder, if it only picked the method that was changed to define the changed tests, or did it run all tests that its file imported that changed dependency file. If it's the second, then it will nearly be a double run, right? And if the E2E workflow took 40 minutes to complete, it would take 80 minutes for someone to be able to merge a PR 😱 That's why I'd prefer running a single workflow related to the PR's draft status. I'll answer the other questions in their respective threads. |
Thanks for the explanation @tpaksu! I think I don't fully understand it though. With my suggestion, I meant:
As it is right now, it:
What's the difference between both approaches that would make us double the |
With the suggested approach:
A change of a helper function may cause 100+ tests to run in the "only changed" workflow. Right? And once it passes, it'll start the next 100+ tests workflow. So it will take 40 minutes for the only changed workflow, and after that, another 40+ minutes for the full test workflow. |
Right, that might be problematic. However, the same thing would happen with the current scenario, right? It would take 40 mins for the only-changed workflow in draft mode + 40 with the full test run. The only difference I can see is that with the current approach we can control when we want to run the only-changed workflow by setting the PR in draft mode. Hmm a potential solution could be running the only-changed tests if there is any change in the tests directory. However, the only reason for advocating for that approach is to provide a better developer experience when running the only-changed tests, as the process would be smoother than making sure everyone knows they can put the PR on draft to run the tests, etc. But if making this smoother gets tricky, I'm happy with the current approach as it is right now 👍 |
Nope, we will only run the "only changed" ones when the PR is in draft mode, and when it's marked as "not draft", it will only run the full test suite. So it will be max 40 when in draft if all tests are affected, and not in draft, the regular run. |
Yep, that's what I mean: 40m if all tests are affected (in draft mode) + 40m regular run (in review mode). The alternative approach would be 40m if all tests are affected + 40m regular run (all in review mode and the regular run running only if the initial - It would take the same time, isn't it? There are some differences though:
I tried to gather all thoughts on this message to make sure we're not missing any details and we're aligned on how the understanding of both approaches. Please let me know if I missed anything! Personally, I'm happy with both approaches. Both of them have pros and cons, and I'm not strongly opinionated on anyone in particular, so I'll support whatever you decide 👍 |
@alopezari Nevermind, I guess I'm going to close this PR as we may break the tests in different workflows per scope (merchant, shopper, subscriptions, etc) in different containers, so the time will be shorter, and it'll be more like what it was with Puppeteer. |
Context: p1737719556299039-slack-C01B8KNUYSW
Changes proposed in this Pull Request
Testing instructions
This PR adds a workflow that runs only the modified E2E tests in it, helping us see the errors faster (~5-10 minutes) without waiting for all tests to complete (Which takes around 35-40 minutes to complete now, before optimizations). Also updates Playwright to 1.50, since the
--only-changed
option was added in v1.46, and we were using v1.43.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