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

tests: [M3-8656] - Mock LKE creation flow + APL coverage #11347

Merged
merged 17 commits into from
Dec 10, 2024

Conversation

abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Dec 2, 2024

Description 📝

PR to remove any "real" entity creation during the create cluster flow and add APL coverage to the create specs.

Note

The reason for mocking the main create flow is to avoid creating real clusters in the CI for security reasons.

Changes 🔄

  • Change main create flow to use mocks
  • Support LKE creation with APL

How to test 🧪

Prerequisites

Verification steps

  • Run code locally + yarn cy:debug > select lke-create
  • Confirm all other LKE suites are running fine in the CI

As an Author, to speed up the review process, I considered 🤔

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@abailly-akamai abailly-akamai self-assigned this Dec 2, 2024
@abailly-akamai abailly-akamai added LKE Related to Linode Kubernetes Engine offerings APL labels Dec 2, 2024
@abailly-akamai abailly-akamai changed the title [M3-8656] - Mock LKE tests + APL integration test: [M3-8656] - Mock LKE tests + APL integration Dec 2, 2024
@abailly-akamai abailly-akamai added Testing e2e Indicates that a PR touches Cypress tests in some way labels Dec 2, 2024
@abailly-akamai abailly-akamai marked this pull request as ready for review December 2, 2024 20:05
@abailly-akamai abailly-akamai requested review from a team as code owners December 2, 2024 20:05
@abailly-akamai abailly-akamai requested review from cliu-akamai, dwiley-akamai, hkhalil-akamai and jdamore-linode and removed request for a team December 2, 2024 20:05
@abailly-akamai abailly-akamai changed the title test: [M3-8656] - Mock LKE tests + APL integration tests: [M3-8656] - Mock LKE tests + APL integration Dec 2, 2024
@abailly-akamai abailly-akamai changed the title tests: [M3-8656] - Mock LKE tests + APL integration tests: [M3-8656] - Mock LKE creation flow + APL coverage Dec 2, 2024
Copy link

github-actions bot commented Dec 2, 2024

Coverage Report:
Base Coverage: 86.84%
Current Coverage: 86.84%

@mjac0bs
Copy link
Contributor

mjac0bs commented Dec 2, 2024

👋🏼 Just created some merge conflicts here in the lke-create spec by merging in #11322. Hopefully they're easy to resolve - LKE-E test coverage is minimal and will be built out as we continue. Thanks for the clean up in this file; will follow suit going forward.

packages/api-v4/src/account/types.ts Outdated Show resolved Hide resolved
packages/api-v4/src/account/types.ts Outdated Show resolved Hide resolved
mockedLKEClusterPools,
mockedLKEClusterTypes
);

it('can create an LKE cluster', () => {
cy.tag('method:e2e', 'purpose:dcTesting');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we are mocking, do we need to update these tags?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! @abailly-akamai I think in this case you can remove the cy.tag(...) call altogether since it's not suitable for DC testing without real API requests 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done! all feedback addressed - @jdamore-linode would be great to have a thorough review (feel free to commit!) to avoid any flake. It's prob the most complicated mocked test I have written so far, wanna make sure it's reliable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdamore-linode - another poke to get your attention on this one when you have a minute - thx!

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ✅
Tests locally and in CI ✅

Screenshot 2024-12-04 at 12 30 34 PM

@hkhalil-akamai hkhalil-akamai added the Approved Multiple approvals and ready to merge! label Dec 5, 2024
Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing feedback!

Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great @abailly-akamai! Ran the tests on repeat and didn't observe any flake, and nothing in the code caught my eye. This is a great refactor! Thanks!

Comment on lines 193 to 196
const labelInput = '[data-qa-textfield-label="Cluster Label"]';
cy.get(labelInput).should('be.visible');
cy.get(labelInput).click();
cy.get(labelInput).type(`${clusterLabel}{enter}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is totally fine, but just wanted to share a couple alternative options when you've got a selector that you don't want to keep repeating:

// Using aliases
cy.get('[data-qa-textfield-label="Cluster Label"]').as('@labelInput');
cy.get('@labelInput').should('be.visible');
cy.get('@labelInput').click();
cy.get('@labelInput').type(`${clusterLabel}{enter}`);
// Using `cy.focused()` when performing multiple interactions on an element.
cy.get('[data-qa-textfield-label="Cluster Label"]').should('be.visible').click();
cy.focused().type(`${clusterLabel}{enter}`);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, thank you!

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🔺 1 failing test on test run #13 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
1 Failing466 Passing2 Skipped99m 16s

Details

Failing Tests
SpecTest
smoke-billing-activity.spec.tsBilling Activity Feed » displays correct timezone for invoice and payment dates

Troubleshooting

Use this command to re-run the failing tests:

yarn cy:run -s "cypress/e2e/core/billing/smoke-billing-activity.spec.ts"

@abailly-akamai abailly-akamai merged commit 906e061 into linode:develop Dec 10, 2024
22 of 23 checks passed
Copy link

cypress bot commented Dec 10, 2024

Cloud Manager E2E    Run #6939

Run Properties:  status check passed Passed #6939  •  git commit 906e061da8: tests: [M3-8656] - Mock LKE creation flow + APL coverage (#11347)
Project Cloud Manager E2E
Branch Review develop
Run status status check passed Passed #6939
Run duration 29m 45s
Commit git commit 906e061da8: tests: [M3-8656] - Mock LKE creation flow + APL coverage (#11347)
Committer Alban Bailly
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 4
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 467
View all changes introduced in this branch ↗︎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APL Approved Multiple approvals and ready to merge! e2e Indicates that a PR touches Cypress tests in some way LKE Related to Linode Kubernetes Engine offerings Testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants