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-7500] - Add tests for Parent/Child Users & Grants page enhancements #10240

Merged
merged 8 commits into from
Apr 25, 2024

Conversation

cliu-akamai
Copy link
Contributor

@cliu-akamai cliu-akamai commented Feb 29, 2024

Description 📝

Add regression tests to check enhancements in users landing page.

Major Changes 🔄

  • Add new tests to check the Users & Grants page.
  • Move some existing test from user permission spec.

How to test 🧪

yarn cy:run -s "cypress/e2e/core/account/users-landing-page.spec.ts,cypress/e2e/core/account/user-permissions.spec.ts"

@cliu-akamai cliu-akamai requested a review from a team as a code owner February 29, 2024 20:19
@cliu-akamai cliu-akamai requested review from jdamore-linode and mjac0bs and removed request for a team February 29, 2024 20:19
Copy link

github-actions bot commented Feb 29, 2024

Coverage Report:
Base Coverage: 81.86%
Current Coverage: 81.86%

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.

Cassie, thanks for tackling this and moving some of the tests around.

I realized our ticket for M3-7500 was not completely accurate, which means we'll need some changes for how we're checking the Child Account Access column. Comments inline, but let me know if you have any questions!

@cliu-akamai cliu-akamai requested a review from a team as a code owner March 18, 2024 18:32
@cliu-akamai cliu-akamai requested review from bnussman-akamai and hkhalil-akamai and removed request for a team March 18, 2024 18:32
@mjac0bs mjac0bs self-requested a review March 18, 2024 19:07
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.

I took another pass at this. Our mocks and test cases aren't reflective of what should happen in CM yet, and part of that was likely my misleading comment about the test cases we should cover. My review comments are an attempt to clarify where we have issues now and outline exactly what test cases we need. Lmk if I can help any further, or if something is still confusing -- happy to help.

@bnussman-akamai
Copy link
Contributor

Can we merge in develop / rebase this PR with develop? I think the Notistack/Snackbar changes might affect these tests

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.

Thanks for addressing feedback. These test cases look accurate now and all are passing on my local runs! I didn't notice any snackbar impact, so I don't think the Notistack update affected things here, but @bnussman-akamai knows more about that than me.

Remaining changes:

  • Can we add the test cases that were removed (because they didn't belong on the user landing test spec, since they were part of the user permissions page) to the user-permissions.spec.ts? I left my earlier comments that were about this unresolved and resolved everything else that has been addressed.
  • Clean up for couple of minor comment updates for clarity/typo fixes

Screenshot 2024-04-15 at 3 17 38 PM

@mjac0bs mjac0bs self-requested a review April 24, 2024 16:39
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.

I did some clean up in 21fe358 to further separate our testing in the Users & Grants landing and User Permissions pages, and avoid duplicating test coverage.

This looks good to me now; both tests passed locally, so as long as all looks good with CI, we can merge this.

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.

Thanks @cliu-akamai and @mjac0bs!

@mjac0bs mjac0bs merged commit 2b951b0 into linode:develop Apr 25, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants