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

PWA-1005: Increase Test Coverage in venia-ui/lib/components/Main #3004

Merged
merged 3 commits into from
Feb 11, 2021

Conversation

anthoula
Copy link
Contributor

@anthoula anthoula commented Feb 9, 2021

Description

Increase test coverage for the Main component.

Related Issue

https://jira.corp.magento.com/browse/PWA-1005

Acceptance

Any developer

Verification Stakeholders

Any developer

Specification

Verification Steps

  1. Run yarn test --collectCoverageFrom=**/main.js packages/venia-ui/lib/components/Main/
  2. Verify tests succeed
  3. Verify acceptable test coverage

Screenshots / Screen Captures (if appropriate)

Checklist

  • I have added tests to cover my changes, if necessary.
  • I have added translations for new strings, if necessary.
  • I have updated the documentation accordingly, if necessary.

- add tests to assert correct rendering of each use case
@anthoula anthoula added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Feb 9, 2021
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Feb 9, 2021

Messages
📖

Associated JIRA tickets: PWA-1005.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against 99593e3

Copy link
Contributor

@tjwiebell tjwiebell left a comment

Choose a reason for hiding this comment

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

Tiny suggestion for an additional test.


expect(root.findByProps({ className: 'root' })).toBeTruthy();
expect(root.findByProps({ className: 'page' })).toBeTruthy();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I know there's some debate about the value of snapshot tests, but I feel like we currently use them to detect breaking changes. I'm not locked into that mindset so I would consider either a snapshot test here, or a simple test to verify the children prop to rendered.

Copy link
Contributor

Choose a reason for hiding this comment

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

For what it's worth I suggested that @anthoula use these direct props assertions rather than snapshots. Your request about rendering the children is still valid though!

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Worth talking about at next peer review/team time then. Snapshot tests can serve a purpose, but we should probably not call them unit tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, originally had snapshot assertions, as that seems to be a current convention widely in use. Added an additional assertion for rendered children

@anthoula anthoula requested a review from tjwiebell February 10, 2021 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:venia-ui Progress: done version: Minor This changeset includes functionality added in a backwards compatible manner.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants