Skip to content

test: [M3-7068] - Add integration tests for Linode migration dialog DC-specific pricing#9681

Merged
cliu-akamai merged 1 commit intolinode:developfrom
cliu-akamai:feature/M3-7068
Sep 20, 2023
Merged

test: [M3-7068] - Add integration tests for Linode migration dialog DC-specific pricing#9681
cliu-akamai merged 1 commit intolinode:developfrom
cliu-akamai:feature/M3-7068

Conversation

@cliu-akamai
Copy link
Contributor

@cliu-akamai cliu-akamai commented Sep 14, 2023

Description 📝

Add new cypress tests for Linode migration dialog DC pricing

Major Changes 🔄

  • Add e2e tests to verify the DC pricing information in Linode migration dialog.

How to test 🧪

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

@mjac0bs mjac0bs changed the title test: [M3-7068] - Add Cypress integration tests for Linode migration dialog DC … test: [M3-7068] - Add integration tests for Linode migration dialog DC-specific pricing Sep 15, 2023
@mjac0bs mjac0bs self-requested a review September 15, 2023 16:10
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 adding! Test passes. ✅
image

I left a few comments about minor clean up on some comments.

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
* - Confirms that pricing comparison is not shown in "Region" section migration occurs in the same DC.
* - Confirms that pricing comparison is not shown in "Region" section if migration occurs in a DC with the same price structure.

Here, it is the same DC, because that's the mock data we have to work with, but for clarity, it's probably best to note that the price notice shouldn't show up if the region prices are the same -- even if they're different regions.

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
* - Confirms that pricing comparison is shown in "Region" section when migration occurs in different DCs.
* - Confirms that pricing comparison is shown in "Region" section when migration occurs in DCs with different price structures.

Similar comment as below.

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('shows DC-specific pricing information when migrating linodes between different DCs', () => {
it('shows DC-specific pricing information when migrating linodes between differently priced DCs', () => {

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
// TODO Remove feature flag mocks once DC pricing is live.
// TODO: DC Pricing - M3-7073: Remove feature flag mocks once DC pricing is live.

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
// TODO Remove feature flag mocks once DC pricing is live.
// TODO: DC Pricing - M3-7073: Remove feature flag mocks once DC pricing is live.

For clarity and ease of clean up later, let's format DC pricing related TODOs with TODO: DC Pricing. In this case, we have a related ticket number, so it's useful to include that too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we mean that this test just not exist once DC-pricing goes live? The other two test cases test that pricing information is shown and isn't shown depending on region prices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think so.

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.

Nice work @cliu-akamai! I agree with the clean up suggestions that Mariah made, and additionally have a few suggestions of my own.

It also seems like there may've been a misunderstanding on the requirements for one of the tests, so I posted a pair of comments (1, 2) that I hope will clear that up without sacrificing any of the coverage you've already added 👍

Comment on lines 188 to 194
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we define a helper in support/intercepts/linodes.ts named mockMigrateLinode() that does this?

Comment on lines 138 to 140
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
// Mock requests to get all Linode types, and to get individual types.
mockGetLinodeType(dcPricingMockLinodeTypes[0]).as('getLinodeTypeOld');
mockGetLinodeType(dcPricingMockLinodeTypes[1]).as('getLInodeTypeNew');
// Mock requests to get individual Linode types.
mockGetLinodeType(dcPricingMockLinodeTypes[0]);
mockGetLinodeType(dcPricingMockLinodeTypes[1]);

No need to assign an alias via .as("..."); here since we don't cy.wait() for it later 👍

Comment on lines 227 to 229
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
// Mock requests to get all Linode types, and to get individual types.
mockGetLinodeType(dcPricingMockLinodeTypes[0]).as('getLinodeTypeOld');
mockGetLinodeType(dcPricingMockLinodeTypes[1]).as('getLInodeTypeNew');
// Mock requests to get individual Linode types.
mockGetLinodeType(dcPricingMockLinodeTypes[0]);
mockGetLinodeType(dcPricingMockLinodeTypes[1]);

(Same as my other comment)

Comment on lines 205 to 206
Copy link
Contributor

Choose a reason for hiding this comment

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

Expanding on Mariah's suggestion, I think there might be a misunderstanding over the requirements for this one.

We want to test that the DC-specific pricing warnings do not appear if we're migrating from one DC to another DC which has the same pricing structure. Right now, the tests confirm that the user can't select the same region where the Linode is already hosted (which is good to have, too!)

We can extend this pretty easily to cover both cases, but we need to modify these two constants first so that 1) they're two different regions, and 2) they're not the same regions we mock in dcPricingMockLinodeTypes

Suggested change
const initialRegion = getRegionById('us-west');
const newRegion = getRegionById('us-west');
const initialRegion = getRegionById('us-southeast');
const newRegion = getRegionById('us-central');

Comment on lines 243 to 253
Copy link
Contributor

@jdamore-linode jdamore-linode Sep 18, 2023

Choose a reason for hiding this comment

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

Following up from my other comment, we can extend these assertions to ensure that the test covers both flows: that the user cannot select the same DC where the Linode is already hosted, and that when the user selects another DC with the same price structure, no DC-specific pricing information is shown.

Suggested change
containsClick(selectRegionString).type(`${newRegion.label}{enter}`);
cy.findByText('No results').should('be.visible');
// No DC pricing information shows up
cy.findByText(dcPricingCurrentPriceLabel).should('not.exist');
cy.get('[data-testid="current-price-panel"]').should('not.exist');
cy.findByText(dcPricingNewPriceLabel).should('not.exist');
cy.get('[data-testid="new-price-panel"]').should('not.exist');
// Confirm that user cannot select the Linode's current DC when migrating.
cy.findByText('New Region')
.click()
.type(`${initialRegion.label}{enter}`);
cy.findByText('No results').should('be.visible');
// Confirm that DC pricing information does not show up
cy.findByText(dcPricingCurrentPriceLabel).should('not.exist');
cy.get('[data-testid="current-price-panel"]').should('not.exist');
cy.findByText(dcPricingNewPriceLabel).should('not.exist');
cy.get('[data-testid="new-price-panel"]').should('not.exist');
// Change region selection to another region with the same price structure.
cy.findByText('New Region')
.click()
.type(`${newRegion.label}{enter}`);
// Confirm that DC pricing information still does not show up.
cy.findByText(dcPricingCurrentPriceLabel).should('not.exist');
cy.get('[data-testid="current-price-panel"]').should('not.exist');
cy.findByText(dcPricingNewPriceLabel).should('not.exist');
cy.get('[data-testid="new-price-panel"]').should('not.exist');
// Confirm that migration queue button is enabled.
ui.button
.findByTitle('Enter Migration Queue')
.should('be.visible')
.should('be.enabled');

@cliu-akamai cliu-akamai requested a review from a team as a code owner September 19, 2023 15:43
@cliu-akamai cliu-akamai requested review from bnussman-akamai and dwiley-akamai and removed request for a team September 19, 2023 15:43
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 for the updates @cliu-akamai, everything looks great!

@cliu-akamai cliu-akamai merged commit 677a17d into linode:develop Sep 20, 2023
@cliu-akamai cliu-akamai deleted the feature/M3-7068 branch September 20, 2023 00:17
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.

3 participants