[WooPay] Increase the test coverage around the "Save my info" component

[a-danae]

Description

Multiple problems have been introduced around the "Save my info" component for WooPay. The goal of this issue is to reduce the chances of introducing these problems by increasing the automated test coverage for it. This could be unit tests or e2e, whatever makes more sense for the cases we want to cover.

Let's use this issue as a starting point. We can create follow-up issues as convenient.

List of issues we've had:

• WooPay can duplicate the "Save my info" element and AJAX requests on the Checkout Block while Stripe for WooCommerce is active #9410
• WooPay user registration flow broken on v8.3.0 on shortcode checkout #9552
• Cannot place order on blocks checkout when buying a subscription while WooPay pre-checked is enabled #9449 (Required a patch release)
• Fixed WooPay opt-in checks that would fail with a hidden message when the customer selected a payment method other than Credit Card #9210

Acceptance criteria

• The scenarios worth covering with automated tests and the best testing approach for them are listed.
• Each scenario is covered by automated tests, either unit or e2e.

[malithsen]

@a-danae I've been working on adding new e2e tests to WooPayments to cover the functionality of the save my info component.
I just realized that most of its functionality is covered in the existing e2e tests in WooPay.

However, these e2e tests are defined in the WooPay repository and always use the trunk of the WooPayments client repository.
Instead of writing new e2e tests in WooPayments for WooPay, I think we should

1. Update WooPay's E2E Manual Dispatch action to work on a given WooPayments branch.
2. Introduce a new GH Action to WooPayments so that it adds a comment to the PR, if it detects changes to WooPay related files asking the author to trigger the e2es in WooPay.
3. Update WooPay e2e tests to check for things that caused the bugs you listed in the description.

This allows us to keep the WooPay e2es consolidated in WooPay repository avoiding duplicate tests.

[malithsen]

Okay, looking into the above approach a little more, it's not as straightforward as I thought, but I still think it's the right one.

WooPay E2E's uses a built version of the WCPay plugin available under releases.
So in step 2 above,
i) we need to upload a build of WooPayments as an artificial.
ii) Print the artifact ID as a comment in the PR.
iii) PR author should now trigger this e2e via Manual Dispatch in WooPay providing the artifact ID.
iv) WooPay e2es should fetch the above artifact when setting up the merchant store.

I am not super familiar with WooPay e2es and GH Actions. @rafaelzaleski Do you see any issues in the above approach?

[a-danae]

Thanks for diving into this @malithsen! Fully agree with an alternative that lets us reuse existing tests. We'll want to reduce duplicates for sure.

Checking if I'm understanding things correctly. Running our existing e2e tests against a WooPayments version with the issues would catch them, right? Right now we're only using trunk.

If that's correct, why weren't these issues flagged? All of them reached production, meaning they were already in WooPayments' trunk and we still missed them, no?

Excluding #9449, for which the tests were indeed failing. Mentioned in Alefe's PR comment of Sep 19 2929-gh-Automattic/woopay and in Slack p1726687198726969/1726255204.252129-slack-C022WMN88KG

[malithsen]

Checking if I'm understanding things correctly. Running our existing e2e tests against a WooPayments version with the issues would catch them, right? Right now we're only using trunk.

As you've already noted, with the existing e2e tests, we'd not have caught all of the recent issues associated with the save-my-info component. So there are definitely some gaps we need to fill.

If that's correct, why weren't these issues flagged? All of them reached production, meaning they were already in WooPayments' trunk and we still missed them, no?

These tests run against the most recent release and needs to be manually triggered.

[rafaelzaleski]

Okay, looking into the above approach a little more, it's not as straightforward as I thought, but I still think it's the right one.

WooPay E2E's uses a built version of the WCPay plugin available under releases.
So in step 2 above,
i) we need to upload a build of WooPayments as an artificial.
ii) Print the artifact ID as a comment in the PR.
iii) PR author should now trigger this e2e via Manual Dispatch in WooPay providing the artifact ID.
iv) WooPay e2es should fetch the above artifact when setting up the merchant store.

I am not super familiar with WooPay e2es and GH Actions. @rafaelzaleski Do you see any issues in the above approach?

