Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(web): Migrate Cypress tests to Playwright #5555

Merged

Conversation

tatarco
Copy link
Contributor

@tatarco tatarco commented May 13, 2024

What changed? Why was the change needed?

Screenshots

Expand for optional sections

Related enterprise PR

Special notes for your reviewer

Copy link

linear bot commented May 13, 2024

Copy link

netlify bot commented May 13, 2024

Deploy Preview for dev-web-novu ready!

Name Link
🔨 Latest commit dcc6568
🔍 Latest deploy log https://app.netlify.com/sites/dev-web-novu/deploys/66582fb8b28a48000816fc57
😎 Deploy Preview https://deploy-preview-5555--dev-web-novu.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented May 13, 2024

Deploy Preview for novu-design ready!

Name Link
🔨 Latest commit dcc6568
🔍 Latest deploy log https://app.netlify.com/sites/novu-design/deploys/66582fb81fa4560008b12dc0
😎 Deploy Preview https://deploy-preview-5555--novu-design.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@tatarco tatarco force-pushed the nv-3650-migrate-the-rest-cypress-tests-to-the-playwright branch 2 times, most recently from c5071ed to 3cb6086 Compare May 19, 2024 16:12
@SokratisVidros SokratisVidros changed the title nv-3650-migrate-the-rest-cypress-tests-to-the-playwright chore(root): nv-3650-migrate-the-rest-cypress-tests-to-the-playwright May 21, 2024
@tatarco tatarco force-pushed the nv-3650-migrate-the-rest-cypress-tests-to-the-playwright branch from 997d62a to b11b151 Compare May 22, 2024 04:31
@tatarco tatarco marked this pull request as ready for review May 22, 2024 06:59
@tatarco tatarco requested review from a team as code owners May 22, 2024 07:00
@tatarco tatarco requested review from scopsy and AliaksandrRyzhou and removed request for a team May 22, 2024 07:00
Comment on lines 104 to 136
test_widget:
name: Test Widget Cypress
needs: [get-affected]
uses: ./.github/workflows/reusable-widget-e2e.yml
with:
ee: true
if: contains(github.event.pull_request.labels.*.name, 'run-e2e')
# if: ${{ contains(fromJson(needs.get-affected.outputs.test-cypress), '@novu/widget') || contains(fromJson(needs.get-affected.outputs.test-unit), '@novu/notification-center') || contains(fromJson(needs.get-affected.outputs.test-unit), '@novu/ws') }}
secrets: inherit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: ‏How are we testing this functionality now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tatarco Widgets should be probably kept here, we migrated just the web tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scopsy please contact @SokratisVidros ,it was a joint decision to cut off cypress as it brings no assurance ATM, push working tests now and move over to fix the rest including the widget but to start by ripping off cypress

.github/workflows/reusable-web-e2e.yml Show resolved Hide resolved
.github/workflows/reusable-widget-e2e.yml Outdated Show resolved Hide resolved
Comment on lines 27 to 32
test_widget:
name: Test Widget Cypress
uses: ./.github/workflows/reusable-widget-e2e.yml
with:
ee: true
secrets: inherit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chore: the widget tests should run in this pipeline

