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-7516] - Cypress tests for Parent → Child account switching flows #10110

Conversation

jdamore-linode
Copy link
Contributor

Description 📝

Adds Cypress integration tests to cover Parent/Child flows involving account switching from a Parent account to a Child account.

Changes 🔄

  • Adds tests to cover Parent/Child account switching flows:
    • Switch from Parent to Child using Account Billing page
    • Switch from Parent to Child using user menu
    • Error flows upon fetching child accounts and switching accounts
  • Adds various utilities to support tests
    • Intercept/mock utils for child account API endpoints
    • Util to assert local storage values

How to test 🧪

We can rely on automated tests for this, but you can also run the test locally:

Prerequisites

Build and serve Cloud:

yarn && yarn build && yarn start:manager:ci

Verification steps

yarn cy:run -s "cypress/e2e/core/parentChild/account-switching.spec.ts"

As an Author I have considered 🤔

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

Comment on lines 155 to 157
// TODO Remove the call to `cy.reload()` once Cloud Manager automatically updates itself upon account switching.
// TODO Add assertions for toast upon account switch.
cy.reload();
Copy link
Contributor Author

@jdamore-linode jdamore-linode Jan 25, 2024

Choose a reason for hiding this comment

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

cc @jaalah-akamai @mjac0bs -- I think this relates to this discussion in the account switching PR.

Cloud definitely isn't responding as expected upon updating the Local Storage auth values, so a page reload is currently necessary on the Cypress (i.e. user) side to trigger the change. I'm guessing if we want to avoid an automatic page reload, it'll involve at least resetting Cloud's Axios interceptors, resetting React Query, and maybe doing something with our Redux state?

Either way, this test can be used to develop/troubleshoot this functionality without having to wait for anything from the API -- just remove these calls to cy.reload()

Copy link

github-actions bot commented Jan 25, 2024

Coverage Report:
Base Coverage: 81.24%
Current Coverage: 81.24%

Comment on lines +45 to +60
/**
* Confirms that expected authentication values are set in Local Storage.
*
* @param token - Authentication token value to assert.
* @param expiry - Authentication expiry value to assert.
* @param scopes - Authentication scope value to assert.
*/
const assertAuthLocalStorage = (
token: string,
expiry: string,
scopes: string
) => {
assertLocalStorageValue('authentication/token', token);
assertLocalStorageValue('authentication/expire', expiry);
assertLocalStorageValue('authentication/scopes', scopes);
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question for @jaalah-akamai or @mjac0bs: does the token value returned by the API already include the Bearer prefix, or do we have logic somewhere that prepends it before saving it in Local Storage?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe it includes the Bearer prefix. It looks like the token first gets set when a session begins and Bearer comes from token_type, which is how it ends up with Bearer in local storage.

And from the API spec:

image

On a related bearer-in-token-related note, I'm not sure that we're supposed to have Bearer when calling getChildAccountPersonalAccessToken here in the switch account drawer. Maybe it was a oversight or I'm missing something and @jaalah-akamai can clarify.

Copy link
Contributor

Choose a reason for hiding this comment

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

On a related bearer-in-token-related note, I'm not sure that we're supposed to have Bearer when calling getChildAccountPersonalAccessToken here in the switch account drawer. Maybe it was a oversight or I'm missing something and @jaalah-akamai can clarify.

This is true - left over probably from one of my iterations ✅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @jaalah-akamai and @mjac0bs -- sorry for the slow follow up here.

If I'm understanding your explanation @mjac0bs, it sounds like we're supposed to have the Bearer prefix in local storage but the token that gets stored during the account switching flow does not contain the prefix.

Is there a ticket for this? I can update this test to expect the Bearer prefix but it will fail in the meantime.

@jdamore-linode jdamore-linode requested a review from a team as a code owner January 25, 2024 18:50
@jdamore-linode jdamore-linode requested review from dwiley-akamai and bnussman-akamai and removed request for a team January 25, 2024 18:50
Comment on lines +151 to +153
mockGetAccount(mockChildAccount);
mockGetProfile(mockParentProfile);
mockGetUser(mockParentUser);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also hoping for an extra set of eyes here -- do these mocks accurately reflect what the account, profile, and user API request will look like in this situation?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that mockGetUser is now left over from when we previously had to call */users/:user to retrieve the user_type and it can now be removed.

Otherwise, this seems correct. We'll have the child account once logged in as a proxy user and we'll also need to fetch the parent's username from */profile.

mockChildAccountToken.token!,
mockChildAccountToken.expiry!,
mockChildAccountToken.scopes
);
Copy link
Contributor

Choose a reason for hiding this comment

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

We also want to ensure that parent tokens are being created here too

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, the Switch Account drawer does that 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.

Going to hold off on this for now -- the Cypress tests deal with a real token, so asserting its value will cause it to be logged and recorded.

Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

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

This looks good overall - nice tests 🙏

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.

Remote and local tests passed ✅
Changeset ✅

packages/manager/cypress/support/util/local-storage.ts Outdated Show resolved Hide resolved
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, Joe - looks good overall.

Left a comment about removing some extra mockGetUser calls now that #10102 is merged and a thought about naming.

Comment on lines +151 to +153
mockGetAccount(mockChildAccount);
mockGetProfile(mockParentProfile);
mockGetUser(mockParentUser);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that mockGetUser is now left over from when we previously had to call */users/:user to retrieve the user_type and it can now be removed.

Otherwise, this seems correct. We'll have the child account once logged in as a proxy user and we'll also need to fetch the parent's username from */profile.

mockChildAccountToken.token!,
mockChildAccountToken.expiry!,
mockChildAccountToken.scopes
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, the Switch Account drawer does that here.

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Feb 6, 2024
jdamore-linode and others added 6 commits February 14, 2024 10:08
Fix doc comment typo

Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
…g.spec.ts


Update test title to be more accurate re: "child" vs "proxy"

Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
@jdamore-linode jdamore-linode merged commit f08b57c into linode:develop Feb 22, 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! Parent / Child Account
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants