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

fix: Fix E2E failures caused by inadequate resource clean-up #9614

Conversation

jdamore-linode
Copy link
Contributor

@jdamore-linode jdamore-linode commented Aug 30, 2023

Description 📝

Another test resource clean-up PR! This stems from the changes made in #9529. This should fix some of the failures we've recently seen in these specs:

  • smoke-create-image.spec.ts
  • backup-linode.spec.ts
  • longview.spec.ts

In all 3 cases, the test failures are due to the tests creating Linodes but failing to clean up existing Linodes first.

How to test 🧪

We can rely on the automated test run to confirm that these 3 specs pass, indicating that these changes haven't broken the tests. However, manual testing may be required to confirm that the clean up actually functions as expected:

  1. Create some test Linodes whose labels start with cy-test-
  2. Navigate to the Linodes landing page in your browser, and stay there
  3. Separately run one of the spec files above, and observe in your browser that the Linodes you created in step 1 get cleaned up automatically by the test
  4. Repeat steps 1-3 for the other 2 spec files

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

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

tests cleaned up the linodes before they ran 🎉

One weird thing I noticed when running backup-linode.spec.ts was that although the test deletes the linode it created at the end, the linode still shows up for a bit in the landing page (but since it's already deleted, the delete modal won't work):
image

The already deleted linode eventually disappears from the landing page after a minute or so -- is this just the frontend lagging behind?

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Aug 31, 2023
@jdamore-linode
Copy link
Contributor Author

jdamore-linode commented Aug 31, 2023

tests cleaned up the linodes before they ran 🎉

One weird thing I noticed when running backup-linode.spec.ts was that although the test deletes the linode it created at the end, the linode still shows up for a bit in the landing page (but since it's already deleted, the delete modal won't work):

The already deleted linode eventually disappears from the landing page after a minute or so -- is this just the frontend lagging behind?

Oh interesting! I tried a time or two to reproduce this and surprisingly wasn't able to, but I think I might have an idea of what's going on there because I think I've seen similar behavior on the Volumes landing page.

I think what may have happened was that the Cloud Manager page running via Cypress polled the API events endpoint and received the event for the Linode that was just deleted, and meanwhile a second or two later the Cloud Manager page running in your browser polled the same events endpoint, and since the delete event had already been consumed, it wasn't included in the response and so Cloud Manager in your browser basically had no way of knowing that that Linode was deleted. Admittedly I probably have the least solid understanding of how our events polling works, but I think this is a possibility given my (limited) understanding.

Edit: Although I'm not sure what would cause the page to finally refresh itself after a minute, unless we have some React Query mechanism that forces the page to re-query the Linodes endpoint after some interval?

@coliu-akamai
Copy link
Contributor

I read in the docs that react query has a mechanism to clear data from its cache after a while and refetch the data -- maybe that's going on here? Haven't looked into it too deeply for here yet, but that seems like it could be it

@jdamore-linode jdamore-linode merged commit 8bb6186 into linode:develop Sep 1, 2023
bnussman-akamai pushed a commit to bnussman-akamai/manager that referenced this pull request Sep 5, 2023
tyler-akamai pushed a commit to bnussman-akamai/manager that referenced this pull request Sep 5, 2023
corya-akamai pushed a commit to corya-akamai/manager that referenced this pull request Sep 6, 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.

3 participants