Comment on lines 201 to 227
{
"name": "Cypress open - web",
"request": "launch",
"runtimeArgs": [
"run-script",
"cypress:open"
],
"runtimeExecutable": "npm",
"cwd": "${workspaceFolder}/apps/web",
"skipFiles": [
"<node_internals>/**"
],
"type": "pwa-node"
},
{
"name": "Cypress open - widget",
"request": "launch",
"runtimeArgs": [
"run-script",
"cypress:open"
],
"runtimeExecutable": "npm",
"cwd": "${workspaceFolder}/apps/widget",
"skipFiles": [
"<node_internals>/**"
],
"type": "pwa-node"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: is there any reason to remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing all cypress, why should it stay?

apps/web/.env Outdated
@@ -12,7 +12,7 @@ REACT_APP_SEGMENT_KEY=
REACT_APP_SENTRY_DSN=
REACT_APP_BLUEPRINTS_API_URL=
REACT_APP_MAIL_SERVER_DOMAIN=
REACT_APP_LAUNCH_DARKLY_CLIENT_SIDE_ID=
REACT_APP_LAUNCH_DARKLY_CLIENT_SIDE_ID=some_fake_id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chore: we need to find another way to set this fake LD key, I'm worried that this env would be user during the docker image field and might build the web app and include this key

apps/web/panda.config.ts Show resolved Hide resolved
apps/web/tests/activity-graph.spec.ts Outdated Show resolved Hide resolved
apps/web/tests/page-models/authLoginPage.ts Show resolved Hide resolved
libs/shared-web/src/hooks/useFeatureFlags.ts Outdated Show resolved Hide resolved
libs/shared-web/src/hooks/useFeatureFlags.ts Outdated Show resolved Hide resolved
@tatarco tatarco force-pushed the nv-3650-migrate-the-rest-cypress-tests-to-the-playwright branch from ecaa0c2 to 5307f32 Compare May 22, 2024 10:27
@tatarco tatarco force-pushed the nv-3650-migrate-the-rest-cypress-tests-to-the-playwright branch from 74bdff7 to cd1ae13 Compare May 22, 2024 11:28
Copy link
Contributor

@rifont rifont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's gooo! LGTM 🚢 it

@@ -93,7 +92,7 @@ jobs:
REACT_APP_API_URL: http://127.0.0.1:1336
REACT_APP_WS_URL: http://127.0.0.1:1340
REACT_APP_WEBHOOK_URL: http://127.0.0.1:1341
REACT_APP_LAUNCH_DARKLY_CLIENT_SIDE_ID: ${{ secrets.TEST_LAUNCH_DARKLY_CLIENT_SIDE_ID }}
REACT_APP_LAUNCH_DARKLY_CLIENT_SIDE_ID: some_fake_id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: ‏ should this still be hardcoded?

@@ -47,22 +43,6 @@ runs:
with:
mongodb-version: 4.2.8

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: ‏ Is this still OK to remove given that we still have cypress for the widget, or is this exclusive to for the web tests?

- name: Playwright Install
working-directory: apps/web
run: pnpm playwright:install

- name: Run Playwright tests
working-directory: apps/web
env:
REACT_APP_LAUNCH_DARKLY_CLIENT_SIDE_ID: some_fake_id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: ‏ Same as above, should this stay hardcoded here like this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be safely removed - its a leftover from some experiment I tried before.

@@ -2,7 +2,7 @@ GOOGLE_OAUTH_CLIENT_ID=11
GOOGLE_OAUTH_CLIENT_SECRET=11
GOOGLE_OAUTH_REDIRECT=http://127.0.0.1:3000/v1/auth/google/callback
STORE_ENCRYPTION_KEY="<ENCRYPTION_KEY_MUST_BE_32_LONG>"
BLUEPRINT_CREATOR=645b648b36dd6d25f8650d37
BLUEPRINT_CREATOR=66507de8e76834a745c98f93
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: ‏ I don't know what this is but am curious why it changed...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use the staging API as a template data source. The id changed as staging env was replaced by dev.

/* Opt out of parallel tests on CI. */
workers: process.env.CI ? 4 : undefined,
retries: 2,
/* Use 4 workers in CI, 50% of CPU count in local */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤓 nitpick (non-blocking): ‏ comment unaligned with actual config, should be removed altogether

@@ -6,9 +6,9 @@ import { useBlueprint } from '../../../hooks/index';
export function EnsureOnboardingComplete({ children }: any) {
useBlueprint();
const location = useLocation();
const { claims } = useAuth();
const { currentOrganization, environmentId } = useAuth();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: ‏ This doesn't build for me locally:

TS2339: Property 'environmentId' does not exist on type '{ inPublicRoute: boolean; inPrivateRoute: boolean; isLoading: boolean; currentUser: { organizationId: string; environmentId: string; _id: string; firstName?: string | undefined; lastName?: string | undefined; ... 7 more ...; hasPassword: boolean; }; ... 5 more ...; claims: IJwtClaims; }'.
     7 |   useBlueprint();
     8 |   const location = useLocation();
  >  9 |   const { currentOrganization, environmentId } = useAuth();
       |                                ^^^^^^^^^^^^^
    10 |
    11 |   if ((!currentOrganization || !environmentId) && location.pathname !== ROUTES.AUTH_APPLICATION) {
    12 |     return <Navigate to={ROUTES.AUTH_APPLICATION} replace />;


 ELIFECYCLE  Command failed with exit code 1.```

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed now.

@SokratisVidros SokratisVidros force-pushed the nv-3650-migrate-the-rest-cypress-tests-to-the-playwright branch from 405d328 to dcc6568 Compare May 30, 2024 07:50
@SokratisVidros SokratisVidros merged commit a9c9055 into next May 30, 2024
27 of 30 checks passed
@SokratisVidros SokratisVidros deleted the nv-3650-migrate-the-rest-cypress-tests-to-the-playwright branch May 30, 2024 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants