-
Notifications
You must be signed in to change notification settings - Fork 10.9k
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
refactor(core): Improve test-webhooks (no-changelog) #8069
Conversation
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.
Thanks for doing this.
I'm seeing a bit of overlap between this and #7223, would like to cherry-pick some bits out of that PR in here if possible.
let path = request.params.path.endsWith('/') | ||
? request.params.path.slice(0, -1) | ||
: request.params.path; | ||
|
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.
considering that we do this for test and prod webhooks, maybe we should move this bit to WebhookHelpers.webhookRequestHandler
, and start passing a context object into executeWebhook
calls.
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.
Not quite familiar yet with WebhookHelpers
to address quickly, I'll come back to this later if needed.
Please go ahead! |
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.
besides the one comment, LGTM.
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.
👍🏽
2 flaky tests on run #3416 ↗︎
Details:
16-webhook-node.cy.ts • 1 flaky test
17-sharing.cy.ts • 1 flaky test
Review all test suite changes for PR #8069 ↗︎ |
✅ All Cypress E2E specs passed |
* master: (22 commits) fix(editor): Make keyboard shortcuts more strict; don't accept extra Ctrl/Alt/Shift keys (#8024) fix(core): Downgrade Rudderstack SDK (no-changelog) (#8107) fix(editor): Move versions check to init function and refactor store (no-changelog) (#8067) refactor(editor): Add telemetry for SSO/SAML (no-changelog) (#8102) fix(editor): Ensure execution data overrides pinned data when copying in executions view (#8009) fix(editor): Fix copy/paste issue when switch node is in workflow (#8103) feat(editor): Upgrade frontend tooling to address a few vulnerabilities (#8100) feat(editor): De-duplicate frontend devDependencies (no-changelog) (#8094) refactor(core): Improve test-webhooks (no-changelog) (#8069) refactor: Add telemetry for RBAC (no-changelog) (#8056) feat(core): Upgrade Rudderstack SDK (no-changelog) (#8090) fix: Upgrade axios to address CVE-2023-45857 (#7713) fix(core): Do not display error when stopping jobless execution in queue mode (#8007) feat(editor): Gracefully ignore invalid payloads in postMessage handler (#8096) feat(editor): Add lead enrichment suggestions to workflow list (#8042) refactor(Discord Node): Stop reporting to Sentry inaccessible guild error (no-changelog) (#8095) feat: Add opt-in enterprise license trial checkbox (no-changelog) (#7826) ci: Remove unnecessary async/await, enable await-thenable linting rule (no-changelog) (#8076) refactor(editor): Add telemetry for log streaming (no-changelog) (#8075) fix(core): Use relative imports for dynamic imports in SecurityAuditService (#8086) ...
Got released with |
Remove duplication, improve readability, and expand tests for
TestWebhooks.ts
- in anticipation for storing test webhooks in Redis.