feat: adding e2e test for article commenting #1158
Conversation
|
@JohnAllenTech is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
|
Caution Review failedThe pull request is closed. WalkthroughThe changes made in this pull request enhance the end-to-end testing suite for the articles page. New test cases are introduced for both unauthenticated and authenticated users regarding comment functionality. The configuration file for Playwright testing is also updated to improve project name clarity, renaming "chromium" to "Desktop Chrome" and "firefox" to "Desktop Firefox," while maintaining existing settings. Changes
Possibly related PRs
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
JohnAllenTech
changed the title
JohnAllenTech
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
playwright.config.ts (1)
49-49: Consistent naming improvement for Firefox projectThe renaming from "firefox" to "Desktop Firefox" is a good change, consistent with the Chrome project naming. This improves clarity and maintains a uniform naming convention.
For complete consistency, consider updating the names of the "Mobile Chrome" and "Mobile Safari" projects to follow the same pattern, e.g., "Mobile Chrome" to "Mobile Android Chrome" and "Mobile Safari" to "Mobile iOS Safari". This would provide even more explicit information about the test environments.
e2e/articles.spec.ts (1)
209-211: Remove unused 'browserName' parameterThe
browserNameparameter is not used within the test and can be safely removed to clean up the code.Apply this diff to remove the unused parameter:
- test("Should post a comment on an article", async ({ - page, - browserName, - }, workerInfo) => { + test("Should post a comment on an article", async ({ page }, workerInfo) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- e2e/articles.spec.ts (3 hunks)
- playwright.config.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
playwright.config.ts (2)
40-40: Improved project naming for clarityThe change from "chromium" to "Desktop Chrome" enhances readability and provides a more explicit description of the test environment. This aligns well with best practices in configuration naming.
Line range hint
1-85: Overall improvement in configuration clarityThe changes made to this configuration file enhance the readability and maintainability of the Playwright test setup. By providing more explicit names for the test projects, it becomes easier for developers to understand the purpose and environment of each test configuration at a glance.
These modifications align well with the PR objectives of improving E2E testing, as they contribute to a more organized and clear testing structure. The functional aspects of the configuration remain unchanged, ensuring that existing tests will continue to run as before.
e2e/articles.spec.ts (1)
2-2: Importing 'randomUUID' from 'crypto' is appropriateThe import statement is necessary for generating unique identifiers in your tests.
JohnAllenTech
temporarily deployed
to
production
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
e2e/articles.spec.ts (1)
62-78: LGTM: New test case for unauthenticated usersThe new test case effectively verifies that unauthenticated users cannot post comments on articles. It checks for the appropriate UI elements and messages.
For consistency with other tests, consider using
toBeVisible()instead oftoBeHidden()on line 71:- await expect(page.getByPlaceholder("What do you think?")).toBeHidden(); + await expect(page.getByPlaceholder("What do you think?")).not.toBeVisible();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- e2e/articles.spec.ts (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
e2e/articles.spec.ts (2)
2-2: LGTM: Import of randomUUIDThe addition of the
randomUUIDimport from thecryptomodule is appropriate for generating unique identifiers in the new test case.
Line range hint
1-221: LGTM: Overall structure and consistencyThe new test cases are well-integrated into the existing test suite, maintaining the file's structure and readability. They follow the established patterns and conventions, which is commendable.
| test("Should post a comment on an article", async ({ page }, workerInfo) => { | ||
| const commentContent = `This is a great read. Thanks for posting! Sent from ${workerInfo.project.name} + ${randomUUID()}`; | ||
| await page.goto("http://localhost:3000"); | ||
| // Waits for articles to be loaded | ||
| await expect(page.getByText("Read Full Article").first()).toBeVisible(); | ||
| await page.getByText("Read Full Article").first().click(); | ||
| await page.waitForURL(/^http:\/\/localhost:3000\/articles\/.*$/); | ||
|
|
||
| await expect(page.getByPlaceholder("What do you think?")).toBeVisible(); | ||
| await page.getByPlaceholder("What do you think?").fill(commentContent); | ||
| await page.getByRole("button", { name: "Submit" }).click(); | ||
|
|
||
| await expect(page.getByText(commentContent)).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
LGTM: New test case for authenticated users
The new test case effectively verifies that authenticated users can post comments on articles. The use of randomUUID() for generating unique comments is a good practice.
There's a missing await before the click() method on line 213. This issue was flagged in a previous review. Please apply the following fix:
- await page.getByText("Read Full Article").first().click();
+ await page.getByText("Read Full Article").first().click();📝 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("Should post a comment on an article", async ({ page }, workerInfo) => { | |
| const commentContent = `This is a great read. Thanks for posting! Sent from ${workerInfo.project.name} + ${randomUUID()}`; | |
| await page.goto("http://localhost:3000"); | |
| // Waits for articles to be loaded | |
| await expect(page.getByText("Read Full Article").first()).toBeVisible(); | |
| await page.getByText("Read Full Article").first().click(); | |
| await page.waitForURL(/^http:\/\/localhost:3000\/articles\/.*$/); | |
| await expect(page.getByPlaceholder("What do you think?")).toBeVisible(); | |
| await page.getByPlaceholder("What do you think?").fill(commentContent); | |
| await page.getByRole("button", { name: "Submit" }).click(); | |
| await expect(page.getByText(commentContent)).toBeVisible(); | |
| }); | |
| test("Should post a comment on an article", async ({ page }, workerInfo) => { | |
| const commentContent = `This is a great read. Thanks for posting! Sent from ${workerInfo.project.name} + ${randomUUID()}`; | |
| await page.goto("http://localhost:3000"); | |
| // Waits for articles to be loaded | |
| await expect(page.getByText("Read Full Article").first()).toBeVisible(); | |
| await page.getByText("Read Full Article").first().click(); | |
| await page.waitForURL(/^http:\/\/localhost:3000\/articles\/.*$/); | |
| await expect(page.getByPlaceholder("What do you think?")).toBeVisible(); | |
| await page.getByPlaceholder("What do you think?").fill(commentContent); | |
| await page.getByRole("button", { name: "Submit" }).click(); | |
| await expect(page.getByText(commentContent)).toBeVisible(); | |
| }); |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
NiallJoeMaher
temporarily deployed
to
production
GitHub Actions
Inactive
✨ Codu Pull Request 💻
Improves E2E testing for the project by adding tests around a user commenting on an article
Pull Request details
Any Breaking changes
Associated Screenshots
Tests passing
Example comment generated by E2E test. The uuid needs to be added as if multiple a user runs the test suite multiple times we want to always ensure we are finding the comment created for that specific test run