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: Update notification language in test to correspond with API change #9972

Conversation

jdamore-linode
Copy link
Contributor

Description 📝

Fixes the resize-linode.spec.ts test by updating the notice string to match the language used by the API since the release of unified migrations.

Changes 🔄

  • Replaces a notification string

How to test 🧪

We can rely on CI for this, but to run the test locally you can use yarn && yarn build && yarn start:manager:ci, and then:

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

(Only the first test is modified by this PR, and it's the only test that's failing, so you don't have to wait for the entire spec to run since these are some of our most time consuming tests.)

As an Author I have considered 🤔

Check all that apply

  • 👀 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

@jdamore-linode jdamore-linode self-assigned this Dec 6, 2023
@jdamore-linode jdamore-linode requested a review from a team as a code owner December 6, 2023 18:44
@jdamore-linode jdamore-linode requested review from cliu-akamai and removed request for a team December 6, 2023 18:44
@jdamore-linode jdamore-linode requested a review from a team as a code owner December 6, 2023 18:50
@jdamore-linode jdamore-linode requested review from bnussman-akamai and carrillo-erik and removed request for a team December 6, 2023 18:50
Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Looks good! That's pretty savage of the backend team to go for a lowercase l in linode.

Screenshot 2023-12-06 at 1 55 15 PM

@@ -38,7 +38,7 @@ describe('resize linode', () => {

// TODO: Unified Migration: [M3-7115] - Replace with copy from API '../notifications.py'
Copy link
Member

Choose a reason for hiding this comment

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

Not related to this, but I noticed we kind of leaked some API code info here. Not a huge deal because it's super basic, but I think we generally try to avoid this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good callout, thanks @bnussman-akamai -- I read these comments but didn't think twice about the code reference there. I know there's another reference to this code elsewhere in this spec, and it might be worthwhile doing a quick search to find any other API code references that might be lurking around.

Copy link

github-actions bot commented Dec 6, 2023

Coverage Report:
Base Coverage: 86.51%
Current Coverage: 86.51%

@jdamore-linode jdamore-linode added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Dec 6, 2023
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.

LGTM 🚢

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Dec 6, 2023
@jdamore-linode jdamore-linode merged commit 188cacf into linode:develop Dec 6, 2023
15 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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants