Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:13469 This environment will automatically shut down when the PR is closed or after 5 hours. |
|
I'm concerned that aria-labels won't be maintained, much like comments, and that it risks bloating our codebase. We could consider React locators : https://playwright.dev/docs/other-locators#react-locator |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📊 API Changes ReportGraphQL Schema ChangesGraphQL Schema Changes[error] Error: Unable to read JSON file: /home/runner/work/twenty/twenty/main-schema-introspection.json: Not valid JSON content GraphQL Metadata Schema ChangesGraphQL Metadata Schema Changes[error] Error: Unable to read JSON file: /home/runner/work/twenty/twenty/main-metadata-schema-introspection.json: Not valid JSON content REST API Analysis ErrorError OutputREST Metadata API Analysis ErrorError Output
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 28 files
Prompt for AI agents (all 8 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-e2e-testing/tests/workflow-visualizer.spec.ts">
<violation number="1" location="packages/twenty-e2e-testing/tests/workflow-visualizer.spec.ts:1">
P2: Consider using Playwright's `test.skip()` or `test.fixme()` instead of commenting out entire tests. This keeps the code type-checked, shows skipped tests in reports, and makes it easier to track and re-enable them.
Example:
```ts
test.skip('Create workflow with every possible step', async ({ workflowVisualizer, page }) => {
// test code remains parseable
});
Or with a reason:
test.fixme('Create workflow with every possible step', async ({ workflowVisualizer, page }) => {
// ...
});
```</violation>
</file>
<file name="packages/twenty-e2e-testing/tests/create-kanban-view.spec.ts">
<violation number="1" location="packages/twenty-e2e-testing/tests/create-kanban-view.spec.ts:52">
P2: Race condition: In `Promise.all`, the click action is listed before `waitForRequest`, which may cause the request to be missed if it fires before the listener is set up. Following the pattern in `workflow-creation.spec.ts`, the wait should come first. Also consider using `waitForResponse` instead of `waitForRequest` for consistency with existing tests.</violation>
</file>
<file name="packages/twenty-e2e-testing/tests/create-record.spec.ts">
<violation number="1" location="packages/twenty-e2e-testing/tests/create-record.spec.ts:96">
P1: Missing `await` on Playwright click action. This can cause race conditions and flaky tests since the test won't wait for the click to complete.</violation>
<violation number="2" location="packages/twenty-e2e-testing/tests/create-record.spec.ts:112">
P1: Missing `await` on Playwright click action. This can cause race conditions and flaky tests since the test won't wait for the click to complete.</violation>
</file>
<file name="packages/twenty-e2e-testing/tests/workflow-creation.spec.ts">
<violation number="1" location="packages/twenty-e2e-testing/tests/workflow-creation.spec.ts:12">
P1: `page.goto(process.env.LINK)` will fail if `LINK` environment variable is not set, as it will pass `undefined`. The original `page.goto('/')` worked with the default baseURL. Consider providing a fallback.</violation>
</file>
<file name="packages/twenty-front/src/modules/object-record/record-group/components/RecordGroupMenuItemDraggable.tsx">
<violation number="1" location="packages/twenty-front/src/modules/object-record/record-group/components/RecordGroupMenuItemDraggable.tsx:40">
P2: `recordGroup.value` can be `null` (per `RecordGroupDefinition` type), resulting in aria labels like "Hide group null". Consider using `recordGroup.title` instead, which is always a string and is already used for the Tag display text.</violation>
</file>
<file name="packages/twenty-e2e-testing/lib/utils/getAccessAuthToken.ts">
<violation number="1" location="packages/twenty-e2e-testing/lib/utils/getAccessAuthToken.ts:16">
P1: Bug: `Array.filter()` always returns an array (possibly empty), never `null` or `undefined`. This check will never throw even when no matching cookies exist. Should check `tokenCookies.length === 0` instead.</violation>
<violation number="2" location="packages/twenty-e2e-testing/lib/utils/getAccessAuthToken.ts:26">
P1: Bug: If no access token cookie is found, `JSON.parse('')` will throw a `SyntaxError`. Add a check for `accessTokenCookie` before parsing its value.</violation>
</file>Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| } | ||
| await page.getByText('Options').click(); | ||
| await page.getByText('Group', { exact: true }).click(); | ||
| await Promise.all([page.getByRole('button', { name: 'Hide group null', exact: true }).click(), |
There was a problem hiding this comment.
P2: Race condition: In Promise.all, the click action is listed before waitForRequest, which may cause the request to be missed if it fires before the listener is set up. Following the pattern in workflow-creation.spec.ts, the wait should come first. Also consider using waitForResponse instead of waitForRequest for consistency with existing tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-e2e-testing/tests/create-kanban-view.spec.ts, line 52:
<comment>Race condition: In `Promise.all`, the click action is listed before `waitForRequest`, which may cause the request to be missed if it fires before the listener is set up. Following the pattern in `workflow-creation.spec.ts`, the wait should come first. Also consider using `waitForResponse` instead of `waitForRequest` for consistency with existing tests.</comment>
<file context>
@@ -0,0 +1,59 @@
+ }
+ await page.getByText('Options').click();
+ await page.getByText('Group', { exact: true }).click();
+ await Promise.all([page.getByRole('button', { name: 'Hide group null', exact: true }).click(),
+ page.waitForRequest((req) => {
+ return req.url().includes('/metadata') &&
</file context>
| await recordFieldList.getByText('Work Preference').nth(1).click({force: true}); | ||
| const options = page.getByRole('listbox'); | ||
| await options.getByText('Hybrid').first().click({force: true}); | ||
| recordFieldList.getByText('Work Preference').first().click({force: true}); |
There was a problem hiding this comment.
P1: Missing await on Playwright click action. This can cause race conditions and flaky tests since the test won't wait for the click to complete.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-e2e-testing/tests/create-record.spec.ts, line 112:
<comment>Missing `await` on Playwright click action. This can cause race conditions and flaky tests since the test won't wait for the click to complete.</comment>
<file context>
@@ -0,0 +1,163 @@
+ await recordFieldList.getByText('Work Preference').nth(1).click({force: true});
+ const options = page.getByRole('listbox');
+ await options.getByText('Hybrid').first().click({force: true});
+ recordFieldList.getByText('Work Preference').first().click({force: true});
+
+ // Fill company relation
</file context>
| await page.getByPlaceholder('URL').press('Enter'); | ||
|
|
||
| // Click on 4th star to rate | ||
| recordFieldList.getByText('Performance Rating').first().click({ force: true }); |
There was a problem hiding this comment.
P1: Missing await on Playwright click action. This can cause race conditions and flaky tests since the test won't wait for the click to complete.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-e2e-testing/tests/create-record.spec.ts, line 96:
<comment>Missing `await` on Playwright click action. This can cause race conditions and flaky tests since the test won't wait for the click to complete.</comment>
<file context>
@@ -0,0 +1,163 @@
+ await page.getByPlaceholder('URL').press('Enter');
+
+ // Click on 4th star to rate
+ recordFieldList.getByText('Performance Rating').first().click({ force: true });
+ const ratingContainer = recordFieldList.locator('div[aria-label="Rating"]');
+ await ratingContainer.locator('svg').nth(3).click({force: true});
</file context>
| const NEW_WORKFLOW_NAME = 'Test Workflow'; | ||
|
|
||
| await page.goto('/'); | ||
| await page.goto(process.env.LINK); |
There was a problem hiding this comment.
P1: page.goto(process.env.LINK) will fail if LINK environment variable is not set, as it will pass undefined. The original page.goto('/') worked with the default baseURL. Consider providing a fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-e2e-testing/tests/workflow-creation.spec.ts, line 12:
<comment>`page.goto(process.env.LINK)` will fail if `LINK` environment variable is not set, as it will pass `undefined`. The original `page.goto('/')` worked with the default baseURL. Consider providing a fallback.</comment>
<file context>
@@ -1,11 +1,15 @@
const NEW_WORKFLOW_NAME = 'Test Workflow';
- await page.goto('/');
+ await page.goto(process.env.LINK);
const workflowsLink = page.getByRole('link', { name: 'Workflows' });
</file context>
| await page.goto(process.env.LINK); | |
| await page.goto(process.env.LINK ?? '/'); |
✅ Addressed in 96913d8
| ? `Hide group ${recordGroup.value}` | ||
| : `Show group ${recordGroup.value}`, |
There was a problem hiding this comment.
P2: recordGroup.value can be null (per RecordGroupDefinition type), resulting in aria labels like "Hide group null". Consider using recordGroup.title instead, which is always a string and is already used for the Tag display text.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-front/src/modules/object-record/record-group/components/RecordGroupMenuItemDraggable.tsx, line 40:
<comment>`recordGroup.value` can be `null` (per `RecordGroupDefinition` type), resulting in aria labels like "Hide group null". Consider using `recordGroup.title` instead, which is always a string and is already used for the Tag display text.</comment>
<file context>
@@ -36,6 +36,9 @@ export const RecordGroupMenuItemDraggable = ({
{
Icon: recordGroup.isVisible ? IconEyeOff : IconEye,
+ ariaLabel: recordGroup.isVisible
+ ? `Hide group ${recordGroup.value}`
+ : `Show group ${recordGroup.value}`,
onClick: () =>
</file context>
| ? `Hide group ${recordGroup.value}` | |
| : `Show group ${recordGroup.value}`, | |
| ? `Hide group ${recordGroup.title}` | |
| : `Show group ${recordGroup.title}`, |
| } | ||
| ); | ||
|
|
||
| const token = JSON.parse(decodeURIComponent(accessTokenCookie?.value ?? '')).accessOrWorkspaceAgnosticToken |
There was a problem hiding this comment.
P1: Bug: If no access token cookie is found, JSON.parse('') will throw a SyntaxError. Add a check for accessTokenCookie before parsing its value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-e2e-testing/lib/utils/getAccessAuthToken.ts, line 26:
<comment>Bug: If no access token cookie is found, `JSON.parse('')` will throw a `SyntaxError`. Add a check for `accessTokenCookie` before parsing its value.</comment>
<file context>
@@ -0,0 +1,30 @@
+ }
+ );
+
+ const token = JSON.parse(decodeURIComponent(accessTokenCookie?.value ?? '')).accessOrWorkspaceAgnosticToken
+ .token;
+
</file context>
| const tokenCookies = storageState.cookies.filter( | ||
| (cookie) => cookie.name === 'tokenPair', | ||
| ); | ||
| if (!tokenCookies) { |
There was a problem hiding this comment.
P1: Bug: Array.filter() always returns an array (possibly empty), never null or undefined. This check will never throw even when no matching cookies exist. Should check tokenCookies.length === 0 instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-e2e-testing/lib/utils/getAccessAuthToken.ts, line 16:
<comment>Bug: `Array.filter()` always returns an array (possibly empty), never `null` or `undefined`. This check will never throw even when no matching cookies exist. Should check `tokenCookies.length === 0` instead.</comment>
<file context>
@@ -0,0 +1,30 @@
+ const tokenCookies = storageState.cookies.filter(
+ (cookie) => cookie.name === 'tokenPair',
+ );
+ if (!tokenCookies) {
+ throw new Error('No auth cookie found');
+ }
</file context>
I agree that we will likely never update the accessibility labels unless accessibility becomes a team concern. However, if we use React locators, the E2E tests will be dependent on implementation details. We should think about the best compromise. |
Devessier
left a comment
Devessier
left a comment
There was a problem hiding this comment.
Thank you for working on bringing back meaningful end-to-end tests!
One main concern: I think we should use data-testid instead of aria-label.
If possible, it would be great to remove the use of process.env.LINK.
packages/twenty-e2e-testing/tests/authentication/signup_invite_email.spec.ts
Outdated
Show resolved
Hide resolved
| page.waitForRequest((req) => { | ||
| return req.url().includes('/metadata') && | ||
| req.method() === 'POST' | ||
| })]); |
There was a problem hiding this comment.
The problem with this kind of assertion is that we can make dozens of calls to /metadata in a single navigation. This assertion proves nothing.
We can leave it as is, but since it doesn’t prove that the action we expect was performed, it could lead to flakiness.
There was a problem hiding this comment.
I don't think we expect other POST requests in this setup, nor that for now we actually do any /metadata post requests while navigating only
There was a problem hiding this comment.
But I intent to make sure this is not flaky so i m keeping that in mind
...ages/twenty-front/src/modules/object-record/record-field-list/components/RecordFieldList.tsx
Outdated
Show resolved
Hide resolved
...t-record/record-field-list/record-detail-section/components/RecordDetailSectionContainer.tsx
Outdated
Show resolved
Hide resolved
| }} | ||
| > | ||
| <RecordDetailSectionContainer | ||
| ariaLabel={`${fieldDefinition.label} (relation)`} |
There was a problem hiding this comment.
I think this should be data-testid instead.
| ? `Hide group ${recordGroup.value}` | ||
| : `Show group ${recordGroup.value}`, |
There was a problem hiding this comment.
That's a proper use of aria-label 👍
...y-front/src/modules/object-record/record-inline-cell/property-box/components/PropertyBox.tsx
Outdated
Show resolved
Hide resolved
| // Generate a random email for testing | ||
| const randomEmail = `testuser_${Math.random().toString(36).substring(2, 10)}@example.com`; | ||
| // Fill first name and last name | ||
| const firstNameInput = page.getByRole('textbox', { name: 'First name' }) |
| await page.getByRole('option', { name: 'Google' }).click({force: true}) | ||
| ]); | ||
|
|
||
| const body = await updatePersonResponse.json() |
In this PR, - current basic E2E tests are fixed, and some were added, covering some basic scenarios - some tests avec been commented out, until we decide whether they are worth fixing The next steps are - evaluate the flakiness of the tests. Once they've proved not to be flaky, we should add more tests + re-write the current ones not using aria-label (cf @lucasbordeau indication). - We will add them back to the development flow
4 participants
In this PR,
The next steps are