Skip to content

chore: removed flaky test from article suite #1156

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.

Sign up for GitHub

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

Merged
NiallJoeMaher merged 1 commit into from
Oct 19, 2024
Merged

chore: removed flaky test from article suite #1156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 4 additions & 1 deletion components/ArticlePreview/ArticleLoading.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
const ArticleLoading = () => (
<div className="my-4 w-full border-l-4 border-l-pink-600 bg-white p-4 shadow dark:bg-neutral-900">
<div
className="my-4 w-full border-l-4 border-l-pink-600 bg-white p-4 shadow dark:bg-neutral-900"
data-testid="article-loading-indicator"
>
<div className="animate-pulse">
<div className="flex space-x-4">
<div className="h-10 w-10 rounded-full bg-neutral-300 dark:bg-neutral-800"></div>
Expand Down
37 changes: 23 additions & 14 deletions e2e/articles.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,27 +102,36 @@ test.describe("Authenticated Articles Page", () => {
// Waits for articles to be loaded
await page.waitForSelector("article");

const initialArticleCount = await page.$$eval(
"article",
(articles) => articles.length,
);
// This delays the requests by 100ms.
// This is needed as the load more article request was resolving too fast
await page.route("**/*", async (route) => {
await new Promise((f) => setTimeout(f, 100));
await route.continue();
});
Comment on lines +105 to +110
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 19, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Limit the network request delay to specific routes to prevent unintended delays

The route handler is currently delaying all network requests by 100ms, which can slow down unrelated requests and impact test performance. To avoid unintended side effects, consider narrowing the route to target only the 'load more articles' request by matching its specific URL or pattern.

Apply this diff to target the specific request:

-await page.route("**/*", async (route) => {
+await page.route("**/api/articles/load-more", async (route) => {
   await new Promise((f) => setTimeout(f, 100));
   await route.continue();
 });

Replace "**/api/articles/load-more" with the exact URL pattern of the 'load more articles' endpoint.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// This delays the requests by 100ms.
// This is needed as the load more article request was resolving too fast
await page.route("**/*", async (route) => {
await new Promise((f) => setTimeout(f, 100));
await route.continue();
});
// This delays the requests by 100ms.
// This is needed as the load more article request was resolving too fast
await page.route("**/api/articles/load-more", async (route) => {
await new Promise((f) => setTimeout(f, 100));
await route.continue();
});


if (!isMobile) {
await page.getByText("Code Of Conduct").scrollIntoViewIfNeeded();
await page.waitForTimeout(5000);
const finalArticleCount = await page.$$eval(
"article",
(articles) => articles.length,
);
expect(finalArticleCount).toBeGreaterThan(initialArticleCount);
const articleLocator = page.locator("article");
const initialArticleCount = await articleLocator.count();

await page
.getByRole("link", { name: "Code Of Conduct" })
.scrollIntoViewIfNeeded();

// We expect to see the loading indicator become visible why loading and then hidden when more articles are loaded
await expect(page.getByTestId("article-loading-indicator")).toBeVisible();
Copy link
Contributor Author

@JohnAllenTech JohnAllenTech Oct 19, 2024

Choose a reason for hiding this comment

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

I know your not a fan of testIds @NiallJoeMaher the alternative to this is

locator('.my-4').first()

which isnt very unique so my thought was using a testId will just make it more robust. Can switch it out if needed

Copy link
Contributor

@NiallJoeMaher NiallJoeMaher Oct 19, 2024

Choose a reason for hiding this comment

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

Yeah, I tend to stay away from test IDs but I also prefer having working tests too 😂

await expect(page.getByTestId("article-loading-indicator")).toBeHidden();
Comment on lines +121 to +122
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 19, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance reliability of loading indicator assertions

The assertions checking the loading indicator's visibility might be timing-sensitive. To improve reliability, consider using timeouts to wait for the expected state or checking for stability.

Modify the assertions as follows:

-await expect(page.getByTestId("article-loading-indicator")).toBeVisible();
-await expect(page.getByTestId("article-loading-indicator")).toBeHidden();
+await expect(page.getByTestId("article-loading-indicator")).toBeVisible({ timeout: 5000 });
+await expect(page.getByTestId("article-loading-indicator")).toBeHidden({ timeout: 5000 });

This ensures the test waits up to 5 seconds for the loading indicator to appear and disappear, enhancing test stability.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await expect(page.getByTestId("article-loading-indicator")).toBeVisible();
await expect(page.getByTestId("article-loading-indicator")).toBeHidden();
await expect(page.getByTestId("article-loading-indicator")).toBeVisible({ timeout: 5000 });
await expect(page.getByTestId("article-loading-indicator")).toBeHidden({ timeout: 5000 });


expect(await articleLocator.count()).toBeGreaterThan(initialArticleCount);
}

await expect(page.getByText("Home")).toBeVisible();
await expect(page.getByRole("link", { name: "Home" })).toBeVisible();
await expect(
page.getByLabel("Footer").getByRole("link", { name: "Events" }),
).toBeVisible();
await expect(page.getByText("Sponsorship")).toBeVisible();
await expect(page.getByText("Code Of Conduct")).toBeVisible();
await expect(page.getByRole("link", { name: "Sponsorship" })).toBeVisible();
await expect(
page.getByRole("link", { name: "Code Of Conduct" }),
).toBeVisible();
});

test("Should write and publish an article", async ({ page, isMobile }) => {
Expand Down
3 changes: 3 additions & 0 deletions playwright.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ export default defineConfig({
/* Collect trace when retrying the failed test. See https://playwright.dev/docs/trace-viewer */
trace: "on-first-retry",
},
expect: {
timeout: 10000,
},

/* Configure projects for major browsers */
projects: [
Expand Down