-
-
Notifications
You must be signed in to change notification settings - Fork 146
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
chore: added article creation e2e test #1139
chore: added article creation e2e test #1139
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 pull request introduces a new test case to the "Authenticated Articles Page" test suite in the 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- e2e/articles.spec.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
e2e/articles.spec.ts (2)
165-176
: Verify that the article content assertions are accurateThe test checks for specific text in the article, such as "Lorem ipsum dolor sit amet," which might be sensitive to changes in the content or formatting.
Ensure that the content you're asserting matches the actual rendered content. If the content includes dynamic elements or formatting, consider using more robust selectors or text matching strategies.
128-129
:⚠️ Potential issueEnsure the user is authenticated before testing article publication
The test assumes that the user is authenticated but does not include any authentication steps. Ensure that the test logs in the user or that authentication is handled in a setup step.
To verify if user authentication is handled elsewhere in the test suite, run the following script:
This script searches for authentication-related code within the test suite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🌮🌮🌮 LGTM!
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
e2e/articles.spec.ts (3)
133-148
: Consider adding an explicit wait after navigationThe navigation logic looks good and correctly handles both mobile and desktop scenarios. However, to improve test reliability, consider adding an explicit wait for an element on the article creation page after navigation.
Apply this diff to add an explicit wait:
await page.waitForURL("http:/localhost:3000/create"); +await page.waitForSelector('input[placeholder="Article title"]');
This ensures that the article creation form is fully loaded before proceeding with the test.
150-161
: Add content validation after filling the formThe article creation process looks good overall. To enhance the test's robustness, consider adding validation steps after filling in the title and content.
Apply this diff to add content validation:
await page.getByPlaceholder("Article title").fill(articleTitle); +await expect(page.getByPlaceholder("Article title")).toHaveValue(articleTitle); await page .getByPlaceholder("Enter your content here 💖") .fill(articleContent); +await expect(page.getByPlaceholder("Enter your content here 💖")).toHaveValue(articleContent);This ensures that the content is correctly entered before proceeding with publication.
166-178
: Consider verifying the full article contentThe verification of the published article is thorough and covers all important aspects. To further enhance the test, consider adding a check for the full article content.
Apply this diff to add full content verification:
await expect(page.getByText("Lorem ipsum dolor sit amet,")).toBeVisible(); +await expect(page.locator('article')).toContainText(articleContent);
This ensures that the entire article content is correctly published and displayed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- e2e/articles.spec.ts (1 hunks)
- playwright.config.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
e2e/articles.spec.ts (1)
Learnt from: JohnAllenTech PR: codu-code/codu#1139 File: e2e/articles.spec.ts:133-147 Timestamp: 2024-10-17T01:03:59.199Z Learning: In `e2e/articles.spec.ts` (TypeScript Playwright tests), avoid refactoring duplicated code into helper functions unless the code is reused elsewhere.
🔇 Additional comments (4)
playwright.config.ts (1)
21-21
: Approved: Increased workers in CI, but clarification needed.The change to increase the number of workers from 2 to 3 in the CI environment can potentially improve test execution speed. However, I have a few points to consider:
Could you please clarify the reason for this specific change? Is it necessary for the new article creation e2e tests, or is it a general performance improvement?
Have you verified that the CI environment has sufficient resources to handle the additional worker without issues?
Consider adding a comment in the code to explain why 3 workers are used in CI. This can help future maintainers understand the reasoning behind this configuration.
To ensure this change doesn't negatively impact the CI pipeline, please run the following command in your CI environment and verify the results:
This will help confirm that the CI environment can handle the increased number of workers.
e2e/articles.spec.ts (3)
128-132
: LGTM: Test setup and content definitionThe test setup and content definition look good. Using constants for the article content and title improves maintainability.
162-164
: LGTM: URL verification after publicationThe URL verification after article publication looks good. Using a regex to match the dynamic article URL is an appropriate approach.
128-178
: Overall assessment: Well-implemented E2E test for article creation and publicationThis new test case is a valuable addition to the test suite, covering the entire process of writing and publishing an article for both mobile and desktop scenarios. The test is well-structured and includes appropriate verifications at each step.
Key strengths:
- Handles both mobile and desktop navigation paths.
- Verifies important elements of the published article.
- Uses regex for dynamic URL matching.
Suggested improvements:
- Add an explicit wait after navigation to the article creation page.
- Validate entered content in the article form.
- Verify the full article content after publication.
These enhancements will further improve the test's reliability and thoroughness.
✨ Codu Pull Request 💻
Fixes #(issue)
Adds to #468
Pull Request details
Any Breaking changes
Associated Screenshots