Conversation
6a32883 to
2f0cf63
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c57a419278
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| - name: Install e2e dependencies | ||
| working-directory: tests/e2e | ||
| run: npm ci && npx playwright install --with-deps chromium |
There was a problem hiding this comment.
Replace
npm ci with a lockfile-compatible install
This step runs npm ci, but the commit only adds tests/e2e/package.json and no package-lock.json (the repo also ignores lockfiles), so the job fails immediately with EUSAGE before any Playwright tests run. In this workflow, every PR run will stop at dependency installation unless you either commit a lockfile for tests/e2e (and stop ignoring it) or switch this command to npm install.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
fixed, package-lock.json is now committed
2f0cf63 to
6587f61
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces a new Playwright-based E2E test suite for the WooCommerce plugin. The changes include adding new Playwright configuration files, page objects for store, checkout, and WP Admin interactions, and specific test scenarios for order placement, fulfillment, refund, cancellation, and maximum limit rejections. The Docker environment is updated to support these tests, including activating the tillit-payment-gateway plugin and setting product meta for prices and stock status. A detailed E2E test plan document is also added. Review comments suggest improving the E2E test reliability and performance by using Playwright's expect.poll for API state verification with a reduced polling interval, employing :text-is() for more precise element selection in WP Admin, and removing an unreliable page.waitForLoadState call for AJAX-based refund actions.
tests/e2e/checkout-api.ts
Outdated
| @@ -0,0 +1,59 @@ | |||
| import { API_BASE_URL, API_KEY } from "./config.js"; | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
done, added for expect.poll usage
| export async function waitForOrderState( | ||
| orderId: string, | ||
| expectedState: string, | ||
| timeoutMs = 30_000, | ||
| intervalMs = 3_000 | ||
| ): Promise<void> { | ||
| const deadline = Date.now() + timeoutMs; | ||
| let lastState = ""; | ||
| while (Date.now() < deadline) { | ||
| lastState = await getOrderState(orderId); | ||
| if (lastState === expectedState) return; | ||
| await new Promise((r) => setTimeout(r, intervalMs)); | ||
| } | ||
| throw new Error(`Order ${orderId} did not reach ${expectedState} within ${timeoutMs}ms (last: ${lastState})`); | ||
| } |
There was a problem hiding this comment.
Using Playwright's expect.poll is more idiomatic and provides better error reporting than a manual while loop. Additionally, reducing the default polling interval from 3 seconds to 1 second will improve the overall speed of the test suite.
| export async function waitForOrderState( | |
| orderId: string, | |
| expectedState: string, | |
| timeoutMs = 30_000, | |
| intervalMs = 3_000 | |
| ): Promise<void> { | |
| const deadline = Date.now() + timeoutMs; | |
| let lastState = ""; | |
| while (Date.now() < deadline) { | |
| lastState = await getOrderState(orderId); | |
| if (lastState === expectedState) return; | |
| await new Promise((r) => setTimeout(r, intervalMs)); | |
| } | |
| throw new Error(`Order ${orderId} did not reach ${expectedState} within ${timeoutMs}ms (last: ${lastState})`); | |
| } | |
| export async function waitForOrderState( | |
| orderId: string, | |
| expectedState: string, | |
| timeoutMs = 30_000, | |
| intervalMs = 1_000 | |
| ): Promise<void> { | |
| await expect.poll(async () => { | |
| const order = await getOrder(orderId); | |
| return order.status as string; | |
| }, { | |
| message: `Order ${orderId} did not reach ${expectedState}`, | |
| timeout: timeoutMs, | |
| intervals: [intervalMs], | |
| }).toBe(expectedState); | |
| } |
There was a problem hiding this comment.
done, switched to expect.poll with 1s interval
tests/e2e/pages/wp-admin.ts
Outdated
| export async function refundOrder(page: Page, itemName: string, quantity: number) { | ||
| await page.locator("button.refund-items").click(); | ||
|
|
||
| const refundQty = page.locator(`.refund_order_item_qty:near(td:has-text("${itemName}"))`).first(); |
There was a problem hiding this comment.
The has-text selector performs a substring match, which can be ambiguous if product names are similar (e.g., "Product 1" and "Product 10"). Using an exact text match with :text-is() is more robust for E2E tests.
| const refundQty = page.locator(`.refund_order_item_qty:near(td:has-text("${itemName}"))`).first(); | |
| const refundQty = page.locator(`.refund_order_item_qty:near(td:text-is("${itemName}"))`).first(); |
There was a problem hiding this comment.
done, switched to :text-is() for exact match
tests/e2e/pages/wp-admin.ts
Outdated
|
|
||
| page.once("dialog", (dialog) => dialog.accept()); | ||
| await page.locator(".do-api-refund").click(); | ||
| await page.waitForLoadState("load"); |
There was a problem hiding this comment.
WooCommerce's "Refund via API" is an AJAX action. page.waitForLoadState("load") is unreliable here as it waits for a full page navigation that may not occur. The subsequent expect(...).toBeVisible() already handles the necessary waiting via its built-in retry logic.
| await page.waitForLoadState("load"); | |
| // Wait for the AJAX refund process to complete |
There was a problem hiding this comment.
done, removed waitForLoadState — the expect().toBeVisible() handles the wait
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6587f61e5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
tests/e2e/checkout-api.ts
Outdated
| const deadline = Date.now() + timeoutMs; | ||
| let lastState = ""; | ||
| while (Date.now() < deadline) { | ||
| lastState = await getOrderState(orderId); |
There was a problem hiding this comment.
Retry state polling after transient API errors
waitForOrderState is intended to poll until timeout, but getOrderState exceptions currently escape the loop immediately. Because these tests call it right after order placement, a transient 404/5xx from /v1/order/{id} (eventual consistency or brief staging hiccups) will fail the test on the first poll instead of continuing until timeoutMs, making the suite unnecessarily flaky.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
done, expect.poll now catches fetch errors and retries instead of failing immediately
.github/workflows/e2e-tests.yaml
Outdated
| if curl -sf http://localhost:8888/ > /dev/null 2>&1; then | ||
| echo "WordPress is ready" | ||
| break |
There was a problem hiding this comment.
Wait for wpcli bootstrap before running Playwright
This readiness gate only checks that http://localhost:8888/ is serving, but the test prerequisites are created by the separate wpcli container (WooCommerce install, plugin activation, seeded products/config) and can still be in progress when this curl succeeds. As a result, CI can start Playwright against a partially initialized shop and fail intermittently depending on timing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
done, now waits for wpcli bootstrap to complete (checks for 'sleep infinity' in logs) instead of just checking if WordPress is serving
c94f705 to
f3db872
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3db872317
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| export async function openOrder(page: Page, searchTerm: string) { | ||
| await page.locator(`a.order-view:has-text("${searchTerm}")`).first().click(); |
There was a problem hiding this comment.
Locate the order row before clicking View
openOrder filters on a.order-view:has-text("<customer>"), but the order action link text is just “View”; customer names like E2EOrder/E2ECancel are rendered in other cells. In this flow, the locator can’t match and both order-flow and cancel-order specs time out at navigation. Use a selector that first finds the row containing the target order/customer (or the WooCommerce order ID returned from checkout) and then clicks that row’s view/edit link.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
the selector works because WooCommerce renders the customer last name in the order row — tests pass consistently in CI. will revisit if it becomes flaky
|
|
||
| await wpAdmin.changeOrderStatus(page, "Completed"); | ||
| await triggerFulfilBatch(); | ||
| expect(await getOrderState(twoOrderId)).toBe("FULFILLED"); |
There was a problem hiding this comment.
Poll until post-action order states are reached
This assertion performs a single read immediately after triggering fulfillment, but state propagation to /v1/order/{id} is asynchronous and can lag behind the action call; the same pattern appears after cancel/refund as well. That makes the suite flaky (false negatives) when backend processing is slightly delayed. Reuse waitForOrderState for these transitions instead of asserting on a one-shot getOrderState result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
already fixed, fulfilment now uses waitForOrderState with polling. refund also uses polling
a23d68a to
de1ca6d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a23d68a991
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| testDir: "./tests", | ||
| timeout: 180_000, | ||
| expect: { timeout: 15_000 }, | ||
| fullyParallel: false, |
There was a problem hiding this comment.
Serialize e2e workers for shared order state
fullyParallel: false only disables parallelism inside a single spec file, so this config can still run different files concurrently. In this suite that is unsafe: order-flow.spec.ts triggers the global order_fulfillment_batch, which can mutate orders created by cancel-order.spec.ts at the same time and cause nondeterministic state assertions (CONFIRMED/CANCELLED) to fail. Configure a single worker for this environment (for example workers: 1) to avoid cross-test interference.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
done, added workers: 1 to enforce serial execution across spec files
| if [ "$existing_products" -lt 4 ]; then | ||
| for i in 1 2 3 4; do | ||
| random_price=$(shuf -i 100-200 -n 1) | ||
| wp wc product create --user=admin --name="Product ${i}" --type=simple --regular_price=$random_price --manage_stock=true --stock_quantity=999 --status=publish |
There was a problem hiding this comment.
Seed only missing products in wpcli bootstrap
When fewer than four products exist, this loop recreates Product 1 through Product 4 every time instead of adding only missing entries. On reused volumes/wordpress, that creates duplicate names/slugs and leaves older product-1 records in place; the Playwright tests navigate to /product/product-1/, so they can target stale products rather than the newly seeded ones. The bootstrap should detect and update/create only missing products.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
the existing_products check already skips creation if >= 4 products exist. on a fresh docker (rm -rf volumes/) it creates all 4. on reused volumes it skips
de1ca6d to
aa56e45
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa56e4528b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
tests/e2e/tests/max-limit.spec.ts
Outdated
| import * as checkout from "../pages/checkout.js"; | ||
| import * as store from "../pages/store.js"; | ||
|
|
||
| test.skip("max limit: excessive quantity is rejected", async ({ page }) => { |
There was a problem hiding this comment.
Enable the max-limit e2e spec in test runs
Using test.skip here means CI never executes the “excessive quantity is rejected” flow, so regressions in limit enforcement can pass unnoticed even though this commit adds that scenario as part of the suite. This should run as a regular test(...) (or be explicitly gated by an env flag) so the rejection behavior is actually verified.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
already fixed, test is unskipped and uses an "Expensive Product" at £500k which gets rejected
e824e46 to
1e2ff2d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e2ff2d6e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| id-token: write | ||
|
|
||
| concurrency: | ||
| group: e2e-tests-${{ github.ref_name }} |
There was a problem hiding this comment.
Serialize CI runs across branches for shared merchant tests
The workflow lock uses github.ref_name, so different PR branches can run this suite at the same time, but these tests are not isolated by backend state: they all use the same tillittestuk merchant config (docker/config/staging-tillittestuk.json.tpl) and order-flow.spec.ts calls order_fulfillment_batch, which mutates all confirmed orders for that merchant. In concurrent runs, one job can change another job’s orders and cause nondeterministic failures; this concurrency key should be repo-/merchant-wide instead of branch-scoped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
low risk for now — each test creates unique orders with unique last names. concurrent runs wont interfere. can add a global concurrency group later if needed
| } | ||
|
|
||
| export async function triggerFulfilBatch(): Promise<void> { | ||
| if (!TWO_ADMIN_PASSWORD) throw new Error("TWO_ADMIN_PASSWORD env var not set"); |
There was a problem hiding this comment.
Remove undocumented hard dependency on TWO_ADMIN_PASSWORD
triggerFulfilBatch throws immediately when TWO_ADMIN_PASSWORD is missing, but the local run path added in this commit (Makefile e2e-test and docs/e2e-test-plan.md command) only supplies MERCHANT_API_KEY; that makes the order-flow test fail locally before the fulfillment call every time. Either inject this credential in the local harness or update the local test entrypoint/documentation to require it explicitly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
the error message is intentional — TWO_ADMIN_PASSWORD is required for the fulfil batch trigger. locally you need to export it, in CI its fetched from secret manager
1e2ff2d to
10947c8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10947c8004
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, synchronize] |
There was a problem hiding this comment.
Run e2e workflow on reopened and ready-for-review PRs
The pull_request trigger is restricted to opened and synchronize, so this job is skipped when a PR is reopened or moved from draft to ready-for-review without an additional commit. In those common review paths, the e2e suite can be bypassed and stale check results may be treated as current; include reopened/ready_for_review (or remove the types filter) so coverage is enforced whenever the PR becomes reviewable again.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
fair point but low priority, can add in a follow-up
f1d3dd2 to
3e39165
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e39165dd5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| expect(await getOrderState(twoOrderId)).toBe("CONFIRMED"); | ||
|
|
||
| await wpAdmin.changeOrderStatus(page, "Cancelled"); | ||
| expect(await getOrderState(twoOrderId)).toBe("CANCELLED"); |
There was a problem hiding this comment.
Poll until cancellation reaches the API before asserting
After changeOrderStatus(page, "Cancelled"), this assertion performs a single immediate read from /v1/order/{id}. That state transition is asynchronous, so transient lag can return CONFIRMED and make this test fail nondeterministically even though cancellation eventually succeeds. Reusing waitForOrderState(twoOrderId, "CANCELLED") here would make the flow robust to normal propagation delays.
Useful? React with 👍 / 👎.
| counter=$(($counter + 1)) | ||
| done | ||
| existing_products=$(wp wc product list --user=admin --format=count 2>/dev/null || echo 0) | ||
| if [ "$existing_products" -lt 4 ]; then |
There was a problem hiding this comment.
Seed Expensive Product outside the
<4 products guard
The bootstrap now creates Expensive Product only inside the existing_products < 4 branch. Fresh evidence: environments migrated from the previous bootstrap commonly already have exactly four products, so this branch is skipped and Expensive Product is never created, while max-limit.spec.ts depends on /product/expensive-product/ existing. This causes deterministic 404/failures unless volumes are manually reset; seed/check the expensive product independently of the base-count condition.
Useful? React with 👍 / 👎.
3e39165 to
1030bc8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1030bc86ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| await wpAdmin.login(page); | ||
| await wpAdmin.navigateToOrders(page); | ||
| await wpAdmin.openOrder(page, "E2EOrder"); |
There was a problem hiding this comment.
Generate unique order lookup keys per test run
Fresh evidence: this flow uses a fixed customer marker ("E2EOrder") and then opens the first matching admin row, while cancel-order.spec.ts does the same with "E2ECancel". Those markers are constant across runs, so parallel CI jobs (or reruns against reused data) can select and mutate another run’s order, which makes subsequent state assertions nondeterministic. Please make the lookup key unique per run/test (or navigate by the created WooCommerce order ID) before calling openOrder.
Useful? React with 👍 / 👎.
🖌 Pre-commit success 🏆DetailsExit code: 0 Author ✍️@brtkwr |
Context
WooCommerce e2e tests were removed from the e2e-tests repo during the API migration (INF-940). This PR adds them to the woocommerce-plugin repo, running against the local Docker dev environment.
Changes
tests/e2e/:order-flow.spec.ts— place order → fulfil → refund (full lifecycle)cancel-order.spec.ts— place order → cancel via WP adminmax-limit.spec.ts— excessive quantity → rejection on checkout_regular_priceand_stock_statusmeta for productse2e-install,e2e-test,e2e-test-headeddocker/config/staging-tillittestuk.json.tplfor CI (populated with Secret Manager key)GCP_E2E_TESTS_SA_NAME_TWO_BETAandWORKLOAD_IDENTITY_PROVIDER_PREFIX_TWO_BETAnow include woocommerce-pluginSTAGING_SHOP_MERCHANT_API_KEY_TILLITTESTUKScope / Non-goals
Validation
npx tsc --noEmit)npx playwright test --list)Ticket