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-1002] Increase Test Coverage in venia-ui/lib/components/CheckoutPage #3018

Merged
merged 4 commits into from
Feb 22, 2021

Conversation

tjwiebell
Copy link
Contributor

Description

As part of our initiative to increase test coverage, stability and confidence in our project we've identified this folder as having a lower than acceptable level of test coverage.

Acceptance Criteria:

Increase test coverage to an acceptable level, ideally 100%

Implementation Note

There was a component in here brainTreeDropIn that I didn't feel comfortable trying to cover. It relied on a lot of waterfall async work to render that wasn't straight forward, so might be better to tap on the original develop for covering that file. Could also be an opportunity to re-visit and see if patterns we've started coalescing on could be added to this component to help simplify.

Related Issue

  • [PWA-1002] Increase Test Coverage in venia-ui/lib/components/CheckoutPage

Acceptance

Verification Stakeholders

Specification

Verification Steps

  1. Run tests locally or look at Coveralls reports
  2. Verify that CheckoutPage, OrderSummary, and PriceAdjustments coverage improved

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.

@@ -0,0 +1,12 @@
import React from 'react';

const MockAccordion = jest.fn(({ children, ...props }) => (
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 did this after discussions from peer review; that our components would benefit from more manual mocks. Would like to discuss this further, but provided manual mocks like these is pretty simple, but not necessary.

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 may have stumbled onto why we haven't embraced this more. It seems some very old Jest bug causes these duplicate mock warnings, because jest-haste-map stores these mocks by name and not by path. Given we're a project where we use index files to export public API, I can see how this warning may have scared us away from manual mocks, since all of these module names would be index.

I'm digging for a solution, but nothing seems to be working. As you can see, the tests function fine, there's just this very in your face warning you get the first time you run the suite. We could mock underlying components instead of index exports, but expect we'd run into a similar issue eventually.

Original Bug: jestjs/jest#2070
The warning:

jest-haste-map: duplicate manual mock found: index
  The following files share their name; please delete one of them:
    * <rootDir>/packages/peregrine/lib/RestApi/__mocks__/index.js
    * <rootDir>/packages/venia-ui/lib/components/Accordion/__mocks__/index.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went ahead and just pulled this out, can disregard.

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Feb 18, 2021

Messages
📖

Associated JIRA tickets: PWA-1002.

📖 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 4a80a7c

@sirugh
Copy link
Contributor

sirugh commented Feb 19, 2021

I haven't looked, but I really hope there aren't too many conflicts with #2969

@tjwiebell tjwiebell added the version: Patch This changeset includes backwards compatible bug fixes. label Feb 22, 2021
@dpatil-magento dpatil-magento merged commit 012f2db into develop Feb 22, 2021
@dpatil-magento dpatil-magento deleted the tommy/tests-for-checkout branch February 22, 2021 22:01
@m2-community-project m2-community-project bot moved this from Reviewer Approved to Done in Pull Request Progress Feb 22, 2021
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: Patch This changeset includes backwards compatible bug fixes.
Development

Successfully merging this pull request may close these issues.

None yet

4 participants