-
Notifications
You must be signed in to change notification settings - Fork 357
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: [M3-8146] - Cypress tests for refactored Linode Create via CLI flow #10765
Conversation
test
: [M3-8146] - Cypress tests for refactored Linode Create via CLI flow
2a826d1
to
655546c
Compare
test
: [M3-8146] - Cypress tests for refactored Linode Create via CLI flow
Coverage Report: ✅ |
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 test failed in CI, but it does run and pass locally (5+ times), and I can't quite follow what happened in the CI run.
It looks like we're testing the wrong flow here, though! See my comment on the legacy-create-linode.spec.ts
.
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.
How did this get in here?
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.
I may be misunderstanding the point of this ticket, but based on the comment on L4-L5 of this file:
// TODO Delete this test file when `linodeCreateRefactor` feature flag is retired.
// Move out any tests (e.g. region select test) for flows that aren't covered by new tests in the meantime.
It sounds like we wouldn't want to make updates in this file, but rather write a new test file that tests CLI in the refactored (v2) flow. Since this test suite mocks the linode create feature flag false, we don't seem to be doing that here.
cy.visitWithLogin('/linodes/'); | ||
|
||
// Confirm that the linode is created. | ||
cy.findByText(linodeLabel).should('be.visible'); | ||
cy.findByText('Provisioning', { exact: false }).should('be.visible'); |
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 what the acceptance criteria requested here: Confirm Cloud Manager behavior to automatically redirect to created Linode after CLI command is executed
. Isn't the redirect to the Linode's details page?
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.
No, it doesn't redirect after sending the CLI.
"tags": [], | ||
"type": "${payload.type}" | ||
}' ${API_ROOT}/linode/instances`; | ||
cy.exec(curlRequest).then((res) => { |
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.
The CI error is being caused by executing the curl command. It seems like the jenkins instance doesn't have curl installed on it.
Stderr:
/usr/bin/bash: line 1: curl: command not found
@jdamore-linode can we update the jenkins docker config to install cURL if we want to go ahead with this approach?
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.
@cliu-akamai (cc @hkhalil-akamai) we probably shouldn't be doing this E2E, and instead should mock the relevant events to trigger/test the redirect behavior, even though this is really, really cool as it is.
(I'm only reluctant to do this because we wouldn't have the normal security guardrails available, like ensuring that the created Linode can't connect to the internet, etc. -- basically none of the protections that were added by #10538 and #10633 would apply to this cURL call)
This PR is stale because it has been open 15 days with no activity. Please attend to this PR or it will be closed in 5 days |
Here's the code that detects the event and triggers the redirect: manager/packages/manager/src/features/Linodes/LinodesCreate/ApiAwarenessModal/ApiAwarenessModal.tsx Lines 64 to 69 in 14b0243
We can try mocking this instead of manually executing the curl command. |
This PR is stale because it has been open 15 days with no activity. Please attend to this PR or it will be closed in 5 days |
Description 📝
Refactor Cypress test to the flow of creating Linode through CLI.
Major Changes 🔄
How to test 🧪