I don’t think the WooPay repo will be able to access artifacts in the WooPayments repo. Also, the artifact ID argument in the manual dispatch is for the WooPay build, not WooPayments. The use case for the artifact ID is when you want to run tests with different plugin versions but use the same WooPay build from a previous test execution. This saves setup time, as the new execution doesn’t need to rebuild the plugin. However, that’s the only benefit.

To test with a specific branch of WooPayments, you’ll need to run the tests locally and modify the WCPAY_CLIENT_BRANCH in the e2e environment variables, or add a new argument for the manual dispatch with the branch name and use it to populate this environment variable in the CI job.

Alternatively, you can wait until it’s included in a WooPayments release (even a test release) and pass the release tag as an argument to the manual dispatch job.

[malithsen]

Thanks @rafaelzaleski

I don’t think the WooPay repo will be able to access artifacts in the WooPayments repo. Also, the artifact ID argument in the manual dispatch is for the WooPay build, not WooPayments.

Agreed. It seems like we can download the artifacts via the GitHub API. We already seem to use GH API in gh-helper.sh. I am wondering if we can update the WooPay to download the artifact when setting up e2es, instead of downloading a release version, so that we use a build of the branch.

To test with a specific branch of WooPayments, you’ll need to run the tests locally and modify the WCPAY_CLIENT_BRANCH in the e2e environment variables

Testing locally sounds like a good alternative but introduces some friction if you don't have e2es setup locally. And in my experience, local e2es can be a bit flakey compared to running in a more stable environment.

or add a new argument for the manual dispatch with the branch name and use it to populate this environment variable in the CI job.

Not sure I understand this step correctly. This would involve cloning the WooPayments repo and building it? If I am reading things correctly, right now, we use the already-built WooPayments available under releases.

Alternatively, you can wait until it’s included in a WooPayments release (even a test release) and pass the release tag as an argument to the manual dispatch job.

This is also a possibility. In this approach, the PR author must wait until the weekly release is created and remember to trigger manual dispatch workflow.
Perhaps we can automate WooPay e2es to match the cadence of WooPayments releases and use the latest test build.

[malithsen]

Perhaps we can automate WooPay e2es to match the cadence of WooPayments releases and use the latest test build.

I've been experimenting with this flow, and it works well.

1. Every Monday, WooPayments release DRI creates a test release of WooPayments with the changes merged onto the trunk.
2. We add a new scheduled workflow (https://github.com/Automattic/woopay/pull/2998), which runs the WooPay e2es against the latest test WooPayments build. This workflow gets triggered on Wednesday.
3. We update WooPay staging release instructions, asking WooPay release DRI to check whether the scheduled e2e run was successful when they make the release on Thursday.
4. We update the existing e2es to improve coverage around the save-my-info components.

This allows us to detect issues before they go into production. However, one downside is that, when an e2e failure happens, it won't immediately indicate what caused the failure. We will have to look through the merged WooPayments PRs to find the culprit.

@a-danae what do you think?

[malithsen]

• https://github.com/Automattic/woopay/pull/2998 introduces the new scheduled workflow, which runs e2es using the latest available merchant WooPayments versions.
• Minor improvements to WooPay test coverage #9671 introduces some minor improvements to tests.

[rafaelzaleski]

Hey @malithsen, we've had this issue open for a while, and I think there's some overlap with what you're planning to do.

For the Slack notification, I implemented it here: e2e-checkout-monitoring.yml.

@bborman22 has been running the E2E tests manually in the workflow when WooPayments and WooCommerce are updated.

Not sure I understand this step correctly. This would involve cloning the WooPayments repo and building it? If I am reading things correctly, right now, we use the already-built WooPayments available under releases.

Yes, you can either use a release version from the WooPayments repo or point to a branch and let the E2E setup build the plugin from that branch. We don’t have a workflow with this option yet—it’s only available when running the tests locally.

[rafaelzaleski]

Not sure I understand this step correctly. This would involve cloning the WooPayments repo and building it? If I am reading things correctly, right now, we use the already-built WooPayments available under releases.

Just to add to this, we already clone the repositories within the E2E setup during the checkout steps of the workflow. We use these cloned repositories to spin up the Docker containers.