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-998] Increase Test Coverage in peregrine/lib/talons/SignIn #2998

Merged
merged 2 commits into from
Feb 10, 2021

Conversation

tjwiebell
Copy link
Contributor

@tjwiebell tjwiebell commented Feb 8, 2021

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%

New Library Added

This PR introduces two changes to how we approach unit testing of talons

  • Introduction of @testing-library/react-hooks - gives us a consistent pattern to align towards when testing talons and custom hooks. Seems proven enough that it's safe to introduce, at least in scope of peregrine. Their docs have lots of good information on usage, but the big patterns are all covered in this PR.
  • Usage of Apollo's MockProvider that simulates actual operations, instead of us mocking those hooks. The only odd piece here is that operations wait for the next tick of the event loop to flush, but RLRH simplifies this by just awaiting those triggers if you're not concerned with loading state. Their docs also do a great job at explaining usage, but the major patterns are covered in this PR.

TODO?

I haven't made any of what I've done in this PR a utility just yet. I'm thinking the whole team should iterate on these new patterns a bit before deciding if usage is consistent enough to add utilities. Advice from the maintainers to handle complex context nesting in the app is to add a test/test-utils utility that export a renderHook method that does this automatically, eg. similar to what renderHookWithProviders does here, but every context so we no longer need to mock useCartContext or useUserContext. This could prove to be difficult with redux still hanging around, but is the ideal solution going forward.

Related Issue

  • [PWA-998] Increase Test Coverage in peregrine/lib/talons/SignIn

Acceptance

Verification Stakeholders

Specification

Verification Steps

  1. Look at coveralls report, verify useSignIn is now covered

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.

- Cover useSignIn talon with tests using new utility
Comment on lines +115 to +116
expect(getCartDetails).toHaveBeenCalled();
expect(getUserDetails).toHaveBeenCalled();
Copy link
Contributor Author

@tjwiebell tjwiebell Feb 8, 2021

Choose a reason for hiding this comment

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

I would like to avoid implementation details from bleeding into tests, but included these two checks since we're mocking both of these contexts. Ideally we wouldn't mock these contexts and include them in the helper renderHookWithProviders, but that proved to be a bit difficult with redux in place. I hope once redux is deprecated from these contexts, this will be easier going forward, and we can assert on context values, instead of checking that methods were just called.

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Feb 8, 2021

Fails
🚫 A version label is required. A maintainer must add one.
Warnings
⚠️ Found the word "TODO" in the PR description. Just letting you know incase you forgot :)
Messages
📖

Associated JIRA tickets: PWA-998.

📖 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 2e31386

Copy link
Member

@larsroettig larsroettig left a comment

Choose a reason for hiding this comment

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

Looks clean ❤️

test('mutation error is returned by talon', async () => {
const signInErrorMock = {
request: signInMock.request,
error: new Error('Uh oh! There was an error signing in :(')
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we seriously have a sad smiley in the error message 🤣 ?

Copy link
Member

Choose a reason for hiding this comment

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

He why not is funny 😸

Copy link
Contributor

@revanth0212 revanth0212 left a comment

Choose a reason for hiding this comment

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

Wonderful work @tjwiebell.

@supernova-at supernova-at added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Feb 10, 2021
@supernova-at supernova-at merged commit 8bcebed into develop Feb 10, 2021
@supernova-at supernova-at deleted the tommy/sign-in-tests branch February 10, 2021 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:peregrine 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.

6 participants