-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
test(web): Migrate Cypress tests to Playwright #5555
Conversation
✅ Deploy Preview for dev-web-novu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for novu-design ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
c5071ed
to
3cb6086
Compare
997d62a
to
b11b151
Compare
.github/workflows/test.yml
Outdated
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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/scheduled_e2e.yml
Outdated
test_widget: | ||
name: Test Widget Cypress | ||
uses: ./.github/workflows/reusable-widget-e2e.yml | ||
with: | ||
ee: true | ||
secrets: inherit |
There was a problem hiding this comment.
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
.vscode/launch.json
Outdated
{ | ||
"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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
ecaa0c2
to
5307f32
Compare
74bdff7
to
cd1ae13
Compare
There was a problem hiding this 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
741f41b
to
e3656a5
Compare
e3656a5
to
240dcc5
Compare
@@ -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 |
There was a problem hiding this comment.
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 | |||
|
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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 */ |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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.```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed now.
ab0a939
to
405d328
Compare
405d328
to
dcc6568
Compare
What changed? Why was the change needed?
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer