-
Notifications
You must be signed in to change notification settings - Fork 69
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
Improve E2E checkout tests #7664
Conversation
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: 0 B Total Size: 1.26 MB ℹ️ View Unchanged
|
tests/e2e/specs/upe-split/shopper/shopper-deferred-intent-creation-upe-enabled.spec.js
Outdated
Show resolved
Hide resolved
tests/e2e/specs/upe-split/shopper/shopper-deferred-intent-creation-upe-enabled.spec.js
Outdated
Show resolved
Hide resolved
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.
First round of the review
Currently, I'm noticing that the new test-class-upe-deferred-intent.php
is identical to the split-upe-gateway test with the caveat that it sets the different feature flag.
Since we now execute everything (including unit tests) with the deferred intent creation flag on by default, the existing test and the one added in this PR perform the same checks. Having said that, I'd vote for keeping only the e2e tests changes in this PR.
Context on what we want to improve further if it goes to test coverage
We would want to merge test-class-wc-payment-gateway-wcpay.php
, test-class-upe-payment-gateway.php
and test-class-upe-split-payment-gateway.php
into a single test, which will be part of the clean up work and hopefully can be achieved easily. @timur27 will cover this
Another goal is to extend unit test coverage in some places that still require coverage. Those are not that easy to identify, but here's an example on what we'll be possibly adding and I'm taking care of it as part of #7770 and will continue working on it in the next cleanup parts.
Next steps for this PR
484bf88
to
742f464
Compare
tests/e2e/specs/upe-split/shopper/shopper-deferred-intent-creation-upe-enabled.spec.js
Outdated
Show resolved
Hide resolved
be9ff20
to
9f5b2c2
Compare
9f5b2c2
to
22ba78e
Compare
71b702b
to
ae8a50c
Compare
tests/e2e/specs/upe-split/shopper/shopper-deferred-intent-creation-upe-enabled.spec.js
Show resolved
Hide resolved
tests/e2e/specs/upe-split/shopper/shopper-deferred-intent-creation-upe-enabled.spec.js
Show resolved
Hide resolved
tests/e2e/specs/upe-split/shopper/shopper-deferred-intent-creation-upe-enabled.spec.js
Show resolved
Hide resolved
tests/e2e/specs/upe-split/shopper/shopper-deferred-intent-creation-upe-enabled.spec.js
Show resolved
Hide resolved
The code is ready for the review. The test was flaky both on this branch and on What's left to review my changes
|
✅ review the above comments that I left on my commits ✅ possibly run tests/e2e/specs/upe-split/shopper/shopper-deferred-intent-creation-upe-enabled.spec.js locally
I was getting failures locally with this change. It turns out that ✅ approve if all above is green |
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.
LGTM
Fixes #6882
Changes proposed in this Pull Request
This is adding more test coverage for the deferred intent UPE. The split UPE tests were ported over to test with the deferred intent flag enabled instead. The e2e coverage is already in pretty good shape and covers the main checkout flows. I've added a couple more covering the following scenarios:
The default payment method test has added some flakiness that I haven't been able to pin down even though the test looks fine in the browser.
#5022 (comment) is a great reference for potential tests in the future.
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