Feat/improving existing e2e tests #1125
Conversation
|
@JohnAllenTech is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request introduces several modifications to end-to-end tests across multiple files. A new accessibility test suite is added to check for alt text on images in the homepage. The articles test suite is restructured into authenticated and unauthenticated blocks, with new tests added. The authentication setup is improved to dynamically retrieve session IDs. The homepage tests are reorganized, and a new unauthenticated homepage suite is introduced. Lastly, the login tests are split into authenticated and unauthenticated sections, enhancing clarity and functionality. Changes
Possibly related PRs
Suggested reviewers
Poem
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (6)
e2e/accessibility.spec.ts (1)
1-13: Great addition of accessibility tests!This new test suite for accessibility checks is a valuable addition to the e2e test suite. It aligns well with the PR objectives of improving existing tests and adds an important layer of quality assurance for accessibility compliance.
A few suggestions for further enhancement:
- Consider adding more specific test cases for different types of images or different pages.
- You might want to include checks for other accessibility features in the future, such as proper heading structure, keyboard navigation, or ARIA attributes.
Overall, this is a solid start for accessibility testing. Great job!
e2e/home.spec.ts (1)
Duplicate test assertions found in
e2e/home.spec.ts
- The assertion
expect(page.locator("h1")).not.toContainText("Unwanted text")is present twice.🔗 Analysis chain
Line range hint
30-41: Comprehensive test case for unauthenticated homepage.The new test case for the unauthenticated homepage view covers important aspects:
- Checks for the absence of unwanted text in the header.
- Verifies the presence of specific content in
h2andh1elements.This aligns well with the PR objective of adding tests to improve user experience verification.
To ensure consistency with other tests, run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for similar patterns in other test files # Test: Search for similar assertions in other test files rg --type typescript 'expect\(.*\.locator\("h1"\)\)\.not\.toContainText\("Unwanted text"\)' e2e rg --type typescript 'expect\(.*\.locator\("h2"\)\)\.toContainText' e2e rg --type typescript 'expect\(.*\.locator\("h1"\)\)\.toContainText' e2eLength of output: 322
Script:
#!/bin/bash # Description: Check for similar patterns in other TypeScript test files # Test: Search for similar assertions in other test files rg 'expect\(.*\.locator\("h1"\)\)\.not\.toContainText\("Unwanted text"\)' -g '*.ts' e2e rg 'expect\(.*\.locator\("h2"\)\)\.toContainText' -g '*.ts' e2e rg 'expect\(.*\.locator\("h1"\)\)\.toContainText' -g '*.ts' e2eLength of output: 539
e2e/auth.setup.ts (2)
36-36: Good improvement: Using environment variable for session IDReplacing the hardcoded session ID with
process.env.E2E_USER_SESSION_IDenhances flexibility and security. This change allows for dynamic session IDs and keeps sensitive data out of the codebase.For added type safety, consider using a non-null assertion or providing a default value:
value: process.env.E2E_USER_SESSION_ID!, // or value: process.env.E2E_USER_SESSION_ID || '',This ensures TypeScript knows the value is defined, aligning with the earlier expectation.
Line range hint
1-54: Overall: Excellent improvements to E2E test authenticationThe changes in this file significantly enhance the authentication setup for E2E tests:
- Ensuring the
E2E_USER_SESSION_IDenvironment variable is defined improves robustness.- Using the environment variable for the session ID increases flexibility and security.
These modifications align well with the PR objectives of hardening the current test suite. They improve the reliability of the authentication process without introducing breaking changes.
To further improve the file:
- Consider adding a comment explaining the purpose of the
E2E_USER_SESSION_IDvariable for better documentation.- You might want to add error handling to provide more informative messages if the environment variable is missing or invalid.
e2e/login.spec.ts (2)
52-54: Clarify the test name to reflect hidden elements for authenticated usersThe test
"Login page contains GitHub button"checks that the GitHub login button is hidden for authenticated users. Renaming the test will improve clarity and maintain consistency.Consider renaming the test:
-test("Login page contains GitHub button", async ({ page }) => { +test("GitHub login button is hidden for authenticated users", async ({ page }) => {
56-58: Clarify the test name to reflect hidden elements for authenticated usersSimilarly, the test for the GitLab button checks that it is hidden for authenticated users. Updating the test name will enhance readability.
Consider renaming the test:
-test("Login page contains GitLab button", async ({ page }) => { +test("GitLab login button is hidden for authenticated users", async ({ page }) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- e2e/accessibility.spec.ts (1 hunks)
- e2e/articles.spec.ts (1 hunks)
- e2e/auth.setup.ts (1 hunks)
- e2e/home.spec.ts (1 hunks)
- e2e/login.spec.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (7)
e2e/accessibility.spec.ts (2)
1-1: LGTM: Imports are correct and concise.The necessary functions
testandexpectare imported from the Playwright test library, which is appropriate for this e2e test file.
3-4: Well-structured test suite hierarchy.The test suite is organized with a clear hierarchy, using nested
describeblocks. This structure enhances readability and provides a good context for the specific accessibility test being performed.e2e/home.spec.ts (4)
3-3: Improved test suite naming for clarity.The renaming of the test suite from "Testing homepage views" to "Authenticated homepage" enhances clarity by explicitly indicating that these tests are for authenticated users. This change aligns well with the PR objective of improving test descriptiveness.
26-29: Well-structured new test suite for unauthenticated homepage.The addition of a separate test suite for the unauthenticated homepage improves test organization and clarity. The
beforeEachhook that clears cookies ensures a clean state for each test, which is a good practice for maintaining test isolation.
Line range hint
1-41: Verify the impact of removed tests.Two significant removals were observed:
- The "Authenticated landing page view" test
- The "Confirm image accessibility content" test suite
While the first removal might be justified if it's redundant with the new "Homepage view" test, the removal of the image accessibility test suite is concerning. This could potentially impact the overall accessibility testing coverage of the application.
Please run the following script to check if these tests have been moved or if similar accessibility tests exist elsewhere:
#!/bin/bash # Description: Check for moved or similar tests # Test: Search for "Authenticated landing page view" in other test files rg --type typescript 'Authenticated landing page view' e2e # Test: Search for image accessibility tests in other files rg --type typescript 'image.*accessibility' e2eIf these tests haven't been moved or replaced, consider reintroducing the image accessibility tests to maintain comprehensive coverage.
4-22: Consider reverting test case name and review logic changes.
The test case name change from "Authenticated homepage view" to "Homepage view" reduces specificity. Consider keeping "Authenticated" in the name for consistency with the suite name and to distinguish it from unauthenticated tests.
The logic changes for checking the "Popular topics" visibility are more robust, accounting for both mobile and non-mobile views. However, ensure that this change doesn't introduce any unintended side effects.
Consider reverting the test name:
- test("Homepage view", async ({ page, isMobile }) => { + test("Authenticated homepage view", async ({ page, isMobile }) => {To ensure the changes don't affect other tests, run the following script:
#!/bin/bash # Description: Check for other occurrences of "Popular topics" in tests # Test: Search for "Popular topics" in other test files rg --type typescript 'Popular topics' e2ee2e/auth.setup.ts (1)
31-31: Excellent addition: Verifying E2E_USER_SESSION_ID is definedThis change enhances the robustness of the authentication setup by ensuring that the
E2E_USER_SESSION_IDenvironment variable is defined before proceeding. It's a good practice to fail early if required configuration is missing, preventing potential issues later in the test execution.
| test("Shared content", async ({ page }) => { | ||
| const imagesWithoutAltText = await page.$$eval( | ||
| "img:not([alt])", | ||
| (images) => images.length, | ||
| ); | ||
| expect(imagesWithoutAltText).toBe(0); // All images should have alt text | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the test case implementation.
The test case effectively checks for alt text on images, which is crucial for accessibility. However, there are a couple of points to consider:
-
The test doesn't navigate to the homepage before performing the check. Consider adding a
page.goto()call at the beginning of the test. -
The test title "Shared content" might not be the most descriptive for this specific check. A more explicit title like "All images should have alt text" would better describe the test's purpose.
Here's a suggested improvement:
test("All images should have alt text", async ({ page }) => {
await page.goto("/"); // Navigate to the homepage
const imagesWithoutAltText = await page.$$eval(
"img:not([alt])",
(images) => images.length,
);
expect(imagesWithoutAltText).toBe(0);
});This change ensures the test is performed on the homepage and provides a more descriptive test title.
| test("Sign up page contains sign up links", async ({ page, isMobile }) => { | ||
| // authenticated users are kicked back to the homepage if they try to go to /get-started | ||
| await page.goto("http://localhost:3000/get-started"); | ||
| expect(page.url()).toEqual("http://localhost:3000/"); |
There was a problem hiding this comment.
Await the URL assertion to ensure navigation completion
The assertion expect(page.url()).toEqual("http://localhost:3000/"); should be awaited to ensure that the URL has fully updated before the assertion runs. Without await, the test might pass prematurely.
Update the code to use await with toHaveURL for better reliability:
-expect(page.url()).toEqual("http://localhost:3000/");
+await expect(page).toHaveURL("http://localhost:3000/");📝 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.
| expect(page.url()).toEqual("http://localhost:3000/"); | |
| await expect(page).toHaveURL("http://localhost:3000/"); |
| await expect(page.getByText("CodúBetaSign in or create")).toBeHidden(); | ||
| await expect( | ||
| page.getByRole("heading", { name: "Sign in or create your account" }), | ||
| ).toBeHidden(); |
There was a problem hiding this comment.
Correct the expected text in the assertion
Similarly, in the authenticated tests, the text "CodúBetaSign in or create" may be missing a space. Ensure the expected text matches the actual on-page text to prevent false negatives in your tests.
Update the assertion:
-await expect(page.getByText("CodúBetaSign in or create")).toBeHidden();
+await expect(page.getByText("CodúBeta Sign in or create")).toBeHidden();📝 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.getByText("CodúBetaSign in or create")).toBeHidden(); | |
| await expect( | |
| page.getByRole("heading", { name: "Sign in or create your account" }), | |
| ).toBeHidden(); | |
| await expect(page.getByText("CodúBeta Sign in or create")).toBeHidden(); | |
| await expect( | |
| page.getByRole("heading", { name: "Sign in or create your account" }), | |
| ).toBeHidden(); |
| await expect(page.getByText("CodúBetaSign in or create")).toBeVisible(); | ||
| await expect( | ||
| page.getByRole("heading", { name: "Sign in or create your account" }), | ||
| ).toBeVisible(); |
There was a problem hiding this comment.
Correct the expected text in the assertion
The text "CodúBetaSign in or create" seems to be missing a space between "CodúBeta" and "Sign in or create". This may cause the test to fail if the actual text on the page includes a space. Please verify the expected text for accuracy.
Consider updating the assertion:
-await expect(page.getByText("CodúBetaSign in or create")).toBeVisible();
+await expect(page.getByText("CodúBeta Sign in or create")).toBeVisible();📝 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.getByText("CodúBetaSign in or create")).toBeVisible(); | |
| await expect( | |
| page.getByRole("heading", { name: "Sign in or create your account" }), | |
| ).toBeVisible(); | |
| await expect(page.getByText("CodúBeta Sign in or create")).toBeVisible(); | |
| await expect( | |
| page.getByRole("heading", { name: "Sign in or create your account" }), | |
| ).toBeVisible(); |
| test("Sign up page contains sign up links", async ({ page, isMobile }) => { | ||
| // authenticated users are kicked back to the homepage if they try to go to /get-started | ||
| await page.goto("http://localhost:3000/get-started"); | ||
| expect(page.url()).toEqual("http://localhost:3000/"); | ||
| await expect(page.getByText("CodúBetaSign in or create")).toBeHidden(); | ||
| await expect( | ||
| page.getByRole("heading", { name: "Sign in or create your account" }), | ||
| ).toBeHidden(); | ||
| await expect(page.getByRole("link", { name: "return home" })).toBeHidden(); | ||
| if (!isMobile) { | ||
| await expect( | ||
| page.getByRole("button", { name: "Sign up for free" }), | ||
| ).toBeHidden(); | ||
| await expect( | ||
| page.getByRole("button", { name: "Sign in", exact: true }), | ||
| ).toBeHidden(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Rename the test to reflect its purpose
The test "Sign up page contains sign up links" under the "Authenticated Login Page" suite is checking that certain elements are hidden and that authenticated users are redirected. The test name might be misleading.
Consider renaming the test for clarity:
-test("Sign up page contains sign up links", async ({ page, isMobile }) => {
+test("Authenticated users are redirected from get-started page", async ({ page, isMobile }) => {📝 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.
| test("Sign up page contains sign up links", async ({ page, isMobile }) => { | |
| // authenticated users are kicked back to the homepage if they try to go to /get-started | |
| await page.goto("http://localhost:3000/get-started"); | |
| expect(page.url()).toEqual("http://localhost:3000/"); | |
| await expect(page.getByText("CodúBetaSign in or create")).toBeHidden(); | |
| await expect( | |
| page.getByRole("heading", { name: "Sign in or create your account" }), | |
| ).toBeHidden(); | |
| await expect(page.getByRole("link", { name: "return home" })).toBeHidden(); | |
| if (!isMobile) { | |
| await expect( | |
| page.getByRole("button", { name: "Sign up for free" }), | |
| ).toBeHidden(); | |
| await expect( | |
| page.getByRole("button", { name: "Sign in", exact: true }), | |
| ).toBeHidden(); | |
| } | |
| }); | |
| test("Authenticated users are redirected from get-started page", async ({ page, isMobile }) => { | |
| // authenticated users are kicked back to the homepage if they try to go to /get-started | |
| await page.goto("http://localhost:3000/get-started"); | |
| expect(page.url()).toEqual("http://localhost:3000/"); | |
| await expect(page.getByText("CodúBetaSign in or create")).toBeHidden(); | |
| await expect( | |
| page.getByRole("heading", { name: "Sign in or create your account" }), | |
| ).toBeHidden(); | |
| await expect(page.getByRole("link", { name: "return home" })).toBeHidden(); | |
| if (!isMobile) { | |
| await expect( | |
| page.getByRole("button", { name: "Sign up for free" }), | |
| ).toBeHidden(); | |
| await expect( | |
| page.getByRole("button", { name: "Sign in", exact: true }), | |
| ).toBeHidden(); | |
| } | |
| }); |
| page.getByRole("heading", { name: "Popular topics" }), | ||
| ).toBeVisible({ visible: !isMobile }); |
There was a problem hiding this comment.
Correct improper usage of toBeVisible() method
The toBeVisible() assertion does not accept a visible option parameter. To conditionally check visibility based on isMobile, you should use .toBeVisible() or .not.toBeVisible() accordingly.
Apply this diff to fix the assertions:
// For lines 11-12
- ).toBeVisible({ visible: !isMobile });
+ if (isMobile) {
+ await expect(
+ page.getByRole("heading", { name: "Popular topics" }),
+ ).not.toBeVisible();
+ } else {
+ await expect(
+ page.getByRole("heading", { name: "Popular topics" }),
+ ).toBeVisible();
+ }
// For lines 15-16
- ).toBeVisible({ visible: !isMobile });
+ if (isMobile) {
+ await expect(
+ page.getByRole("link", { name: '"Codú Writing Challenge" text' }),
+ ).not.toBeVisible();
+ } else {
+ await expect(
+ page.getByRole("link", { name: '"Codú Writing Challenge" text' }),
+ ).toBeVisible();
+ }
// For lines 67-68
- ).toBeVisible({ visible: !isMobile });
+ if (isMobile) {
+ await expect(
+ page.getByRole("heading", { name: "Popular topics" }),
+ ).not.toBeVisible();
+ } else {
+ await expect(
+ page.getByRole("heading", { name: "Popular topics" }),
+ ).toBeVisible();
+ }
// For lines 71-72
- ).toBeVisible({ visible: !isMobile });
+ if (isMobile) {
+ await expect(
+ page.getByRole("link", { name: '"Codú Writing Challenge" text' }),
+ ).not.toBeVisible();
+ } else {
+ await expect(
+ page.getByRole("link", { name: '"Codú Writing Challenge" text' }),
+ ).toBeVisible();
+ }Also applies to: 15-16, 67-68, 71-72
| }); | ||
| }); | ||
|
|
||
| test.describe("Authenticated Articles Page", () => { |
There was a problem hiding this comment.
Set up authentication context in authenticated tests
The authenticated tests need to ensure that the user is authenticated before running the tests. Currently, there is no beforeEach hook to set up the authentication context.
Consider adding a beforeEach hook to authenticate the user before each test:
test.beforeEach(async ({ page }) => {
// Replace with your authentication logic
await page.goto("http://localhost:3000/login");
await page.fill('input[name="email"]', 'user@example.com');
await page.fill('input[name="password"]', 'password');
await page.click('button[type="submit"]');
await page.waitForNavigation();
});If you have a helper function or fixture for authentication, you can use that instead to keep your tests DRY.
|
|
||
| if (!isMobile) { | ||
| await page.getByText("Code Of Conduct").scrollIntoViewIfNeeded(); | ||
| await page.waitForTimeout(5000); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using fixed delays with waitForTimeout; use proper waiting mechanisms
Using await page.waitForTimeout(5000); introduces fixed delays which can slow down tests and may not be reliable. Consider using more robust waiting methods like waiting for network idle or waiting for specific elements to appear.
Apply this diff to improve the test:
- await page.waitForTimeout(5000);
+ await page.waitForLoadState('networkidle');Alternatively, wait for a specific element that appears after loading more articles:
- await page.waitForTimeout(5000);
+ await page.waitForSelector("article:nth-child(${initialArticleCount + 1})");Committable suggestion was skipped due to low confidence.
NiallJoeMaher
temporarily deployed
to
production
GitHub Actions
Inactive
✨ Codu Pull Request 💻
Fixes #(issue)
More work on #468. Mostly just hardening whats there but some additions
Pull Request details
Any Breaking changes
Associated Screenshots