Skip to content
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

[37, 119, 191, 207, 105] Fix flaky test and remove retries #5142

Merged
merged 20 commits into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/quiet-camels-raise.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"saleor-dashboard": patch
---

Some end-to-end Playwright tests now have extended timeouts for UI elements to load. This means that automation tests should fail less. Playwright retires value has been set to 0.
45 changes: 24 additions & 21 deletions playwright.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,39 +4,42 @@ import dotenv from "dotenv";
dotenv.config();

const env = process.env;
const DEFAULT_RETRIES = "1";
const DEFAULT_WORKERS = "2";
// const DEFAULT_RETRIES = "1";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need that comment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to see how disabling retries behaves and if needed revert this change.


export default defineConfig({
testDir: "playwright/tests",
fullyParallel: true,
forbidOnly: !!env.CI,
retries: parseInt(env.RETRIES || DEFAULT_RETRIES),
retries: 0,
// We are disabling retries for now as it prolongs the test run time
// as the test will most likely fail again. We can enable it later if needed.
// retries: parseInt(env.RETRIES || DEFAULT_RETRIES),
workers: parseInt(env.WORKERS || DEFAULT_WORKERS),
reporter: process.env.CI
? [
["blob"],
["github"],
[
"playwright-testmo-reporter",
{
outputFile: "testmo/testmo.xml", // Optional: Output file path. Defaults to 'testmo.xml'.
embedBrowserType: true, // Optional: Embed browser type in the XML file. Defaults to false.
embedTestSteps: true, // Optional: Embed test steps in the XML file. Defaults to true.
testStepCategories: ["hook", "expect", "pw:api", "test.step"], // Optional: Test step categories to include in the XML file. Defaults to ["hook","expect","pw:api","test.step"]. Possible options are "hook", "expect", "pw:api", "test.step".
testTitleDepth: 1, // Optional: Test case title depth to report in the XML file. Defaults to 1. Increase this to 2 include suite name. Increase this even further to include the path.
attachmentBasePathCallback: () =>
process.env.URL_TO_RUN ? process.env.URL_TO_RUN : "",
},
],
]
["blob"],
["github"],
[
"playwright-testmo-reporter",
{
outputFile: "testmo/testmo.xml", // Optional: Output file path. Defaults to 'testmo.xml'.
embedBrowserType: true, // Optional: Embed browser type in the XML file. Defaults to false.
embedTestSteps: true, // Optional: Embed test steps in the XML file. Defaults to true.
testStepCategories: ["hook", "expect", "pw:api", "test.step"], // Optional: Test step categories to include in the XML file. Defaults to ["hook","expect","pw:api","test.step"]. Possible options are "hook", "expect", "pw:api", "test.step".
testTitleDepth: 1, // Optional: Test case title depth to report in the XML file. Defaults to 1. Increase this to 2 include suite name. Increase this even further to include the path.
attachmentBasePathCallback: () =>
process.env.URL_TO_RUN ? process.env.URL_TO_RUN : "",
},
],
]
: [["html"], ["list"]],
expect: { timeout: 5000 },
expect: { timeout: 10 * 1000 },
Cloud11PL marked this conversation as resolved.
Show resolved Hide resolved
maxFailures: 10,
timeout: 30000,
timeout: 30 * 1000,
use: {
baseURL: env.BASE_URL || "",
trace: env.CI ? "on-all-retries" : "on",
trace: env.CI ? "retain-on-failure" : "on",
screenshot: "only-on-failure",
testIdAttribute: "data-test-id",
video: env.CI ? "retain-on-failure" : "off",
Expand All @@ -51,7 +54,7 @@ export default defineConfig({
name: "e2e",
dependencies: ["setup"],
use: { ...devices["Desktop Chrome"] },
testIgnore: "playwright/tests/apps.spec.ts"
testIgnore: "playwright/tests/apps.spec.ts",
},
{
name: "apps-e2e",
Expand Down
1 change: 1 addition & 0 deletions playwright/pages/customersPage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export class CustomersPage extends BasePage {
readonly issueNewGiftCardButton = page.getByTestId("issue-new-gift-card"),
readonly emailPageTitleText = page.getByTestId("user-email-title"),
readonly customerActiveCheckbox = page.getByTestId("customer-active-checkbox").locator("input"),
readonly amountDropdown = page.locator('div[name="balanceCurrency"]'),
) {
super(page);
this.addressForm = new AddressForm(page);
Expand Down
7 changes: 5 additions & 2 deletions playwright/pages/dialogs/issueGiftCardDialog.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ export class IssueGiftCardDialog extends BasePage {
readonly okButton = page.getByTestId("submit"),
readonly copyCodeButton = page.getByTestId("copy-code-button"),
readonly option = page.getByTestId("select-option"),
readonly issueGiftCardDialog = page.getByTestId("gift-card-dialog"),
readonly amountDropdown = page.locator('div[name="balanceCurrency"]'),
) {
super(page);
}
Expand Down Expand Up @@ -57,6 +59,7 @@ export class IssueGiftCardDialog extends BasePage {

async typeCustomTag(tag: string) {
await this.tagsInput.fill(tag);
await expect(this.issueGiftCardDialog.getByText("Loading...")).not.toBeVisible();
await this.tagsInputOptions.filter({ hasText: `Add new value: ${tag}` }).click();
}

Expand Down Expand Up @@ -95,7 +98,7 @@ export class IssueGiftCardDialog extends BasePage {
return allTexts[0];
}

async blur() {
await this.page.click("[data-test-id='gift-card-dialog']");
async tagsInputBlur() {
await this.tagsInput.blur();
}
}
4 changes: 3 additions & 1 deletion playwright/pages/pageElements/rightSideDetailsSection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,9 @@ export class RightSideDetailsPage extends BasePage {

async expectOptionsSelected(section: Locator, names: string[]) {
for (const name of names) {
await expect(section.getByText(name)).toBeVisible({ timeout: 30000 });
await expect(section.getByText(name)).toBeVisible({
timeout: 30 * 1000, // 30 seconds
});
}
}

Expand Down
32 changes: 13 additions & 19 deletions playwright/tests/apps.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,45 +18,39 @@ test.beforeEach(({ page }) => {

const PRE_INSTALLATION_TIMEOUT = 20 * 1000;
const INSTALLATION_PENDING_TIMEOUT = 50 * 1000;
const APP_LISTED_TIMEOUT = 15 * 1000;
const APP_INSTALLED_TIMEOUT = 50 * 1000;
const APP_EXPECT_UI_TIMEOUT = 15 * 1000;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need such long timeouts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


test("TC: SALEOR_119 User should be able to install and configure app from manifest @e2e", async ({
page,
}) => {
await appsPage.gotoAppsList();
await appsPage.waitForDOMToFullyLoad();
await expect(appsPage.installExternalAppButton).toBeVisible();
await appsPage.installExternalAppButton.click();
await appsPage.typeManifestUrl("https://klaviyo.saleor.app/api/manifest");
await appsPage.installAppFromManifestButton.click();
// Klaviyo app can take a while to respond with manifest if it's
// cold-starting
await expect(installationPage.appInstallationPageHeader).toHaveText(
"You are about to install Klaviyo",
{
// Klaviyo app can take a while to respond with manifest if it's
// cold-starting
timeout: PRE_INSTALLATION_TIMEOUT,
},
);
await installationPage.installAppButton.click();
await appsPage.expectSuccessBanner();

await expect(appsPage.successBanner).toBeVisible({ timeout: INSTALLATION_PENDING_TIMEOUT });
await expect(appsPage.installedAppRow.first()).toBeVisible();
await appsPage.installationPendingLabel.waitFor({
state: "hidden",
timeout: INSTALLATION_PENDING_TIMEOUT,
});
await expect(appsPage.appKlaviyo).toContainText("Klaviyo", {
timeout: APP_LISTED_TIMEOUT,
});
await appsPage.installedAppRow
.filter({ hasText: "Klaviyo" })
.first()
.waitFor({ state: "visible", timeout: APP_INSTALLED_TIMEOUT });
await expect(appsPage.installationPendingLabel).not.toBeVisible();

await expect(appsPage.appKlaviyo).toContainText("Klaviyo");
await expect(appsPage.installedAppRow.filter({ hasText: "Klaviyo" }).first()).toBeVisible();
await appsPage.appKlaviyo.click();

const iframeLocator = page.frameLocator("iframe");

await expect(iframeLocator.getByLabel("PUBLIC_TOKEN")).toBeVisible();
await expect(iframeLocator.getByLabel("PUBLIC_TOKEN")).toBeVisible({
// Klavyio's UI can take a while to load initially
timeout: APP_EXPECT_UI_TIMEOUT,
});
await iframeLocator.getByLabel("PUBLIC_TOKEN").fill("test_token");
await iframeLocator.getByText("Save").click();
await appsPage.expectSuccessBanner();
Expand Down
2 changes: 2 additions & 0 deletions playwright/tests/customers.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,8 @@ test("TC: SALEOR_207 Issue a new gift card for the customer @e2e @customer", asy

await customersPage.gotoCustomerDetailsPage(CUSTOMERS.editCustomer.id);
await customersPage.clickIssueNewGiftCard();

await expect(customersPage.amountDropdown).toBeVisible();
await customersPage.issueGiftCardDialog.typeAmount(amount);
await customersPage.issueGiftCardDialog.typeCustomTag(faker.lorem.word());
await customersPage.issueGiftCardDialog.typeNote(faker.lorem.sentences(3));
Expand Down
3 changes: 2 additions & 1 deletion playwright/tests/giftCards.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,10 @@ test.beforeEach(async ({ page, request }) => {
});
test("TC: SALEOR_105 Issue gift card @e2e @gift", async () => {
await giftCardsPage.clickIssueCardButton();
await expect(giftCardsPage.issueGiftCardDialog.amountDropdown).toBeVisible();
await giftCardsPage.issueGiftCardDialog.typeAmount("50");
await giftCardsPage.issueGiftCardDialog.typeCustomTag("super ultra automation discount");
await giftCardsPage.issueGiftCardDialog.blur();
await giftCardsPage.issueGiftCardDialog.tagsInputBlur();
await giftCardsPage.issueGiftCardDialog.clickRequiresActivationCheckbox();
await giftCardsPage.issueGiftCardDialog.clickIssueButton();
await expect(giftCardsPage.issueGiftCardDialog.cardCode).toBeVisible();
Expand Down
6 changes: 5 additions & 1 deletion playwright/tests/orders.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,10 @@ test("TC: SALEOR_84 Create draft order @e2e @draft", async () => {
});

test("TC: SALEOR_191 Refund products from the fully paid order @e2e @refunds", async () => {
// All steps of this test pass (including after hooks), but Playwright
// marks it as failed because of exceeding 30s timeout
test.slow();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this marks test as slow, and triples the timeout, meaning something similar as if you were added 3x base timeout here. I does not look like fix for the flaky test.

Since you are fixing couple of test in single PR, please mark this as TODO, and leave ticket number for further fixing.

Copy link
Member Author

@Cloud11PL Cloud11PL Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last time it failed the test threw the following error:

Test finished within timeout of 30000ms, but tearing down "context" ran out of time.
Please allow more time for the test, since teardown is attributed towards the test timeout budget.

here's the recording:
32_timeout_PW.webm - every step passed


const order = ORDERS.fullyPaidOrderWithSingleTransaction;

await ordersPage.goToExistingOrderPage(order.id);
Expand All @@ -268,7 +272,7 @@ test("TC: SALEOR_191 Refund products from the fully paid order @e2e @refunds", a

const productRow = await refundPage.getProductRow(order.lineItems[0].name);

expect(productRow.locator(refundPage.productQuantityInput)).toHaveValue(
await expect(productRow.locator(refundPage.productQuantityInput)).toHaveValue(
order.lineItems[0].quantity,
);

Expand Down
9 changes: 9 additions & 0 deletions playwright/tests/shippingMethods.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,17 @@ test("TC: SALEOR_37 Update a shipping method @shipping-method @e2e", async () =>
SHIPPING_METHODS.shippingMethodToBeUpdated.name,
);

await shippingMethodsPage.rightSideDetailsPage.expectOptionsSelected(
channelSection,
alreadyAssignedChannels,
);
await shippingMethodsPage.rightSideDetailsPage.clickChannelsSelectShippingPage();
await shippingMethodsPage.rightSideDetailsPage.selectSingleChannelShippingPage();
await shippingMethodsPage.rightSideDetailsPage.expectOptionsSelected(
warehouseSection,
alreadyAssignedWarehouses,
);

await shippingMethodsPage.rightSideDetailsPage.clickWarehouseSelectShippingPage();
await shippingMethodsPage.rightSideDetailsPage.typeAndSelectMultipleWarehousesShippingPage(
warehousesToBeAssigned,
Expand Down
Loading