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.

Sign up for GitHub

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

Jump to bottom

test: [M3-8565] - Add unit tests for rest of NodeBalancers package #10945

Merged
merged 9 commits into from
Sep 18, 2024
Merged

test: [M3-8565] - Add unit tests for rest of NodeBalancers package #10945

merged 9 commits into from
Sep 18, 2024

Conversation

coliu-akamai
Copy link
Contributor

@coliu-akamai coliu-akamai commented Sep 16, 2024 •
edited
Loading

Description 📝

  • Adds unit tests for the rest of NodeBalancers package to improve test coverage and confirm things render as expected

Target release date 🗓️

n/a

How to test 🧪

  • yarn test NodeBalancers - confirm all tests pass

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

@coliu-akamai coliu-akamai self-assigned this Sep 16, 2024
@coliu-akamai coliu-akamai added Ready for Review Unit tests Improves unit test coverage labels Sep 16, 2024
@coliu-akamai coliu-akamai marked this pull request as ready for review September 16, 2024 17:25
@coliu-akamai coliu-akamai requested a review from a team as a code owner September 16, 2024 17:25
@coliu-akamai coliu-akamai requested review from dwiley-akamai and cpathipa and removed request for a team September 16, 2024 17:25
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 16, 2024 •
edited
Loading

Coverage Report: ✅
Base Coverage: 86.64%
Current Coverage: 86.8%

dwiley-akamai
dwiley-akamai approved these changes Sep 16, 2024
View reviewed changes
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.

Tests passed locally & in CI ✅

  • there was a failure for nodebalancers/nodebalancers-create-in-complex-form.spec.ts -- worth looking into but it shouldn't have been caused by any of the changes in this PR

Screenshot 2024-09-16 at 1 59 14 PM

packages/manager/src/features/NodeBalancers/NodeBalancerConfigNode.test.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/NodeBalancers/NodeBalancerConfigPanel.test.tsx
@@ -28,7 +28,7 @@ const node: NodeBalancerConfigNodeFields = {
weight: 100,
};

const props: NodeBalancerConfigPanelProps = {
export const nbConfigPanelMockPropsForTest: NodeBalancerConfigPanelProps = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to not stick with props for the variable name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I exported the props to use in NodeBalancerPassiveCheck.test.tsx - thought it might be helpful to mirror how we update the names of interfaces for component props to be more specific when exporting them, and avoid something like import { props } from file.

Can definitely change back to props though!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh gotcha, in that case I agree with how you handled it originally here 👍🏾

packages/manager/src/features/NodeBalancers/NodeBalancerCreate.test.tsx Outdated Show resolved Hide resolved
coliu-akamai and others added 2 commits September 16, 2024 15:19
@coliu-akamai @dwiley-akamai
Update packages/manager/src/features/NodeBalancers/NodeBalancerConfig…
e6e2833 
…Node.test.tsx

Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
@coliu-akamai @dwiley-akamai
Update packages/manager/src/features/NodeBalancers/NodeBalancerCreate…
0b047fb 
….test.tsx

Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
@coliu-akamai coliu-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Sep 16, 2024
@coliu-akamai
Copy link
Contributor Author

coliu-akamai commented Sep 16, 2024 •
edited
Loading

This is weird... nodebalancers/nodebalancers-create-in-complex-form.spec.ts passes locally when I use yarn cy:debug, but not when I run it directly... gonna look into this a bit more!

probably something to do with this privacy policy/EU check - maybe sometimes a region from the EU is chosen which triggers this check? cc @jdamore-linode
image (sorry for the blurry photo, just screenshotted it from the outputted video)

cpathipa
cpathipa approved these changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@cpathipa cpathipa left a comment

Choose a reason for hiding this comment

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

@coliu-akamai Thank you for adding testing coverage for NodeBalancers.

@cpathipa cpathipa added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Sep 17, 2024
@coliu-akamai
Copy link
Contributor Author

created M3-8600 to look into the test flake for nodebalancers/nodebalancers-create-in-complex-form.spec.ts some more, separate from this pr. Will be merging this one in

@coliu-akamai coliu-akamai merged commit 75c5986 into linode:develop Sep 18, 2024
18 of 20 checks passed
This was referenced Sep 25, 2024
Merged
Merged
@coliu-akamai coliu-akamai deleted the m3-8565 branch October 9, 2024 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dwiley-akamai dwiley-akamai dwiley-akamai approved these changes

@cpathipa cpathipa cpathipa approved these changes

Assignees

@coliu-akamai coliu-akamai

Labels
Approved Multiple approvals and ready to merge! Unit tests Improves unit test coverage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@coliu-akamai @dwiley-akamai @cpathipa