Test fixture/POM cleanup#47
Merged
Merged
Conversation
Added more compatibility questions
Added compatibility question tests and verifications Updated tests to cover Keywords and Headline changes recently made Updated tests to cover all of the big5 personality traits
Updated signUp.spec.ts to use new fixture Updated Account information variable names Deleted "deleteUserFixture.ts" as it was incorporated into the "base.ts" file
Updated seedDatabase.ts to throw an error if the user already exists, to also add display names and usernames so they seedUser func acts like a normal basic user Some organising of the google auth code
Added account deletion checks
Updated settings POM
|
@O-Bots is attempting to deploy a commit to the Compass Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe PR consolidates e2e test infrastructure by introducing a unified Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/web/utils/deleteUser.ts (1)
35-38:⚠️ Potential issue | 🟡 MinorReplace
console.logwithdebug()fromcommon/logger.Per coding guidelines,
console.logshould not be used. Usedebug()instead for development logging.Proposed fix
+import {debug} from 'common/logger' + // ... in the catch block: - console.log(`Email not found, skipping user deletion for ${account?.email}`) + debug(`Email not found, skipping user deletion for ${account?.email}`) return } - console.log(err) + debug(err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/utils/deleteUser.ts` around lines 35 - 38, Replace the two console.log calls in the deleteUser helper with the project's debug logger: import and use debug from common/logger and call debug(...) instead of console.log(...), including the message that references account?.email and the err object so both the "Email not found, skipping user deletion for ${account?.email}" message and the error output use debug() rather than console.log.
🧹 Nitpick comments (4)
.vscode/settings.json (1)
2-2: Please avoid committing personal editor preferences in this PR.This workspace setting is unrelated to the POM/fixture refactor and may override contributor IDE behavior. Prefer keeping it in local (untracked) workspace settings, or document it separately if this is a team-wide standard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vscode/settings.json at line 2, Remove the personal workspace editor setting "codium.codeCompletion.enable": false from the committed settings file; this is a user-specific preference that should not be in the repository. Revert or delete the entry in the settings JSON (the "codium.codeCompletion.enable" key) and either add the file to .gitignore or move this preference to an untracked local workspace/settings file or to documented team-wide config if it should be shared.tests/e2e/web/pages/app.ts (3)
39-39: Avoid hardcoded survey text in App workflows.
'Delete me'should be sourced from shared constants/test data to keep flows maintainable.As per coding guidelines, “Flag any hardcoded strings; they should be in the constants file.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/pages/app.ts` at line 39, The test uses a hardcoded survey string in the App workflow—replace the literal 'Delete me' passed to this.settings.fillDeleteAccountSurvey with a shared constant from the central test data/constants module (e.g., DELETE_ACCOUNT_SURVEY_TEXT or testData.deleteAccountSurveyText); import that constant into the module and use it in the call to fillDeleteAccountSurvey so the value is maintained centrally and not hardcoded in the test.
36-77: Add docstrings for public App workflow methods.These methods are shared entry points for multiple specs and should be documented.
As per coding guidelines, “Ensure all public functions have docstrings.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/pages/app.ts` around lines 36 - 77, Add JSDoc-style docstrings to each public App workflow method (deleteProfileFromSettings, skipOnboardingHeadToProfile, registerWithEmail, signinWithEmail) describing what the method does, its parameters (e.g., UserAccountInformation fields like email, password, username, display_name) and any return/side-effects (navigations, clicks, throws). Place the docstring immediately above each method, include parameter types and a short usage note for signinWithEmail (accepts either an account object or email+password), and keep wording concise to satisfy the “all public functions have docstrings” guideline.
13-13: Renameapp.tstoApp.tsto match the repo’s POM naming rule.Class naming is correct, but file naming currently deviates from the Playwright guideline.
As per coding guidelines, “File and class names must use PascalCase (e.g.,
CompatibilityPage.ts/class CompatibilityPage).”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/pages/app.ts` at line 13, Rename the file from app.ts to App.ts so it follows the repo POM naming rule (PascalCase) and keep the exported class App unchanged; update any imports that reference "./app" or "app" to "./App" (or "App") to match the new filename, ensuring imports in tests/pages (or any consumers) reference the PascalCase filename.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/web/specs/onboardingFlow.spec.ts`:
- Line 223: Remove the brittle workaround call to app.auth.fillPasswordField('')
and instead fix the Google auth readiness check in the auth-page flow (e.g., the
AuthPage or app.auth readiness/wait method) so the test waits deterministically
for the actual interactive Google sign-in UI; replace any implicit sleeps with
Playwright auto-waiters such as waiting for the Google iframe or its
email/password input locators to be attached/visible/enabled (use
frame.locator(...).waitFor or expect(locator).toBeVisible()/toBeEnabled()) and
ensure the readiness method returns only after those locators are ready so tests
no longer require the empty-password hack.
In `@tests/e2e/web/specs/signIn.spec.ts`:
- Line 42: The test currently calls await app.deleteProfileFromSettings()
directly in the test body (and similarly at the other location), which can leak
accounts if the test fails; remove these direct calls from signIn.spec.ts and
instead perform the cleanup in the fixture teardown where the app fixture is
defined by adding the deletion after the await use(...) boundary (e.g., in the
fixture that provides the app/page, call await app.deleteProfileFromSettings()
in the teardown section); this ensures cleanup always runs even if the test
fails and avoids placing destructive cleanup inside test bodies or afterEach
hooks.
In `@tests/e2e/web/specs/signUp.spec.ts`:
- Around line 12-14: Replace the direct console.log calls in the signUp.spec.ts
tests with the project logger (use debug() or log()) — specifically change the
console.log that prints the test start message using specAccount.username in the
"should disable the button \"Next\" when the display name field is empty" test
and the other console.log instances at the nearby block (lines ~27-29) to use
debug() or log(), preserving the same string interpolation/contents; ensure you
import or reference the project's logger instance used across tests and remove
any remaining console.log usages.
In `@tests/e2e/web/utils/deleteUser.ts`:
- Around line 16-22: The catch block is swallowing errors from deleteAccount and
conflating deletion with DB lookup; split the concerns by wrapping
deleteAccount(loginInfo?.data.idToken) in its own try/catch and surface or log
any error rather than ignoring it, then separately call
userInformationFromDb(account) and handle its thrown "not found" case explicitly
(don't rely on a falsy return); only call deleteFromDb(loginInfo?.data.localId)
when userInformationFromDb confirms the user exists, and catch/handle errors
from deleteFromDb independently so failures are not silently swallowed (refer to
functions deleteAccount, userInformationFromDb, deleteFromDb and variables
loginInfo and account).
---
Outside diff comments:
In `@tests/e2e/web/utils/deleteUser.ts`:
- Around line 35-38: Replace the two console.log calls in the deleteUser helper
with the project's debug logger: import and use debug from common/logger and
call debug(...) instead of console.log(...), including the message that
references account?.email and the err object so both the "Email not found,
skipping user deletion for ${account?.email}" message and the error output use
debug() rather than console.log.
---
Nitpick comments:
In @.vscode/settings.json:
- Line 2: Remove the personal workspace editor setting
"codium.codeCompletion.enable": false from the committed settings file; this is
a user-specific preference that should not be in the repository. Revert or
delete the entry in the settings JSON (the "codium.codeCompletion.enable" key)
and either add the file to .gitignore or move this preference to an untracked
local workspace/settings file or to documented team-wide config if it should be
shared.
In `@tests/e2e/web/pages/app.ts`:
- Line 39: The test uses a hardcoded survey string in the App workflow—replace
the literal 'Delete me' passed to this.settings.fillDeleteAccountSurvey with a
shared constant from the central test data/constants module (e.g.,
DELETE_ACCOUNT_SURVEY_TEXT or testData.deleteAccountSurveyText); import that
constant into the module and use it in the call to fillDeleteAccountSurvey so
the value is maintained centrally and not hardcoded in the test.
- Around line 36-77: Add JSDoc-style docstrings to each public App workflow
method (deleteProfileFromSettings, skipOnboardingHeadToProfile,
registerWithEmail, signinWithEmail) describing what the method does, its
parameters (e.g., UserAccountInformation fields like email, password, username,
display_name) and any return/side-effects (navigations, clicks, throws). Place
the docstring immediately above each method, include parameter types and a short
usage note for signinWithEmail (accepts either an account object or
email+password), and keep wording concise to satisfy the “all public functions
have docstrings” guideline.
- Line 13: Rename the file from app.ts to App.ts so it follows the repo POM
naming rule (PascalCase) and keep the exported class App unchanged; update any
imports that reference "./app" or "app" to "./App" (or "App") to match the new
filename, ensuring imports in tests/pages (or any consumers) reference the
PascalCase filename.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 52f42d5a-2185-4632-b755-625c58be2f50
📒 Files selected for processing (14)
.vscode/settings.jsonbackend/api/tests/unit/get-profiles.unit.test.tsplaywright.config.tstests/e2e/tsconfig.jsontests/e2e/utils/firebaseUtils.tstests/e2e/web/fixtures/base.tstests/e2e/web/fixtures/signInFixture.tstests/e2e/web/pages/app.tstests/e2e/web/pages/authPage.tstests/e2e/web/specs/onboardingFlow.spec.tstests/e2e/web/specs/signIn.spec.tstests/e2e/web/specs/signUp.spec.tstests/e2e/web/utils/deleteUser.tstests/e2e/web/utils/testCleanupHelpers.ts
💤 Files with no reviewable changes (1)
- tests/e2e/web/utils/testCleanupHelpers.ts
Co-authored-by: Martin Braquet <martin.braquet@gmail.com>
Collaborator
Author
|
Making the changes pr otw |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/e2e/web/specs/onboardingFlow.spec.ts (2)
4-8: Spec file naming convention.Based on learnings, E2E tests should use
*.e2e.spec.tsfilename pattern. Consider renaming toonboardingFlow.e2e.spec.tsfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/specs/onboardingFlow.spec.ts` around lines 4 - 8, The spec filename onboardingFlow.spec.ts does not follow the project's E2E test naming convention; rename the file to onboardingFlow.e2e.spec.ts and update any test runner or import references so the test.describe('when given valid input') / test('should successfully complete the onboarding flow with email', ...) remains unchanged but is discovered by the E2E suite.
134-134: Replaceconsole.logwithdebug()fromcommon/logger.This
console.logstatement violates coding guidelines. Usedebug()for development logging.♻️ Proposed fix
+import {debug} from 'common/logger' // ... at line 134: - console.log(dbInfo.profile) + debug('dbInfo.profile:', dbInfo.profile)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/specs/onboardingFlow.spec.ts` at line 134, Replace the console.log(dbInfo.profile) call in the onboardingFlow.spec.ts test with the debug logger from common/logger: import the debug function (named export) at the top of the file and call debug(...) instead of console.log, passing the same value (dbInfo.profile) or a short contextual message plus the value to preserve the original output; ensure you remove the console.log usage and use debug(...) consistently in this test.tests/e2e/web/pages/app.ts (1)
1-1: File should be namedApp.tsper coding guidelines.The E2E test guidelines specify "File and class names must use PascalCase (e.g.,
CompatibilityPage.ts/class CompatibilityPage)". This file should be renamed fromapp.tstoApp.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/pages/app.ts` at line 1, Rename the file from app.ts to App.ts and ensure the exported class uses PascalCase (e.g., rename class app / default export to App or AppPage) so filename and class follow the PascalCase guideline; update any imports that reference "app" to reference "App" (and adjust the class name usage) to keep names consistent across the codebase.tests/e2e/web/fixtures/base.ts (1)
18-18: Replaceconsole.logwithdebug()fromcommon/logger.Multiple
console.logstatements violate the coding guidelines. Per guidelines: "Do not use console.log; usedebug()orlog()instead" and "Usedebug()fromcommon/loggerinstead of console.log for logging".♻️ Proposed fix
Add the import at the top of the file:
import {debug} from 'common/logger'Then replace each
console.logwithdebug:- console.log('Cleaning up onboarding 1 account...') + debug('Cleaning up onboarding 1 account...')Apply similarly to lines 24, 31, 38, and 44.
Also applies to: 24-25, 31-32, 38-39, 44-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/fixtures/base.ts` at line 18, Replace all console.log calls in this fixture with the project's debug logger: add an import for the debug function from common/logger at the top of the file and change each console.log('Cleaning up onboarding 1 account...') (and the other console.log occurrences on the same fixture around the following cleanup messages) to use debug(...) instead; ensure you import the symbol debug and call debug(...) in the same places where console.log(...) was used (apply the same change for the other listed occurrences).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/web/fixtures/base.ts`:
- Line 18: Replace all console.log calls in this fixture with the project's
debug logger: add an import for the debug function from common/logger at the top
of the file and change each console.log('Cleaning up onboarding 1 account...')
(and the other console.log occurrences on the same fixture around the following
cleanup messages) to use debug(...) instead; ensure you import the symbol debug
and call debug(...) in the same places where console.log(...) was used (apply
the same change for the other listed occurrences).
In `@tests/e2e/web/pages/app.ts`:
- Line 1: Rename the file from app.ts to App.ts and ensure the exported class
uses PascalCase (e.g., rename class app / default export to App or AppPage) so
filename and class follow the PascalCase guideline; update any imports that
reference "app" to reference "App" (and adjust the class name usage) to keep
names consistent across the codebase.
In `@tests/e2e/web/specs/onboardingFlow.spec.ts`:
- Around line 4-8: The spec filename onboardingFlow.spec.ts does not follow the
project's E2E test naming convention; rename the file to
onboardingFlow.e2e.spec.ts and update any test runner or import references so
the test.describe('when given valid input') / test('should successfully complete
the onboarding flow with email', ...) remains unchanged but is discovered by the
E2E suite.
- Line 134: Replace the console.log(dbInfo.profile) call in the
onboardingFlow.spec.ts test with the debug logger from common/logger: import the
debug function (named export) at the top of the file and call debug(...) instead
of console.log, passing the same value (dbInfo.profile) or a short contextual
message plus the value to preserve the original output; ensure you remove the
console.log usage and use debug(...) consistently in this test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 73931d92-4fc7-4e2f-9434-b447e83d60cf
📒 Files selected for processing (9)
.coderabbit.yamldocs/TESTING.mdtests/e2e/utils/firebaseUtils.tstests/e2e/web/fixtures/base.tstests/e2e/web/fixtures/signInFixture.tstests/e2e/web/pages/app.tstests/e2e/web/specs/onboardingFlow.spec.tstests/e2e/web/specs/signIn.spec.tstests/e2e/web/specs/signUp.spec.ts
💤 Files with no reviewable changes (2)
- tests/e2e/utils/firebaseUtils.ts
- docs/TESTING.md
✅ Files skipped from review due to trivial changes (1)
- .coderabbit.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/e2e/web/fixtures/signInFixture.ts
- tests/e2e/web/specs/signUp.spec.ts
- tests/e2e/web/specs/signIn.spec.ts
MartinBraquet
approved these changes
Apr 4, 2026
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Member
|
@coderabbitai give me the commit name and update the PR title |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1060 🏁 Script executed: Length of output: 84 🏁 Script executed: Length of output: 252
Unfortunately, updating the PR title is blocked due to workflow restriction settings in this repository. You can update it manually by editing the PR title to the above. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #45
Summary by CodeRabbit
Tests
Chores