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-7768] - Add VM Placement Group landing page empty state test #10350

Conversation

jdamore-linode
Copy link
Contributor

Description πŸ“

Adds a quick integration test to confirm the VM Placement Groups landing page empty state. The real purpose of this PR is to provide a bare minimum example showing how to mock one of our new object-based feature flags, but it's not much different from mocking our boolean flags.

Changes πŸ”„

  • Add integration test to confirm VMPG landing page empty state
  • Adds a mock util for Placement Group fetching

How to test πŸ§ͺ

yarn cy:run -s "cypress/e2e/core/vmPlacement/vm-placement-landing-page.spec.ts"

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

Comment on lines 12 to 18
mockAppendFeatureFlags({
placementGroups: makeFeatureFlagData({
beta: true,
enabled: true,
}),
});
mockGetFeatureFlagClientstream();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only real difference here is we're passing an object in the same shape as the feature flag rather than a true or false (however, there's no type safety here that'll catch us if we make a mistake).

@jdamore-linode jdamore-linode requested a review from a team as a code owner April 3, 2024 21:55
@jdamore-linode jdamore-linode requested review from bnussman-akamai and jaalah-akamai and removed request for a team April 3, 2024 21:55
Copy link

github-actions bot commented Apr 3, 2024

Coverage Report: βœ…
Base Coverage: 81.79%
Current Coverage: 81.79%

Copy link
Contributor

@abailly-akamai abailly-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 great! thanks for making this one, looking forward to writing more

// Mock the VM Placement Groups feature flag to be enabled for each test in this block.
beforeEach(() => {
mockAppendFeatureFlags({
placementGroups: makeFeatureFlagData<Flags['placementGroups']>({
Copy link
Contributor

Choose a reason for hiding this comment

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

@jdamore-linode since it's a generic we can add type safety here. We'll just have to remember to do it and catch it at code reviews if not present.

Pushed a fix for this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Beautiful, wasn't aware of that Flags type. Thanks @abailly-akamai!

By the way, do you know anything about the vmPlacement LD flag? Was that a holdover from earlier in the project that isn't going to be used?

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, lemme delete it now so there's no confusion anymore

@carrillo-erik
Copy link
Contributor

carrillo-erik commented Apr 4, 2024

@jdamore-linode Looks good and thanks for getting this started. My only reservation is whether the folder and spect should be renamed to placementGroups/ and placement-group-landing-page.spec.ts. This change would reflect the feature and component names in the rest of the codebase.

@jdamore-linode
Copy link
Contributor Author

My only reservation is whether the folder and spect should be renamed to placementGroups/ and placement-group-landing-page.spec.ts. This change would reflect the feature and component names in the rest of the codebase.

Great callout, thanks @carrillo-erik! If that's more consistent and makes more sense then I absolutely agree. I'll try to take care of that before I sign out tonight!

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! Placement Groups and removed Ready for Review labels Apr 8, 2024
@abailly-akamai abailly-akamai force-pushed the M3-7768-vm-placement-landing-page-empty branch from 1e5690f to 49d2653 Compare April 11, 2024 18:55
@abailly-akamai abailly-akamai merged commit 26c3f9d into linode:develop Apr 11, 2024
17 of 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! Placement Groups
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants