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-7053] - Add Cypress integration test for VPC create flow #9730

Merged

Conversation

jdamore-linode
Copy link
Contributor

@jdamore-linode jdamore-linode commented Sep 28, 2023

Description 📝

Adds a Cypress integration test for the VPC create flow.

Major Changes 🔄

   - Confirms VPC creation flow using mock API data.
   - Confirms that users can add and remove subnets during create flow.
   - Confirms client side validation when entering invalid labels and IP ranges.
   - Confirms that UI handles API errors gracefully.
   - Confirms that UI redirects to created VPC page after creating a VPC.

How to test 🧪

We can refer to the automated test run, but to run the tests locally you can use this command:

yarn cy:run -s "cypress/e2e/core/vpc/vpc-create.spec.ts"

@jdamore-linode jdamore-linode added Ready for Review VPC Relating to VPC project labels Sep 28, 2023
@jdamore-linode jdamore-linode requested a review from a team as a code owner September 28, 2023 16:59
@jdamore-linode jdamore-linode self-assigned this Sep 28, 2023
@jdamore-linode jdamore-linode requested review from bnussman-akamai and jaalah-akamai and removed request for a team September 28, 2023 16:59
.click()
.type(`${vpcRegion.label}{enter}`);

cy.findByText('VPC label').should('be.visible').click().type(mockVpc.label);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should "VPC label" be "VPC Label" to be more consistent with other resource create flows? (This question also applies to "Subnet label" vs "Subnet Label" later in the flow)

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 makes sense to me too -- I'll ask UX to double check :D

Copy link
Contributor

Choose a reason for hiding this comment

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

confirmed that label should be capitalized :D

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 for confirming @coliu-akamai! I'll make those changes in this PR if that works for everyone 👍

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!

Copy link
Contributor

@hana-akamai hana-akamai left a comment

Choose a reason for hiding this comment

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

Ran the tests locally and they passed 🎉

Comment on lines 72 to 75
const totalSubnetUniqueLinodes = mockSubnets.reduce(
(acc: number, cur: Subnet) => acc + cur.linodes.length,
0
);
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a function to calculate this called getUniqueLinodesFromSubnets

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 @hana-linode! I'll take a look at that and see if I can use it from the test

.click()
.type(`${vpcRegion.label}{enter}`);

cy.findByText('VPC label').should('be.visible').click().type(mockVpc.label);
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me

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.

✅ confirmed tests pass. Thanks Joe!

.click()
.type(`${vpcRegion.label}{enter}`);

cy.findByText('VPC label').should('be.visible').click().type(mockVpc.label);
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 makes sense to me too -- I'll ask UX to double check :D

@coliu-akamai coliu-akamai added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Sep 29, 2023
@coliu-akamai
Copy link
Contributor

coliu-akamai commented Oct 4, 2023

Hey @jdamore-linode! Just wanted to give a heads-up that the VPC Create flow will be changed a from this PR/ticket:
#9751

The changes are:

  • All subnets have the remove [x] button, including the first subnet
  • The button text for adding subnets is changed from “Add Subnet” to “Add Another Subnet” (affects line 156, 199 most likely)
    • If there are no subnets, it's "Add a Subnet"
  • There's a divider below each subnet now

@jdamore-linode
Copy link
Contributor Author

Hey @jdamore-linode! Just wanted to give a heads-up that the VPC Create flow will be changed a from this PR/ticket: #9751

The changes are:

  • All subnets have the remove [x] button, including the first subnet

  • The button text for adding subnets is changed from “Add Subnet” to “Add Another Subnet” (affects line 156, 199 most likely)

    • If there are no subnets, it's "Add a Subnet"
  • There's a divider below each subnet now

Thanks @coliu-akamai and @hana-linode for the heads up! I'll review #9751 later today, and my plan is to wait for that work to be reviewed and merged, and then I'll circle back here and update this PR to account for these changes and add additional coverage where necessary (being able to delete the first subnet specifically comes to mind)

@jdamore-linode
Copy link
Contributor Author

@coliu-akamai @hana-linode Just letting you know I pushed some changes to account for the recent improvements made to the VPC create page:

  • Updated tests to account for "Add a Subnet" label change to "Add another Subnet"
  • Added a new test to create a VPC without Subnets (confirms user can delete the subnet on the create page, confirms that button label changes to "Add a Subnet", etc.)

I don't know if this warrants a re-review but wanted to give you both a heads up in case you wanted to put some eyes on it!

@coliu-akamai
Copy link
Contributor

Thanks @jdamore-linode! Will take a quick look/rerun the tests 👍

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.

Other than missing a new Tests changeset and the minor issue I commented about, this looks good + the cypress tests passed. Reapproving on those being resolved -- thanks Joe!!

@@ -63,15 +63,15 @@ export const SubnetNode = (props: Props) => {
disabled={disabled}
errorText={subnet.labelError}
inputId={`subnet-label-${idx}`}
label="Subnet label"
label="Subnet Label"
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry I forgot to mention -- this change will affect a bunch of tests in the VPCs/VPCCreate folder and the SubnetCreateDrawer.test.tsx file too. It should be a super quick fix (just updating everything to 'Subnet Label' in those tests, but lmk if you've questions!

Copy link
Contributor Author

@jdamore-linode jdamore-linode Oct 9, 2023

Choose a reason for hiding this comment

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

sorry I forgot to mention -- this change will affect a bunch of tests in the VPCs/VPCCreate folder and the SubnetCreateDrawer.test.tsx file too. It should be a super quick fix (just updating everything to 'Subnet Label' in those tests, but lmk if you've questions!

Ah, thank you for the heads up! I'll take care of that soon. Shame on me for not considering the unit tests 😅

Edit: Done! Also replaced "Subnet label" with "Subnet Label" in the Subnet delete dialog. Thanks again for the catch, @coliu-akamai!

Copy link
Contributor

Choose a reason for hiding this comment

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

awesome thanks!

@coliu-akamai
Copy link
Contributor

@jdamore-linode Oh one quick thing, I saw that you changed the Subnet delete dialog to be 'Subnet Label' instead of 'Subnet label' too -- this will affect some of the cypress tests too 😅 (vpc-details-page.spec.ts, line 174, line 241 I think)

@jdamore-linode
Copy link
Contributor Author

@jdamore-linode Oh one quick thing, I saw that you changed the Subnet delete dialog to be 'Subnet Label' instead of 'Subnet label' too -- this will affect some of the cypress tests too 😅 (vpc-details-page.spec.ts, line 174, line 241 I think)

Classic. 😅 Thanks Connie!

@jdamore-linode
Copy link
Contributor Author

Going to merge this despite the E2E failure. The failing tests are caused by a gateway timeout when attempting to create LKE clusters, so it's not related to the work in this PR.

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! VPC Relating to VPC project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants