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-7315] - Add tests assgining VPCs during Linode creation flow #9939

Merged
merged 6 commits into from
Jan 29, 2024

Conversation

cliu-akamai
Copy link
Contributor

Description 📝

Add regression tests for assigning VPCs during Linode creation flow.

Major Changes 🔄

  • Add regression tests to assign a VPC when creating a Linode.

How to test 🧪

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

@cliu-akamai cliu-akamai requested a review from a team as a code owner November 28, 2023 16:57
@cliu-akamai cliu-akamai requested review from bnussman-akamai and tyler-akamai and removed request for a team November 28, 2023 16:57
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.

Thanks Cassie! Left a comment below. Could we also add a changeset for this PR?

getVisible('[data-testid="vpc-panel"]').within(() => {
containsVisible('Assign this Linode to an existing VPC.');
// VPC in different region should not be available.
containsVisible('VPC is not available in the selected region.');
Copy link
Contributor

Choose a reason for hiding this comment

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

This text shows up when the region we've selected doesn't support VPCs. Since we're testing assigning a Linode to a VPC during creation, can we mock a region that supports VPCs and confirms this text doesn't show up? Then, could we test the user flow for assigning a VPC? For a region that supports VPCs, it looks like this:

assign-vpc-flow.mov

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.

Great start @cliu-akamai! I left some feedback -- feel free to reach out if you have any questions!

before(() => {
cleanUp('linodes');
mockConfig.interfaces.splice(2, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we create the mockConfig object with the expected interfaces rather than modifying them after the fact?

Comment on lines 69 to 97
const region: Region = getRegionById('us-southeast');
const diskLabel: string = 'Debian 10 Disk';
const mockConfig: Config = linodeConfigFactory.build({
id: randomNumber(),
});
const mockDisks: Disk[] = [
{
id: 44311273,
status: 'ready',
label: diskLabel,
created: '2020-08-21T17:26:14',
updated: '2020-08-21T17:26:30',
filesystem: 'ext4',
size: 81408,
},
{
id: 44311274,
status: 'ready',
label: '512 MB Swap Image',
created: '2020-08-21T17:26:14',
updated: '2020-08-21T17:26:31',
filesystem: 'swap',
size: 512,
},
];
const mockVLANs: VLAN[] = VLANFactory.buildList(2);
const mockVPCs: VPC[] = vpcFactory.buildList(5, {
region: 'us-southeast',
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move these to the VPC assignment test since they're not shared by any of the other tests?

Comment on lines 418 to 493
// the "VPC" section is present, and the VPC in the same region of
// the linode can be selected.
getVisible('[data-testid="vpc-panel"]').within(() => {
containsVisible('Assign this Linode to an existing VPC.');
// VPC in different region should not be available.
containsVisible('VPC is not available in the selected region.');
});
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 we'll need to do more here -- it's good that we're confirming the state of the UI when VPC isn't available, but we primarily want this test to be able to select a VPC and proceed with the Linode creation.

I think this might require some changes and additions to the mocks, specifically:

  • mockGetVPCs should be passed an array of VPC objects which contains the mockVPC object you created -- this should allow the mock to appear in the list of VPCs when attempting to select it
  • We'll probably need to mock the regions request so that us-southeast has the VPCs capability (we do something similar in the VPC create test).

Comment on lines 402 to 407
mockGetVLANs(mockVLANs);
mockGetVPC(mockVPC).as('getVPC');
mockCreateLinode(mockLinode).as('linodeCreated');
mockGetLinodeConfigs(mockLinode.id, [mockConfig]).as('getLinodeConfigs');
mockGetLinodeDisks(mockLinode.id, mockDisks).as('getDisks');
mockGetLinodeVolumes(mockLinode.id, []).as('getVolumes');
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll probably want to move these calls before cy.visitWithLogin() -- putting them here usually works, but it can be flaky because it's possible for Cloud to make these HTTP requests before Cypress sets up the intercepts.

@cliu-akamai cliu-akamai requested a review from a team as a code owner December 5, 2023 22:21
@cliu-akamai cliu-akamai requested review from jdamore-linode and removed request for a team December 5, 2023 22:21
Copy link

github-actions bot commented Dec 5, 2023

Coverage Report:
Base Coverage: 80.18%
Current Coverage: 80.18%

@cliu-akamai cliu-akamai changed the title M3-7315 Add tests assgining VPCs during Linode creation flow test: [M3-7315] - Add tests assgining VPCs during Linode creation flow Dec 7, 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.

awesome, thanks Cassie!
✅ confirmed both tests work as expected

It may or may not be out of scope for this ticket - should we also have a test for creating a VPC in the Linode Create flow and assigning it to this Linode? I can create a new ticket for it if needed @jdamore-linode

@@ -321,4 +336,181 @@ describe('create linode', () => {
fbtVisible(linodeLabel);
cy.contains('RUNNING', { timeout: 300000 }).should('be.visible');
});

it('assigns a VPC not enabled in the region to the linode during create flow', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('assigns a VPC not enabled in the region to the linode during create flow', () => {
it('prevents a VPC from being assigned in a region that doesn't support VPCs during the Linode Create flow', () => {

containsVisible(
'Allow Linode to communicate in an isolated environment.'
);
// VPC in different region should not be available.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// VPC in different region should not be available.
// Helper text appears if VPC is not available in selected region

@coliu-akamai coliu-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Dec 20, 2023
@jaalah-akamai
Copy link
Contributor

I had 3 tests fail for me:
Screenshot 2024-01-17 at 10 14 22 AM

Copy link
Contributor

@hkhalil-akamai hkhalil-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 overall look good. Highlighting one small issues causing failures for me locally.

cy.contains('RUNNING', { timeout: 300000 }).should('be.visible');

fbtClick('Configurations');
cy.wait(['@getLinodeConfigs', '@getVPC', '@getDisks', '@getVolumes']);
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing this error repeatedly across runs -- I think it originates from this line.
Screenshot 2024-01-22 at 4 42 18 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be related to this 404? Do we need to add a mock for this route?
Screenshot 2024-01-22 at 4 44 34 PM

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 @hkhalil-akamai! Just pushed a couple commits that should resolve the merge conflict and fix this failure.

@cliu-akamai Nice work! Approved pending Cypress test results -- thanks for addressing the feedback and sorry for the slow follow up on my end!

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Jan 26, 2024
Copy link
Contributor

@hkhalil-akamai hkhalil-akamai 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!

@jdamore-linode jdamore-linode merged commit 7663ebd into linode:develop Jan 29, 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! Testing VPC Relating to VPC project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants