test: [M3-7093] - Add integration tests for "Enable Linode Backups" notice, DC-specific pricing

[jdamore-linode]

Description 📝

Adds a few integration tests to confirm that the "Enable Linode Backups" notice and drawer both behave as expected, and refactors existing backup tests to improve performance. Additionally, makes some small additions in src to add ARIA labels to some elements.

Major Changes 🔄

• Added integration tests for "Enable Linode Backups" notice and drawer
  • Confirms that the "Enable Linode Backups" appears when expected and is hidden when expected
  • Confirms that the "Enable Linode Backups" notice can be dismissed
  • Confirms that the "Enable Linode Backups" drawer does not show Linode regions or DC-specific pricing information when DC-specific pricing is disabled
  • Confirms that the "Enable Linode Backups" drawer shows Linode regions and expected DC-specific pricing information when DC-specific pricing is enabled
• Refactor existing backup tests to improve performance
  • Tests no longer wait for the Linodes to boot, only provision
  • Renamed snapshot test to better reflect what the test actually accomplishes
• Added a couple aria-label attributes in src
  • Added aria-label="Dismiss Enable Linode Backups notice" to the Enable Linode Backups notice dismissal button, which previously had no label and was announced by VoiceOver as "button"
  • Added aria-label="List of Linodes" to the table containing Linodes in the backup drawer

How to test 🧪

We can rely on the automated tests to confirm that the test suite runs as expected. To run the backup tests locally, you can run yarn && yarn build && yarn start:manager:ci, and then:

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

[jdamore-linode]

I followed the pattern set by the Linode Landing Page table, which also has aria-label="List of Linodes", but was curious if something more specific here would be better since this list only contains Linodes which do not have backups enabled.

[coliu-akamai]

Hm...imo a more specific label wouldn't hurt 👀

[jdamore-linode]

Any ideas? My initial thought was something extremely literal, like List of Linodes without backups enabled, but I opted against that since it's kind of wordy (but maybe that's ok?).

Maybe List of Linodes to back up or List of Linodes without backups? Any thoughts, @mjac0bs?

[cpathipa]

@jdamore-linode Good call! I think aria-label is a suitable approach for providing providing context for elements like tables when there isn’t a more semantic way to convey the the tables purpose to screen readers. However, we could use other HTML elements like h2 to convey the same information in a more semantic way.

I mean, some of our tables have heading we could use
<h2> List of Linodes </h2> to provide the title, making it more semantically meaningful. Additionally you can use aria-label to provide supplementary information / override the default labeling.

Basically h2 provides title of the table and aria-label provides additional context of the table’s content.

[mjac0bs]

I think including a title for the table here could help clarify what this list is to everyone, but it would be a little awkward because this is an awkward list of linodes to name! 😅 I was also considering whether another heading would take away user focus from other important in the drawer (e.g. the total price). We could opt to make it an h3, since the drawer title and total price are already h2s.

If we just stick with an aria-label, I think List of Linodes without backups is best.

If we wanted to go the table title route, Linodes without backups?

[cpathipa]

LGTM! verified Linode's backup specs in local.

[coliu-akamai]

