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

refactor: Make Cloud Manager Cypress test clean-up more granular #9529

Merged

Conversation

jdamore-linode
Copy link
Contributor

Description 📝

This changes the behavior of our test resource clean up when running Cypress.

Currently, Cypress sends API requests to delete all test account data (i.e. any resources with labels starting with cy-test-) before it runs every .spec.ts file.

This changes the clean up behavior so that only relevant test account resources are cleaned up at the beginning of each test. This brings advantages and disadvantages, so I'd like to hear the opinion of the team before moving forward with this.

Advantages ✅

  • Better performance: Because test account data is only deleted when required, performance should improve for most tests since less time will be spent fetching and deleting resources. The majority of our tests require no clean up at all.
  • Improved stability: Bad HTTP responses during test clean up is one of our biggest causes of test flake. Reducing the amount of clean up that's performed should dramatically improve flakiness, and when bad responses do occur, the resulting test failure will actually be relevant since tests only clean up resources when necessary

Disadvantages ❌

  • Accumulation of test resources: Test resources will accumulate more under this model since clean up will be less frequent. This should not impact test stability but may become a burden when running tests locally since more test resources will be leftover. Additionally, this may increase the test account maintenance burden in certain circumstances (like when we need to free up resources for certain regions, etc.)
  • Greater consideration needed when writing tests: Right now, very little consideration regarding clean up is required when writing new tests. This change will force us to consider the state that the test account must be in for each test to function, and will require us to explicitly clean up resources to achieve that state in a before() hook.

(If anybody can think of any other advantages or disadvantages I'd be happy to add them to the lists above)

Major Changes 🔄

  • Remove existing clean up behavior
  • Add clean up utils
  • Implement clean up individually in each test which requires it
  • Improve resiliency of attached Volume clean up

How to test 🧪

We can rely on the automated tests to confirm this is stable (I plan to run it multiple times). However, I am more concerned about the developer experience implications that come with this change.

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.

Things definitely felt it bit quicker when running tests with yarn cy:debug locally. It also looks like the tests could be slightly faster in Jenkins.

You know best if the benefits outweigh the disadvantages so the decision is ultimately up to you but this has my approval!

@bnussman-akamai bnussman-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Aug 9, 2023
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

nice, thanks for improving performance! feels faster indeed.

left a couple comments that are more questions 👍

@@ -46,7 +48,12 @@ const fakeRegionsData = {
],
};

authenticate();
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably unimportant, but could it matter that we need to authenticate in order to run a clean up job for some other tests? (now or in the future)

I also assume the authenticate job takes some time.

Again, not at all a blocker, just curious about trade offs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abailly-akamai I don't think so! We're already calling the authenticate() function before doing clean up the current way, and it's not actually making any API request, it just configures Axios to automatically include the auth token in each API request.

I don't think this function can technically fail, but if for some reason it didn't succeed, it would result in the same behavior that happens when MANAGER_OAUTH isn't defined in .env (or is defined with a bad value): HTTP 401 responses when trying to clean up resources, resulting in the entire spec failing without even starting the tests.

const promise = async () => {
for (const resource of resourcesArray) {
const cleanFunction = cleanUpMap[resource];
// Perform clean-up sequentially to avoid API rate limiting.
Copy link
Contributor

Choose a reason for hiding this comment

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

Running this in parallel could have been nice! Again, not crucial at all, but do you know if rate limiting could be disabled for say, an account with a specific flag?

Copy link
Contributor Author

@jdamore-linode jdamore-linode Aug 10, 2023

Choose a reason for hiding this comment

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

Running this in parallel could have been nice! Again, not crucial at all, but do you know if rate limiting could be disabled for say, an account with a specific flag?

Actually, the existing clean up is done in parallel using Promise.all(), but when I started it was done sequentially using cy.request(). I replaced that with Promises expecting to see a big performance boost and I'm not sure it made much of an impact whatsoever.

I would imagine there's a way to disable the rate limiting but I'd also hate for a new contributor to attempt to run the tests only to get rate limited themselves!

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Aug 10, 2023
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! Missing Changeset
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants