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: [various] - Cypress test cleanup and flake fixes #10405

Merged

Conversation

jdamore-linode
Copy link
Contributor

@jdamore-linode jdamore-linode commented Apr 25, 2024

Description 📝

Cleans up and fixes various tests that have been especially flaky lately.

Changes 🔄

List any change relevant to the reviewer.

  • Fixes a flaw with VPC subnet Linode assignment test that would cause it to randomly fail. This was because the mock subnet object contained 5 Linodes with random IDs, and occasionally the mock Linode would have a matching ID, preventing it from appearing in the assignment select drop-down. See M3-8036.
  • Fixes an OBJ access key test flaw where the test would fail on accounts that have many OBJ buckets*
  • Works around an OBJ bug to prevent failures in Object Storage end-to-end tests. See also M3-8027.
  • Refactors Linode config tests (and related Linode create utils) to be resilient to future kernel upgrades. See M3-8034.
  • Cleans up power state tests, removes some redundant assertions to shed some time off the tests
  • Marks various utils as deprecated to discourage their use in new development

How to test 🧪

We can rely on CI.

Copy link

github-actions bot commented Apr 25, 2024

Coverage Report:
Base Coverage: 81.82%
Current Coverage: 81.82%

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Very nice 🔥 Thanks for getting all of these things addressed

Left one question, but it doesn't need to hold up merging

Comment on lines +81 to +88
// Fetch Linode kernel data from the API.
// We'll use this data in the tests to confirm that config labels are rendered correctly.
cy.defer(fetchAllKernels(), 'Fetching Linode kernels...').then(
(fetchedKernels) => {
kernels = fetchedKernels;
}
);
});
Copy link
Member

Choose a reason for hiding this comment

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

I'm cool with this, but are there other ways to do it?

For example, could we intercept the kernels response that Cloud Manager makes to avoid needing to defer here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question! Intercepting the request and looking at the response was my first approach, and it technically worked, but I didn't really like it because of these points:

  • We don't get any type safety on the response object, so we have to explicitly check and guard against undefined values, etc.. The end result was I had all these conditionals in the middle of the tests checking the response data and throwing if they didn't have the right shape, and it was just kind of ugly. We do do this in other places throughout the tests, though.
    • Long term I'd like to come up with a command that can intercept requests and validate the response so we don't have to do it manually every time, but I'm not 100% certain it's possible (haven't tried yet), and it doesn't address the next point...
  • This is a paginated response, so it only works when we're certain that the data we want is included in the response that we're intercepting. The kernels endpoint responds with fewer than 500 items so, for now, linode/latest-64bit is guaranteed to be in the response, but that might not be the case as more and more items get added to the response (there are 328 items right now). Things start getting really tricky when we start trying to intercept multiple requests.

It is a bummer to have to fire off more API requests in the tests so definitely happy to keep discussing. I'd love a better approach and I bet we could figure one out!

Copy link
Member

Choose a reason for hiding this comment

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

That all makes sense! Thanks for the detailed explanation 💯

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.

Thank you for all this clean up and the corresponding explanations! 🧼

CI looks good; I read through the PR and ran the linode-config test locally to watch it. (That form has a lot of possible situations resulting in warning notices to display, and I don't think we're testing all of them, currently, so I might add to it at the end of M3-7977.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you! I had wondered about these.

@mjac0bs mjac0bs added the Approved Multiple approvals and ready to merge! label Apr 25, 2024
@jdamore-linode jdamore-linode merged commit 1534a9c into linode:develop Apr 26, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants