-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: add multistep construct and support for running test session [sc-17814] #868
Conversation
This pull request has been linked to Shortcut Story #17814: Update public API to allow to run/test multi-steps through the CLI. |
This pull request has been linked to Shortcut Story #17815: Add new MultiStepCheck constructor to the CLI. |
* | ||
* This class make use of the multi-step checks endpoints. | ||
*/ | ||
export class MultiStepCheck extends Check { |
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.
In the PR description, you mention Adds multistep check construct, based off of browser construct
but it extends Check and not BrowserCheck. Is it wanted?
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.
Yeah it was just based off of the browsercheck construct, meaning they both extend Check
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.
@ndom91 @Antoine-C we should not add the sslDomain
property
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.
Haha we had this discussion elsewhere recently. They're still included in the multistep UI as well. So should we remove them completely from multistep?
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.
Removed
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.
Do we need to update the API?
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.
Yeah good question, that might be the reason for the failing tests. Doing some more digging here today..
I hereby confirm that I followed the code guidelines found at engineering guidelines
Affected Components
Notes for the Reviewer
17814
, we already supportedMULTI_STEP
check runs via test-session due tocheckly-backend/api/src/modules/public-api/test-sessions/PublicTestSessionsController.js:93
and the following code paths submitting the check run to the correct queue based off of itscheckType
.Manually tested with
npm create checkly
simple example and the following files:Adding playwrights
context
tohomepage.spec.ts:5
test callback arguments correctly returnscannot use 'context' in MULTI_STEP checks
, confirming its been submitted and handled by2023-09-multistep
runner.New Dependency Submission