chore: organize test #155
Closed
chore: organize test #155
Conversation
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
Reviewer's GuideThis PR reorganizes the Cypress test suite by centralizing common utilities and configuration, removing repetitive boilerplate, and streamlining key test flows (notably the Share Page tests). Custom commands and support modules replace scattered login, exception handling, and environment-logging code, resulting in a more maintainable and consistent test base. Sequence diagram for centralized login flow using custom commandsequenceDiagram
actor Tester
participant Cypress
participant AuthTestUtils
Tester->>Cypress: cy.loginTestUser(email?)
Cypress->>AuthTestUtils: signInWithTestUrl(email)
AuthTestUtils-->>Cypress: authentication result
Cypress-->>Tester: returns test email
Sequence diagram for exception handling setup in testssequenceDiagram
participant TestFile
participant ExceptionHandlers
TestFile->>ExceptionHandlers: setupCommonExceptionHandlers()
ExceptionHandlers-->>TestFile: sets up uncaught:exception handler
Class diagram for new Cypress support modules and custom commandsclassDiagram
class TestConfig {
+baseUrl: string
+gotrueUrl: string
+apiUrl: string
+logTestEnvironment()
}
class ExceptionHandlers {
+setupCommonExceptionHandlers(additionalPatterns: string[])
+ignoreAllExceptions()
}
class Cypress {
+loginTestUser(email?: string): Chainable<string>
}
TestConfig <.. Cypress : uses
ExceptionHandlers <.. Cypress : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- There are still many fixed-duration cy.wait calls (e.g. in loginTestUser and share tests); consider swapping these for element- or network-based waits (like waiting for specific UI elements or API responses) to make the tests faster and less flaky.
- Several new hard-coded selectors (e.g.
[data-testid="share-button"],.filtercallbacks) appear directly in tests—extract these into your centralized selector files to keep the tests DRY and maintainable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are still many fixed-duration cy.wait calls (e.g. in loginTestUser and share tests); consider swapping these for element- or network-based waits (like waiting for specific UI elements or API responses) to make the tests faster and less flaky.
- Several new hard-coded selectors (e.g. `[data-testid="share-button"]`, `.filter` callbacks) appear directly in tests—extract these into your centralized selector files to keep the tests DRY and maintainable.
## Individual Comments
### Comment 1
<location> `cypress/support/commands.ts:43` </location>
<code_context>
+ * ```
+ */
+Cypress.Commands.add('loginTestUser', (email?: string) => {
+ const { v4: uuidv4 } = require('uuid');
+ const { AuthTestUtils } = require('./auth-utils');
+
</code_context>
<issue_to_address>
**nitpick:** Consider moving require statements to the top of the file for consistency.
Moving require statements to the top enhances readability, maintainability, and compatibility with static analysis tools.
</issue_to_address>
### Comment 2
<location> `cypress/support/exception-handlers.ts:52` </location>
<code_context>
+ *
+ * @param additionalPatterns - Optional array of additional error patterns to ignore
+ */
+export const setupCommonExceptionHandlers = (additionalPatterns: string[] = []) => {
+ const allPatterns = [...IGNORED_ERROR_PATTERNS, ...additionalPatterns];
+
</code_context>
<issue_to_address>
**suggestion:** Consider making error pattern matching case-insensitive for broader coverage.
Matching errors in a case-insensitive way will help ensure all relevant messages are properly ignored, regardless of capitalization.
Suggested implementation:
```typescript
export const setupCommonExceptionHandlers = (additionalPatterns: string[] = []) => {
// Convert all patterns to lowercase for case-insensitive matching
const allPatterns = [...IGNORED_ERROR_PATTERNS, ...additionalPatterns].map(pattern => pattern.toLowerCase());
```
You will also need to update any code that matches error messages against these patterns (e.g., in event handlers or filter functions) to convert the error message to lowercase before checking for a match. For example:
<<<<<<< SEARCH
if (allPatterns.some(pattern => errorMessage.includes(pattern))) {
=======
if (allPatterns.some(pattern => errorMessage.toLowerCase().includes(pattern))) {
>>>>>>> REPLACE
Make sure to apply this change wherever error messages are checked against the patterns.
</issue_to_address>
### Comment 3
<location> `cypress/support/exception-handlers.ts:55` </location>
<code_context>
*/
setupBeforeEach: () => {
- // Suppress known transient errors
- cy.on('uncaught:exception', (err) => {
- if (
- err.message.includes('Minified React error') ||
</code_context>
<issue_to_address>
**suggestion (testing):** Logging to console may not be visible in CI environments; consider using cy.task for logging.
Using cy.task('log', ...) will ensure that these logs appear in CI test output, unlike console.warn.
</issue_to_address>
### Comment 4
<location> `cypress/e2e/page/share-page.cy.ts:25` </location>
<code_context>
userBEmail = generateRandomEmail();
});
it('should invite user B to page via email and then remove their access', () => {
- // Handle uncaught exceptions during workspace creation
- cy.on('uncaught:exception', (err: Error) => {
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the test by extracting repeated flows into custom Cypress commands and replacing manual waits with assertion-based waits for improved readability and maintainability.
Here are a few quick wins to make that single “invite → remove” spec far more readable and maintainable without losing any of the new functionality:
1. Extract your common flows into Cypress commands (login, logout, wait‐for‐app, share/remove, etc.).
2. Replace hard `cy.wait(...)`/`waitForReactUpdate` calls with polite “.should(…)” awaits inside those commands.
3. Collapse the big `it()` down to a linear, high-level story.
### support/commands.ts
```ts
Cypress.Commands.add('login', (email: string) => {
cy.visit('/login', { failOnStatusCode: false });
cy.wait(1000);
new AuthTestUtils().signInWithTestUrl(email);
// wait for shell to load
TestTool.waitForPageLoad();
TestTool.waitForSidebarReady();
});
Cypress.Commands.add('logout', () => {
WorkspaceSelectors.dropdownTrigger().click();
cy.get('[data-testid="logout-button"]').click();
cy.wait(1000);
});
Cypress.Commands.add('createPage', (): Cypress.Chainable<string> => {
PageSelectors.newPageButton().click();
return PageSelectors.names().last()
.should('be.visible')
.invoke('text');
});
Cypress.Commands.add('sharePage', (pageTitle: string, email: string) => {
cy.get(`[data-testid="share-button"]`).click();
cy.get('[data-testid="share-input"]').type(email);
cy.get('[data-testid="permission-dropdown"]').click();
cy.get('[data-testid="permission-can-edit"]').click();
cy.get('[data-testid="share-invite-button"]').click();
return cy.get('[data-testid="share-member-list"]')
.should('contain', email);
});
Cypress.Commands.add('removeShare', (email: string) => {
cy.get(`[data-testid="remove-member-${email}"]`).click();
cy.get('[data-testid="share-member-list"]')
.should('not.contain', email);
});
```
### Updated spec
```ts
it('invites user B then removes their access', () => {
cy.login(userAEmail);
cy.createPage().then(pageTitle => {
cy.sharePage(pageTitle, userBEmail);
cy.logout();
cy.login(userBEmail);
// verify shared page is visible under “Shared with me”
cy.get('[data-testid="shared-with-me-section"]').click();
cy.contains(pageTitle).should('exist');
cy.logout();
cy.login(userAEmail);
cy.contains(pageTitle).click();
cy.removeShare(userBEmail);
});
});
```
This keeps every step but:
- removes deep nesting
- zeroes out magic waits
- makes each line express *intent*, not low-level clicks.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| * ``` | ||
| */ | ||
| Cypress.Commands.add('loginTestUser', (email?: string) => { | ||
| const { v4: uuidv4 } = require('uuid'); |
There was a problem hiding this comment.
nitpick: Consider moving require statements to the top of the file for consistency.
Moving require statements to the top enhances readability, maintainability, and compatibility with static analysis tools.
| * | ||
| * @param additionalPatterns - Optional array of additional error patterns to ignore | ||
| */ | ||
| export const setupCommonExceptionHandlers = (additionalPatterns: string[] = []) => { |
There was a problem hiding this comment.
suggestion: Consider making error pattern matching case-insensitive for broader coverage.
Matching errors in a case-insensitive way will help ensure all relevant messages are properly ignored, regardless of capitalization.
Suggested implementation:
export const setupCommonExceptionHandlers = (additionalPatterns: string[] = []) => {
// Convert all patterns to lowercase for case-insensitive matching
const allPatterns = [...IGNORED_ERROR_PATTERNS, ...additionalPatterns].map(pattern => pattern.toLowerCase());You will also need to update any code that matches error messages against these patterns (e.g., in event handlers or filter functions) to convert the error message to lowercase before checking for a match. For example:
<<<<<<< SEARCH
if (allPatterns.some(pattern => errorMessage.includes(pattern))) {
if (allPatterns.some(pattern => errorMessage.toLowerCase().includes(pattern))) {
REPLACE
Make sure to apply this change wherever error messages are checked against the patterns.
| userBEmail = generateRandomEmail(); | ||
| }); | ||
|
|
||
| it('should invite user B to page via email and then remove their access', () => { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the test by extracting repeated flows into custom Cypress commands and replacing manual waits with assertion-based waits for improved readability and maintainability.
Here are a few quick wins to make that single “invite → remove” spec far more readable and maintainable without losing any of the new functionality:
- Extract your common flows into Cypress commands (login, logout, wait‐for‐app, share/remove, etc.).
- Replace hard
cy.wait(...)/waitForReactUpdatecalls with polite “.should(…)” awaits inside those commands. - Collapse the big
it()down to a linear, high-level story.
support/commands.ts
Cypress.Commands.add('login', (email: string) => {
cy.visit('/login', { failOnStatusCode: false });
cy.wait(1000);
new AuthTestUtils().signInWithTestUrl(email);
// wait for shell to load
TestTool.waitForPageLoad();
TestTool.waitForSidebarReady();
});
Cypress.Commands.add('logout', () => {
WorkspaceSelectors.dropdownTrigger().click();
cy.get('[data-testid="logout-button"]').click();
cy.wait(1000);
});
Cypress.Commands.add('createPage', (): Cypress.Chainable<string> => {
PageSelectors.newPageButton().click();
return PageSelectors.names().last()
.should('be.visible')
.invoke('text');
});
Cypress.Commands.add('sharePage', (pageTitle: string, email: string) => {
cy.get(`[data-testid="share-button"]`).click();
cy.get('[data-testid="share-input"]').type(email);
cy.get('[data-testid="permission-dropdown"]').click();
cy.get('[data-testid="permission-can-edit"]').click();
cy.get('[data-testid="share-invite-button"]').click();
return cy.get('[data-testid="share-member-list"]')
.should('contain', email);
});
Cypress.Commands.add('removeShare', (email: string) => {
cy.get(`[data-testid="remove-member-${email}"]`).click();
cy.get('[data-testid="share-member-list"]')
.should('not.contain', email);
});Updated spec
it('invites user B then removes their access', () => {
cy.login(userAEmail);
cy.createPage().then(pageTitle => {
cy.sharePage(pageTitle, userBEmail);
cy.logout();
cy.login(userBEmail);
// verify shared page is visible under “Shared with me”
cy.get('[data-testid="shared-with-me-section"]').click();
cy.contains(pageTitle).should('exist');
cy.logout();
cy.login(userAEmail);
cy.contains(pageTitle).click();
cy.removeShare(userBEmail);
});
});This keeps every step but:
- removes deep nesting
- zeroes out magic waits
- makes each line express intent, not low-level clicks.
appflowy
force-pushed
the
organize_e2e_test
branch
from
758d302 to
7786384
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Checklist
General
Testing
Feature-Specific
Summary by Sourcery
Organize and streamline the Cypress E2E test suite by centralizing test configuration, error handling, and authentication. Introduce shared support modules (loginTestUser command, exception-handlers, TestConfig) and refactor existing tests to use these utilities, eliminating duplication and improving maintainability.
New Features:
Enhancements: