Workflow e2e tests – 1st batch #9713

Devessier · 2025-01-17T11:27:40Z

Clean Playwright's configuration:
- Remove artificial 500ms delay between each step
- Group all tests under a chrome project relying on a setup project to get an authentication state which all tests can reuse
Changes on the Sign up with invite link via email test:
- Generate a new email for each test trial, as previously it was failing when run many times
- Make deleting the account part of the test; if we write other tests for account sign-up, we'll prefer to delete the accounts with an HTTP call to speed up things
Added some assertions to ensure we reached steps when expected, as we removed the 500ms delay between each step, and it made some assertions fail
Wrote new tests for workflows:
- Created Create workflow, a test asserting we can create a workflow from the record table
- Created Create simple workflow, a test asserting we can create a simple flow; I will add more assertions to this test and write other tests once this first PR is approved
- I make HTTP calls to delete and destroy workflows after they run to keep the database clean
- Added a data-testid to ensure we focus elements from the Cmd+K; our selectors are not strong – see getByRole('textbox') – and I preferred to scope them to a root element
- Added an aria-label to a button

…f a first test

Deleting an account doesn't clear the user from the database, it just removes the user from the workspace. The test used to fail when ran several times.

github-actions · 2025-01-17T11:28:34Z

	Warnings
⚠️	Changes were made to the environment variables, but not to the documentation - Please review your changes and check if a change needs to be documented!

Generated by 🚫 dangerJS against 4cdc8fd

greptile-apps

PR Summary

This PR introduces end-to-end testing for workflow functionality while streamlining the testing infrastructure. The changes focus on workflow creation, visualization, and cleanup operations.

Added new workflow E2E test suite with blank-workflow.ts fixture and corresponding test files for workflow creation and visualization
Removed redundant utility files (customreporter.ts, env_variables.ts, shell_driver.ts) in favor of built-in Playwright functionality
Added GraphQL request handlers in /lib/requests/ for workflow CRUD operations with proper authentication
Added data-testid attributes to frontend components to support E2E test selectors
Simplified environment setup by automating .env file creation from example template

_{21 file(s) reviewed, 24 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-01-17T11:28:50Z

packages/twenty-e2e-testing/.env.example

 # Note that provide always without trailing forward slash to have expected behaviour
 FRONTEND_BASE_URL=http://localhost:3001


logic: the comment about trailing forward slash is now inconsistent with line 9 which requires a trailing forward slash for REST API

greptile-apps · 2025-01-17T11:28:51Z

packages/twenty-e2e-testing/drivers/env_variables.ts

logic: This file was deleted without providing an alternative mechanism for setting up test environment variables. Ensure there is a new implementation for managing test environment configuration.

greptile-apps · 2025-01-17T11:28:51Z

packages/twenty-e2e-testing/drivers/shell_driver.ts

logic: Removing the shell command execution capability could break existing tests or utilities that depend on it. Need confirmation that this functionality is either no longer needed or has been moved elsewhere.

greptile-apps · 2025-01-17T11:29:35Z

packages/twenty-e2e-testing/lib/fixtures/blank-workflow.ts

+    const response = await createWorkflow({
+      page: this.#page,
+      workflowId: id,
+      workflowName: this.workflowName,
+    });


style: no try/catch block around API call could cause test failures to be unclear if API is down

greptile-apps · 2025-01-17T11:29:36Z

packages/twenty-e2e-testing/lib/fixtures/blank-workflow.ts

+      await deleteWorkflow({
+        page,
+        workflowId: workflowVisualizer.workflowId,
+      });
+      await destroyWorkflow({
+        page,
+        workflowId: workflowVisualizer.workflowId,
+      });


style: cleanup operations should be in try/finally to ensure they run even if test fails

greptile-apps · 2025-01-17T11:35:28Z

packages/twenty-e2e-testing/tests/workflow-visualizer.spec.ts

+import { expect } from '@playwright/test';
+import { test } from '../lib/fixtures/blank-workflow';
+
+test('Create simple workflow', async ({ workflowVisualizer, page }) => {


logic: workflowVisualizer fixture is imported but never used in the test

greptile-apps · 2025-01-17T11:35:29Z

packages/twenty-e2e-testing/tests/workflow-visualizer.spec.ts

+  await expect(
+    page.getByTestId('command-menu').getByRole('textbox'),
+  ).toHaveValue('When a Company is Created');


style: add a timeout to this expectation since command menu may take time to populate

greptile-apps · 2025-01-17T11:35:29Z

packages/twenty-e2e-testing/tests/workflow-visualizer.spec.ts

+  await expect(triggerNode).toHaveClass(/selected/);
+  await expect(triggerNode).toHaveText(/Company is Created/);


style: use exact text matching with triggerNode.toHaveText() to ensure precise validation

greptile-apps · 2025-01-17T11:35:30Z

packages/twenty-e2e-testing/tests/workflow-visualizer.spec.ts

+  const createRecordNode = page
+    .locator('.react-flow__node.selected')
+    .getByText('Create Record');


style: locating by CSS class '.react-flow__node.selected' is fragile - consider using a data-testid instead

greptile-apps · 2025-01-17T11:35:55Z

...nty-front/src/modules/workflow/workflow-diagram/components/WorkflowDiagramCreateStepNode.tsx

@@ -15,7 +15,7 @@ export const WorkflowDiagramCreateStepNode = () => {
    <StyledContainer>
      <StyledTargetHandle type="target" position={Position.Top} />

-      <IconButton Icon={IconPlus} size="medium" />
+      <IconButton Icon={IconPlus} size="medium" ariaLabel="Add a step" />


syntax: The prop should be 'aria-label' instead of 'ariaLabel' to follow standard HTML accessibility attribute naming

thomtrp

Really nice! Scenario LGTM. Can you check with someone who knows end to end tests setup?

prastoin

That's very good !
Interesting scenarios and tests cleanups

It seems like there're two directions of describing tests
We should take a decision regarding the Page Object Model abstraction degree of integration we wanna reach to

In my best of the world, tests scenario reports should be readable by anyone, not necessary a tech. I mean any product team mate that has access to a playwright report should be able to interpret the result of a scenario failure, in order to produce a exhaustive backlog/issue.

That's the first time I'm reviewing the twenty-e2e-testing package, I'm not a Playwright expert at all, below can be found a list of things I think we could try to improve regarding the package as whole and not necessary this PR diff:

Typing and validating process.env
Typecheck twenty-e2e-testing package in the ci
setup webDevServer in order to simplify and centralize building logic locally and in the ci
Define a file tree architecture, regarding fixture localization and so on and use absolute imports
Add .nx caching in e2e ci
Publish and version playwright reports, e.g on a static website

prastoin · 2025-01-17T13:02:21Z

packages/twenty-e2e-testing/lib/utils/getAuthToken.ts

+  if (!authCookie) {
+    throw new Error('No auth cookie found');
+  }
+  const token = JSON.parse(decodeURIComponent(authCookie.value)).accessToken


Nitpick: Token is any here

prastoin · 2025-01-17T14:00:07Z

packages/twenty-e2e-testing/project.json

@@ -8,7 +8,8 @@
      "options": {
        "cwd": "packages/twenty-e2e-testing",
        "commands": [
-          "yarn playwright install"
+          "yarn playwright install",
+          "cp .env.example .env"


Praise: Good idea, about what greptile says I think this might even be a better behavior as for the moment we don't any env variables that needs a user prompt.

prastoin · 2025-01-17T14:20:18Z

packages/twenty-e2e-testing/tests/workflow-creation.spec.ts

+import { deleteWorkflow } from '../lib/requests/delete-workflow';
+import { destroyWorkflow } from '../lib/requests/destroy-workflow';
+
+test('Create workflow', async ({ page }) => {


Remark: I think in a more Page Object Model oriented vision this could result from interaction with the workflow Page

prastoin · 2025-01-17T14:21:47Z

packages/twenty-e2e-testing/tests/workflow-visualizer.spec.ts

+import { test } from '../lib/fixtures/blank-workflow';
+
+test('Create simple workflow', async ({ workflowVisualizer, page }) => {
+  const addTriggerButton = page.getByText('Add a Trigger');


Nitpick: Unused workflowVisualizer destructured args
Nitpick: filename workflow-visualizer.spec.ts, in my opinion, does not really relate the test's business core

prastoin · 2025-01-17T14:22:42Z

packages/twenty-e2e-testing/playwright.config.ts

@@ -30,35 +30,24 @@ export default defineConfig({
    screenshot: 'on', // either 'on' here or in different method in modules, if 'on' all screenshots are overwritten each time the test is run
    headless: true, // instead of changing it to false, run 'yarn test:e2e:debug' or 'yarn test:e2e:ui'
    testIdAttribute: 'data-testid', // taken from Twenty source
-    viewport: { width: 1920, height: 1080 }, // most laptops use this resolution
-    launchOptions: {
-      slowMo: 500, // time in milliseconds between each step, better to use it than explicitly define timeout in tests


Praise: 👀

Devessier added 12 commits January 16, 2025 17:25

chore: delete unused files

55028b1

build: copy the .env.example file when setting up the e2e tests

2bf41b1

test: simplify projects and rename e2e test files

0c38b5e

test: write test for workflow creation

740fbd3

test: delete the created workflow after the end of the test

370ff36

test: update the code to work with the new command menu

8af64d8

test: centralize backend url declaration and rename first test

5421eee

test: create fixture for the workflow visualizer with the beginning o…

6cebc2d

…f a first test

test: generate a new user for every test

a5880af

Deleting an account doesn't clear the user from the database, it just removes the user from the workspace. The test used to fail when ran several times.

test: strenghten some selectors and add assertions

47f7d05

test: remove the delay between each step

f8ebb0f

test: permanently destroy the created workflows after the tests

c83b7f0

Devessier added the -PR: awaiting author label Jan 17, 2025

greptile-apps bot reviewed Jan 17, 2025

View reviewed changes

FelixMalfait added the run-e2e label Jan 17, 2025

Devessier added -PR: awaiting review and removed -PR: awaiting author labels Jan 17, 2025

prastoin self-requested a review January 17, 2025 12:54

thomtrp reviewed Jan 17, 2025

View reviewed changes

prastoin and others added 2 commits January 17, 2025 15:14

ci(e2e): delegate .env copy to the twenty-e2e-testing setup command

d331081

ci: remove duplicated cp command

4cdc8fd

prastoin approved these changes Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workflow e2e tests – 1st batch #9713

Workflow e2e tests – 1st batch #9713

Devessier commented Jan 17, 2025 •

edited

Loading

github-actions bot commented Jan 17, 2025 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Jan 17, 2025

greptile-apps bot Jan 17, 2025

greptile-apps bot Jan 17, 2025

greptile-apps bot Jan 17, 2025

greptile-apps bot Jan 17, 2025

greptile-apps bot Jan 17, 2025

greptile-apps bot Jan 17, 2025

greptile-apps bot Jan 17, 2025

greptile-apps bot Jan 17, 2025

greptile-apps bot Jan 17, 2025

thomtrp left a comment

prastoin left a comment •

edited

Loading

prastoin Jan 17, 2025 •

edited

Loading

prastoin Jan 17, 2025 •

edited

Loading

prastoin Jan 17, 2025

prastoin Jan 17, 2025

prastoin Jan 17, 2025

		# Note that provide always without trailing forward slash to have expected behaviour
		FRONTEND_BASE_URL=http://localhost:3001

		await expect(triggerNode).toHaveClass(/selected/);
		await expect(triggerNode).toHaveText(/Company is Created/);

Workflow e2e tests – 1st batch #9713

Are you sure you want to change the base?

Workflow e2e tests – 1st batch #9713

Conversation

Devessier commented Jan 17, 2025 • edited Loading

github-actions bot commented Jan 17, 2025 • edited Loading

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Jan 17, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 17, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 17, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 17, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 17, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 17, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 17, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 17, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 17, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 17, 2025

Choose a reason for hiding this comment

thomtrp left a comment

Choose a reason for hiding this comment

prastoin left a comment • edited Loading

Choose a reason for hiding this comment

prastoin Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

prastoin Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

prastoin Jan 17, 2025

Choose a reason for hiding this comment

prastoin Jan 17, 2025

Choose a reason for hiding this comment

prastoin Jan 17, 2025

Choose a reason for hiding this comment

Devessier commented Jan 17, 2025 •

edited

Loading

github-actions bot commented Jan 17, 2025 •

edited

Loading

prastoin left a comment •

edited

Loading

prastoin Jan 17, 2025 •

edited

Loading

prastoin Jan 17, 2025 •

edited

Loading