Fix/e2e tests #1061
Fix/e2e tests #1061
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 significant modifications to the 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 (
|
![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 (7)
e2e/login.spec.ts (2)
Line range hint
4-10: Approve navigation step, but note inconsistency in button names.The addition of the explicit navigation step to the "get-started" page is good. It makes the test more self-contained and easier to understand.
However, there's an inconsistency in the button names between this test case and the second one. Here, it's "Login with GitHub", while in the second test case, it's "Continue with GitHub". This inconsistency should be addressed to ensure the tests are checking for the correct button text.
Consider updating the button name to match the one used in the second test case:
- name: "Login with GitHub", + name: "Continue with GitHub",
Line range hint
12-29: Approve changes, suggest minor formatting adjustment.The changes in this test case look good:
- The explicit navigation to the "get-started" page improves test clarity.
- The button name update to "Continue with GitHub" is correct and consistent with the AI summary.
- The test logic remains comprehensive, checking for correct navigation and presence of login fields.
Consider removing the empty line at line 16 for consistency with the rest of the file:
test("should navigate to GitHub login page when clicking the login button", async ({ page, }) => { await page.goto("http://localhost:3000/get-started"); - const button = page.getByRole("button", { name: "Continue with GitHub", });e2e/articles.spec.ts (4)
16-16: Good addition to stabilize the test, consider adding a short wait.The new line scrolling the "Code Of Conduct" text into view is a good addition to stabilize the test. To further enhance stability, consider adding a short wait after this action to ensure the page has settled before scrolling.
You could modify the line as follows:
await page.getByText("Code Of Conduct").scrollIntoViewIfNeeded(); await page.waitForTimeout(500); // Add a short wait to ensure the page has settled
Line range hint
7-7: Consider using an environment variable for the URL.Using a hard-coded URL might cause issues when running tests in different environments (e.g., staging, production).
Consider using an environment variable for the base URL:
await page.goto(`${process.env.BASE_URL}/articles`);Don't forget to set up the
BASE_URLenvironment variable in your test configuration or CI/CD pipeline.
Line range hint
1-30: Add error handling and timeout specifications to improve test reliability.The test currently lacks error handling and specific timeout settings, which could lead to flakiness in slow network conditions or when unexpected errors occur.
Consider adding try-catch blocks and specific timeout settings:
test("Should load more articles when scrolling to the end of the page", async ({ page, }) => { try { await page.goto("http://localhost:3000/articles", { timeout: 30000 }); await page.waitForSelector("article", { timeout: 10000 }); // ... rest of the test code ... } catch (error) { console.error("Test failed:", error); throw error; } });Also, consider adding
test.setTimeout(60000);at the beginning of the test to set a global timeout for this specific test case if needed.
Line range hint
27-28: Make the assertion for article count increase more specific.The current assertion only checks if the final count is greater than the initial count. This could pass even if only one additional article is loaded, which might not be the expected behavior.
Consider making the assertion more specific:
const expectedMinimumIncrease = 5; // Adjust this number based on your actual requirements expect(finalArticleCount).toBeGreaterThanOrEqual(initialArticleCount + expectedMinimumIncrease);This ensures that a minimum number of articles are loaded, making the test more robust and meaningful.
playwright.config.ts (1)
Line range hint
1-82: Overall configuration looks good with room for improvementThe Playwright configuration file demonstrates several good practices:
- Using the HTML reporter for detailed test results.
- Setting up a
webServerconfiguration to run the dev server before tests.- Providing examples of configurations for different browsers and devices (albeit commented out).
To further improve the configuration:
- Consider uncommenting and utilizing more browser configurations (Firefox, WebKit) for broader coverage.
- Evaluate if the
fullyParallel: truesetting is optimal for your specific test suite and CI environment.- Review the
retriesandworkerssettings to ensure they align with your team's testing strategy and CI capabilities.These suggestions can help enhance the robustness and efficiency of your E2E testing setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
.github/workflows/playwright.ymlis excluded by!**/*.ymlpackage-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonpackage.jsonis excluded by!**/*.json
📒 Files selected for processing (5)
- e2e/articles.spec.ts (1 hunks)
- e2e/home.spec.ts (1 hunks)
- e2e/login.spec.ts (2 hunks)
- playwright.config.ts (1 hunks)
- server/api/router/post.ts (1 hunks)
🔇 Additional comments (3)
e2e/login.spec.ts (1)
Line range hint
1-29: Overall improvements align with PR objectives, minor consistency issue noted.The changes in this file align well with the PR objectives of fixing and stabilizing E2E tests:
- Removal of hooks and addition of explicit navigation steps can help reduce test flakiness and improve test clarity.
- The tests cover important aspects of the login functionality, including button presence and navigation to GitHub's login page.
However, there's a minor consistency issue with the button names between the two test cases that should be addressed.
To ensure consistency across the codebase, let's verify the correct button text:
This will help determine which button text is used consistently across the project.
e2e/home.spec.ts (2)
Line range hint
1-45: Overall assessment of e2e/home.spec.tsThe change in this file appears to be in line with the PR objectives of fixing and stabilizing E2E tests. The update to the locator text is the only modification, and it seems appropriate if it reflects a UI change.
The rest of the file remains unchanged and contains well-structured tests for various aspects of the homepage, including content verification and image accessibility checks.
24-24: Verify UI update and consider adding a comment.The locator text has been updated from "Recommended topics" to "Popular topics". This change looks good if it reflects an actual UI update.
- Please confirm that this change matches the current UI of the application.
- Consider adding a comment explaining the reason for this change, which would improve code maintainability.
To ensure consistency across the codebase, let's verify if this change is reflected elsewhere:
This script will help identify any inconsistencies or places where the text might need to be updated.
✅ Verification successful
Verified UI update and comments.
The locator text has been updated from "Recommended topics" to "Popular topics", and this change aligns with the current UI of the application.
Consider adding a comment explaining the reason for this change to improve code maintainability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for occurrences of both "Recommended topics" and "Popular topics" # in the codebase to ensure consistency. echo "Searching for 'Recommended topics':" rg --type-add 'web:*.{js,ts,jsx,tsx,html,css}' --type web "Recommended topics" echo "\nSearching for 'Popular topics':" rg --type-add 'web:*.{js,ts,jsx,tsx,html,css}' --type web "Popular topics"Length of output: 492
|
Let me know when this is no longer WIP! |
|
Working on reusing a single login across multiple tests. Currently hitting this after maybe ~10 test suite runs
|
|
Uh oh! @JohnAllenTech, the image you shared is missing helpful alt text. Check #1061 (comment). Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs. |
|
@JohnAllenTech can we try making login/auth specific tests serial so they don't rate limit us. Or move out the auth tests to a separate project and run them standalone with a different GitHub oauth app? |
|
This is actually a nightmare 🤣 Current state is
Once auth passes the rest of the tests are fine. |
|
What a strange one... |
|
@JohnAllenTech I was not aware of this PR status so I created one for e2e and have some work there, PR link #1086 |
|
@praveen-quizizz no issues. I see you havent tackled the authentication setup yet. Did you plan to use a GH oauth app for this? Are you referring to a GH oauth app or a GH app ? They are actually two different things. How would you plan to do the GH SSO flow if you authenticate with a GH app? My reason for asking is I am running out of ideas to get the GH SSO + Next Auth working. It seems to be a known issue. |
|
My last idea on this is a little hacky. Create a session for the user manually using drizzle. If both of these are their it will appear as if the user has logged in using a valid authN prcoess... (I think) |
…t level or a run level
JohnAllenTech
had a problem deploying
to
production
GitHub Actions
Failure
JohnAllenTech
temporarily deployed
to
production
GitHub Actions
Inactive
JohnAllenTech
temporarily deployed
to
production
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
playwright.config.ts (1)
38-42: Approve changes to "chromium" project and suggest improvementThe modifications to the "chromium" project, including the addition of
storageStateand dependency on the "setup" project, are good improvements. These changes should help address the authentication issues mentioned in the PR objectives.Consider adding a comment explaining the purpose of the
storageStatefile and its relationship to the authentication process. This would improve code maintainability. For example:use: { ...devices["Desktop Chrome"], // Use the authenticated state created by the setup project storageState: "playwright/.auth/browser.json", },drizzle/seed.ts (1)
12-17: Consider removing hardcoded E2E constants in a future PRWhile the hardcoded constants for E2E testing are acceptable temporarily, please remember to remove them and switch to using environment variables in a follow-up PR to enhance security and configurability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- drizzle/seed.ts (3 hunks)
- e2e/home.spec.ts (1 hunks)
- e2e/login.spec.ts (1 hunks)
- playwright.config.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/home.spec.ts
- e2e/login.spec.ts
🧰 Additional context used
🔇 Additional comments (4)
playwright.config.ts (2)
35-35: Approve addition of "setup" project for authenticationThe addition of a "setup" project specifically for authentication tests is a good practice. This aligns with the PR objectives of improving E2E test stability and addressing authentication issues.
Could you provide more details about the
auth.setup.tsfile and how it handles the authentication process? This information would be helpful for a more comprehensive review.
21-21: Consider the impact of increasing CI workersIncreasing the number of workers from 1 to 2 for CI environments could potentially improve test execution speed. However, this change might also lead to increased resource usage and potential instability if the CI environment is not properly configured to handle parallel test execution.
To ensure this change doesn't negatively impact your CI pipeline, please run the following script to check the CI environment's capabilities:
drizzle/seed.ts (2)
119-147: FunctionseedE2EUsercorrectly seeds the E2E test userThe implementation for
seedE2EUsercorrectly checks for the existence of the E2E test user and creates one if it doesn't exist. This ensures that the E2E tests have a consistent user to operate with.
252-253: E2E user and session seeding integrated correctlyThe calls to
seedE2EUser()andseedE2EUserSession(user.id)ensure that the E2E test user and session are properly seeded during the data seeding process, which is essential for the E2E tests to function reliably.
| const seedE2EUserSession = async (userId: string) => { | ||
| const [existingE2EUserSession] = await db | ||
| .selectDistinct() | ||
| .from(session) | ||
| .where(eq(session.sessionToken, E2E_SESSION_ID)); | ||
|
|
||
| if (existingE2EUserSession) { | ||
| console.log("E2E Test session already exists. Skipping creation"); | ||
| return existingE2EUserSession; | ||
| } | ||
|
|
||
| try { | ||
| const currentDate = new Date(); | ||
|
|
||
| return await db | ||
| .insert(session) | ||
| .values({ | ||
| userId, | ||
| sessionToken: E2E_SESSION_ID, | ||
| // Set session to expire in 6 months. | ||
| expires: new Date(currentDate.setMonth(currentDate.getMonth() + 6)), | ||
| }) | ||
| .returning(); | ||
| } catch (err) { | ||
| console.log(err); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Handle errors appropriately in seedE2EUserSession
In the seedE2EUserSession function, the catch block logs the error but does not rethrow or return a value. This could lead to unexpected behavior if an error occurs. Consider rethrowing the error or returning a default value to ensure that any issues are properly propagated.
Apply this diff to improve error handling:
} catch (err) {
console.log(err);
+ throw err;
}📝 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.
| const seedE2EUserSession = async (userId: string) => { | |
| const [existingE2EUserSession] = await db | |
| .selectDistinct() | |
| .from(session) | |
| .where(eq(session.sessionToken, E2E_SESSION_ID)); | |
| if (existingE2EUserSession) { | |
| console.log("E2E Test session already exists. Skipping creation"); | |
| return existingE2EUserSession; | |
| } | |
| try { | |
| const currentDate = new Date(); | |
| return await db | |
| .insert(session) | |
| .values({ | |
| userId, | |
| sessionToken: E2E_SESSION_ID, | |
| // Set session to expire in 6 months. | |
| expires: new Date(currentDate.setMonth(currentDate.getMonth() + 6)), | |
| }) | |
| .returning(); | |
| } catch (err) { | |
| console.log(err); | |
| } | |
| }; | |
| const seedE2EUserSession = async (userId: string) => { | |
| const [existingE2EUserSession] = await db | |
| .selectDistinct() | |
| .from(session) | |
| .where(eq(session.sessionToken, E2E_SESSION_ID)); | |
| if (existingE2EUserSession) { | |
| console.log("E2E Test session already exists. Skipping creation"); | |
| return existingE2EUserSession; | |
| } | |
| try { | |
| const currentDate = new Date(); | |
| return await db | |
| .insert(session) | |
| .values({ | |
| userId, | |
| sessionToken: E2E_SESSION_ID, | |
| // Set session to expire in 6 months. | |
| expires: new Date(currentDate.setMonth(currentDate.getMonth() + 6)), | |
| }) | |
| .returning(); | |
| } catch (err) { | |
| console.log(err); | |
| throw err; | |
| } | |
| }; |
JohnAllenTech
had a problem deploying
to
production
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
README.md (3)
100-113: LGTM: Improved GitHub authentication setup instructionsThe expanded instructions and added screenshots for setting up GitHub OAuth are excellent additions. They provide clear, step-by-step guidance that will help users avoid common setup pitfalls.
Consider changing "setup" to "set up" on line 102 for grammatical correctness:
-For development, make sure you setup this with a **Homepage URL** of +For development, make sure you set up this with a **Homepage URL** of🧰 Tools
🪛 LanguageTool
[grammar] ~102-~102: The word “setup” is a noun. The verb is spelled with a space.
Context: ... below. For development, make sure you setup this with a Homepage URL of ``` ht...(NOUN_VERB_CONFUSION)
🪛 Markdownlint
104-104: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
110-110: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
144-153: LGTM with suggestions: New End-to-End Testing sectionThe addition of the End-to-End Testing section with Playwright is excellent. It provides clear instructions for setting up the environment, seeding the database, and running the tests in both headless and UI modes.
The descriptions of the E2E environment variables (lines 144-153) could be improved for clarity. Consider updating them as follows:
### E2E_USER_SESSION_ID - -This is the sessionToken uuid that . -This is currently hardcoded and there is no reason to change this until we require multiple E2E test users within the same test suite + +This is the UUID used as the sessionToken for E2E test authentication. +It's currently hardcoded and doesn't need to be changed unless multiple E2E test users are required within the same test suite. ### E2E_USER_ID - -This is the userId if the E2E user used for testing . -This is currently hardcoded and there is no reason to change this until we require multiple E2E test users within the same test suite + +This is the userId of the E2E user used for testing. +It's currently hardcoded and doesn't need to be changed unless multiple E2E test users are required within the same test suite. + +### E2E_USER_EMAIL + +This is the email address of the E2E user used for testing. +It's currently hardcoded and doesn't need to be changed unless multiple E2E test users are required within the same test suite.These changes provide more context and clarity about the purpose and usage of these environment variables.
Also applies to: 166-205
104-104: Minor formatting improvementsTo enhance the readability and consistency of the document, consider making the following changes:
- Add language specifications to the code blocks:
-``` +```plaintext http://localhost:3000/-
+plaintext
http://localhost:3000/api/auth-``` +```bash npm run db:seed2. Remove the loose punctuation marks at the end of list items: ```diff -- `E2E_USER_ID`: The id of the E2E user for testing. -- `E2E_USER_EMAIL`: The email of the E2E user for testing. -- `E2E_USER_SESSION_ID`: The session id that the user will use to authenticate. +- `E2E_USER_ID`: The id of the E2E user for testing +- `E2E_USER_EMAIL`: The email of the E2E user for testing +- `E2E_USER_SESSION_ID`: The session id that the user will use to authenticateThese changes will improve the overall formatting and consistency of the README.
Also applies to: 110-110, 174-176, 185-185
🧰 Tools
🪛 Markdownlint
104-104: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
.github/workflows/e2e-tests.ymlis excluded by!**/*.ymlpackage-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonpackage.jsonis excluded by!**/*.json
📒 Files selected for processing (2)
- README.md (5 hunks)
- e2e/home.spec.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/home.spec.ts
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~102-~102: The word “setup” is a noun. The verb is spelled with a space.
Context: ... below. For development, make sure you setup this with a Homepage URL of ``` ht...(NOUN_VERB_CONFUSION)
[uncategorized] ~174-~174: Loose punctuation mark.
Context: ...et in your.envfile: -E2E_USER_ID: The id of the E2E user for testing. - `...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~175-~175: Loose punctuation mark.
Context: ...E2E user for testing. -E2E_USER_EMAIL: The email of the E2E user for testing. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~176-~176: Loose punctuation mark.
Context: ...ser for testing. -E2E_USER_SESSION_ID: The session id that the user will use t...(UNLIKELY_OPENING_PUNCTUATION)
🪛 Markdownlint
README.md
104-104: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
110-110: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
185-185: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
🔇 Additional comments (2)
README.md (2)
29-29: LGTM: Improved installation instructionsThe additions to the installation instructions are helpful. The note about checking the Node version against
.nvmrcand the clarified instructions for creating the.envfile and seeding the database will help users set up the project more easily.Also applies to: 53-53, 61-61
Line range hint
1-247: LGTM: Significantly improved READMEThe changes made to this README file are substantial and greatly improve its usefulness. Key improvements include:
- Clearer installation instructions with notes on Node version compatibility.
- More detailed guidance on setting up GitHub authentication, including helpful screenshots.
- A new section on End-to-End Testing with Playwright, providing clear instructions for setup and execution.
- Overall better structure and formatting throughout the document.
These changes will make it easier for new contributors to set up the project and understand its various components. Great work on improving the documentation!
🧰 Tools
🪛 LanguageTool
[uncategorized] ~174-~174: Loose punctuation mark.
Context: ...et in your.envfile: -E2E_USER_ID: The id of the E2E user for testing. - `...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~175-~175: Loose punctuation mark.
Context: ...E2E user for testing. -E2E_USER_EMAIL: The email of the E2E user for testing. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~176-~176: Loose punctuation mark.
Context: ...ser for testing. -E2E_USER_SESSION_ID: The session id that the user will use t...(UNLIKELY_OPENING_PUNCTUATION)
🪛 Markdownlint
185-185: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
JohnAllenTech
temporarily deployed
to
production
GitHub Actions
Inactive
JohnAllenTech
temporarily deployed
to
production
GitHub Actions
Inactive
✨ Codu Pull Request 💻
Improves #468
Pull Request details
Any Breaking changes
Associated Screenshots