Hm.. I might be missing something but three of the tests failed on me for local (yet I can see them passing in the github checks so maybe I've configured something incorrectly?)

I checked out your branch/pulled to make sure things were up to date, then ran yarn && yarn build && yarn start:manager:ci. On a different terminal, I ran yarn cy:run -s "cypress/e2e/core/linodes/backup-linode.spec.ts":

[coliu-akamai]

Hm...imo a more specific label wouldn't hurt 👀

[jdamore-linode]

Hm.. I might be missing something but three of the tests failed on me for local (yet I can see them passing in the github checks so maybe I've configured something incorrectly?)

I checked out your branch/pulled to make sure things were up to date, then ran yarn && yarn build && yarn start:manager:ci. On a different terminal, I ran yarn cy:run -s "cypress/e2e/core/linodes/backup-linode.spec.ts":

(screenshot 🖼️)

Thanks Connie!

My guess is it could be something account-specific which I haven't taken into account. I'll take a closer look soon and might reach out if I have any questions while I try to reproduce!

(Just off the top of my head -- is there any chance you have Managed enabled on your test account?)

[coliu-akamai]

👀 I do have managed enabled, yep! Could that be why?

[jdamore-linode]

👀 I do have managed enabled, yep! Could that be why?

Ah yep, that explains all 3! Backups get enabled implicitly with Managed, so Cloud removes all the notices and adjusts the UI to account for that.

I can fix the integration tests easily by mocking the account so that Managed is disabled (I should've been doing that from the start, so I'm glad you caught this 👨‍🔬). The end-to-end test is a bit trickier...

We actually currently check for Managed in the test, and only proceed with the test interactions if Managed is disabled, but I removed that intentionally as part of this PR. My thought was that if an automated test account gets enrolled in Managed mistakenly, I want the test to fail so we have visibility into the issue, and I definitely don't want the test to pass when the backup functionality isn't actually being covered.

I'll spend some time thinking about how best to address this today! Thanks again for catching this, Connie!

[mjac0bs]

Confirmed the test passes locally and verified that we're testing the necessary DC-pricing changes. ✅ Aside from a decision on the table aria-label, this looks ready to go!

Oh wait, one question: the Add Ons section and summary at the bottom of Linode Create also contains backup pricing and will update dynamically -- is that something that would be easier to test elsewhere, like #9701?

[mjac0bs]

I think including a title for the table here could help clarify what this list is to everyone, but it would be a little awkward because this is an awkward list of linodes to name! 😅 I was also considering whether another heading would take away user focus from other important in the drawer (e.g. the total price). We could opt to make it an h3, since the drawer title and total price are already h2s.

If we just stick with an aria-label, I think List of Linodes without backups is best.

If we wanted to go the table title route, Linodes without backups?

[jdamore-linode]

Oh wait, one question: the Add Ons section and summary at the bottom of Linode Create also contains backup pricing and will update dynamically -- is that something that would be easier to test elsewhere, like #9701?

@mjac0bs That's a good catch! This should definitely be covered, and I think covering it as part of the Linode Create tests would probably make more sense. Do you think we could spin it off into a separate ticket so as to not increase the scope of #9701, or would you prefer to try squeezing it in there?

(In either case, I think Cassie's work in #9701 gets us about 90% of the way there -- I think we could add a few lines towards the bottom of her shows DC-specific pricing information during create flow test to click "Enable Backups" and confirm that the price gets displayed as expected. CC @cliu-akamai)

[mjac0bs]

Do you think we could spin it off into a separate ticket so as to not increase the scope of #9701, or would you prefer to try squeezing it in there?

(In either case, I think Cassie's work in #9701 gets us about 90% of the way there -- I think we could add a few lines towards the bottom of her shows DC-specific pricing information during create flow test to click "Enable Backups" and confirm that the price gets displayed as expected. CC @cliu-akamai)

Up to Cassie, I think! @cliu-akamai - if you're willing to add that into your open PR and don't think it will be a time consuming addition (which it doesn't sound like it is), then great, let's add it there. If you would rather get your PR merged as is and not increase scope, then a follow up is fine.

[mjac0bs]

Approving this PR pending a decision is made on how to follow up on the backups in the Linode Create flow. In either case, I agree that it would fit better either as a separate ticket or as an add on to the existing Create flow PR than this ticket.

[jdamore-linode]

Approving this PR pending a decision is made on how to follow up on the backups in the Linode Create flow. In either case, I agree that it would fit better either as a separate ticket or as an add on to the existing Create flow PR than this ticket.

Thanks @mjac0bs! I'l write up a ticket to add some assertions for backup prices in the Linode Create flow just so this doesn't get lost.

In the interest of getting this merged, for now I'm going to let the Managed-specific failures go, but I did write up M3-7183 to address this (cc @coliu-akamai) so that we can run the tests locally with Managed enabled, not get failures, but allow failures to occur in CI where Managed should not be enabled.

[mjac0bs]

@jdamore-linode Are we all set to merge here? Thanks for the follow up tickets to tie up loose ends. 🙌🏼 The one CI e2e failure looks like linodes/linode-storage.spec.ts, which passes locally.

[jdamore-linode]

@jdamore-linode Are we all set to merge here? Thanks for the follow up tickets to tie up loose ends. 🙌🏼 The one CI e2e failure looks like linodes/linode-storage.spec.ts, which passes locally.

Sorry @mjac0bs, I think we are! I was trying to get the green check to merge this a few days ago and got sidetracked, but I'll merge this now. Thanks for taking a look at that most recent run!