chore: removed flaky test from article suite #1156
Conversation
|
@JohnAllenTech is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in this pull request involve modifications to three files: the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
JohnAllenTech
temporarily deployed
to
production
GitHub Actions
Inactive
| .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(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yeah, I tend to stay away from test IDs but I also prefer having working tests too 😂
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
playwright.config.ts (1)
32-34: LGTM! Consider monitoring the impact on test execution times.The addition of the
expectproperty with a 10-second timeout is a good step towards improving test reliability. This change aligns well with the PR's objective of addressing flaky tests.However, it's important to monitor how this global timeout affects overall test execution times. If you notice significant increases in test duration, you might want to consider:
- Adjusting the timeout value.
- Using more specific timeouts for individual tests or test suites that require longer wait times.
To ensure this change doesn't mask performance issues, consider implementing performance monitoring for your tests. This could involve tracking and reporting on test execution times, which would help identify any unexpected slowdowns introduced by this change.
e2e/articles.spec.ts (2)
120-120: Correct the typo in the commentThere's a typo in the comment: "We expect to see the loading indicator become visible why loading...". It should be "while".
Apply this diff to fix the typo:
-// We expect to see the loading indicator become visible why loading and then hidden when more articles are loaded +// We expect to see the loading indicator become visible while loading and then hidden when more articles are loaded
124-124: Include 'await' before 'expect' for consistencyWhile
expectis synchronous, addingawaitbefore it maintains consistency with other asynchronous operations and improves readability.Apply this diff:
-expect(await articleLocator.count()).toBeGreaterThan(initialArticleCount); +await expect(await articleLocator.count()).toBeGreaterThan(initialArticleCount);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- components/ArticlePreview/ArticleLoading.tsx (1 hunks)
- e2e/articles.spec.ts (1 hunks)
- playwright.config.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
components/ArticlePreview/ArticleLoading.tsx (1)
2-5: Excellent addition for improved testability!The addition of the
data-testid="article-loading-indicator"attribute to the outer<div>is a great improvement. This change enhances the component's testability by providing a reliable and specific selector for automated tests. It aligns well with the PR objective of improving test reliability and removing flaky tests.This modification:
- Does not affect the component's visual appearance or functionality.
- Facilitates more robust and stable end-to-end tests.
- Follows best practices for making components easily testable.
e2e/articles.spec.ts (1)
127-134: LGTM!The assertions correctly verify the visibility of the footer links, ensuring that key navigation elements are present.
| // 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(); | ||
| }); |
There was a problem hiding this comment.
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.
| // 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(); | |
| }); |
| await expect(page.getByTestId("article-loading-indicator")).toBeVisible(); | ||
| await expect(page.getByTestId("article-loading-indicator")).toBeHidden(); |
There was a problem hiding this comment.
🛠️ 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.
| 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 }); |
✨ Codu Pull Request 💻
Fixes 1/2 on #1154
Pull Request details
Any Breaking changes
Associated Screenshots