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: [M3-8146] - Cypress tests for refactored Linode Create via CLI flow #10765

Closed
wants to merge 2 commits into from

Conversation

cliu-akamai
Copy link
Contributor

Description 📝

Refactor Cypress test to the flow of creating Linode through CLI.

Major Changes 🔄

  • Refactor Cypress test that creates Linode through CLI.

How to test 🧪

yarn cy:run -s "cypress/e2e/core/linodes/legacy-create-linode.spec.ts"

@cliu-akamai cliu-akamai requested review from a team as code owners August 8, 2024 21:24
@cliu-akamai cliu-akamai requested review from jdamore-linode, mjac0bs and hkhalil-akamai and removed request for a team August 8, 2024 21:24
@cliu-akamai cliu-akamai changed the title Feature/m3 8146 test: [M3-8146] - Cypress tests for refactored Linode Create via CLI flow Aug 8, 2024
@cliu-akamai cliu-akamai changed the title test: [M3-8146] - Cypress tests for refactored Linode Create via CLI flow test: [M3-8146] - Cypress tests for refactored Linode Create via CLI flow Aug 8, 2024
Copy link

github-actions bot commented Aug 8, 2024

Coverage Report:
Base Coverage: 82.41%
Current Coverage: 82.41%

Copy link
Contributor

@mjac0bs mjac0bs left a 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.

Copy link
Contributor

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?

Copy link
Contributor

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.

Comment on lines +308 to +312
cy.visitWithLogin('/linodes/');

// Confirm that the linode is created.
cy.findByText(linodeLabel).should('be.visible');
cy.findByText('Provisioning', { exact: false }).should('be.visible');
Copy link
Contributor

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?

Copy link
Contributor Author

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) => {
Copy link
Contributor

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?

Copy link
Contributor

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)

Copy link

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

@github-actions github-actions bot added the Stale label Aug 28, 2024
@hkhalil-akamai
Copy link
Contributor

Here's the code that detects the event and triggers the redirect:

const linodeCreationEvent = events?.find(
(event) =>
(event.action === 'linode_create' || event.action === 'linode_clone') &&
event.entity?.label === payLoad.label &&
(event.status === 'scheduled' || event.status === 'started')
);

We can try mocking this instead of manually executing the curl command.

@cliu-akamai

@github-actions github-actions bot removed the Stale label Aug 29, 2024
Copy link

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

@github-actions github-actions bot added the Stale label Sep 14, 2024
@github-actions github-actions bot closed this Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants