Skip to content

Conversation

@christopher-stripe
Copy link
Contributor

#93 adds support for Strict Mode, but we don't have any way to make sure we don't regress this change since Enzyme doesn't support Strict Mode. This PR migrates our usage of Enzyme to React Testing Library, which does support Strict Mode, so that we can confidently merge #93.

A few notes about the migration:

  • I opted to test hooks with react-hooks-testing-library where it seemed reasonable
  • I converted the tests from JavaScript to TypeScript since it was an easy change to make concurrently, and because I find the type system to be slightly more helpful than irritating in test code. But I am also peaceful keeping tests as JS if you have strong opinions about this
  • I also added a few failing tests for Strict Mode, which are currently skipped

);
});

test('provides elements and stripe with the ElementsConsumer component in Strict Mode', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't actually test strict mode 🤦‍♀️

Copy link
Contributor

@dweedon-stripe dweedon-stripe left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

const {result, rerender} = renderHook(() => useElements(), {wrapper});
expect(result.current).toBe(null);

stripeProp = mockStripe;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I get how this works, but it so badly breaks the React model that if feels like it should not. Can this be a bit more idiomatic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, does feel a bit weird (though it is what's prescribed in the React Hooks Testing Library). Because you expect that props should not be mutated outside of the React lifecycle, right?

Would this be preferable?

  test('allows a transition from null to a valid Stripe object', () => {
    const TestComponent = () => {
      const elements = useElements();

      if (!elements) {
        return null;
      }

      if (elements === mockElements) {
        return <>expected</>;
      }

      throw new Error(`unexpected useElements value: ${elements}`);
    };

    const {container, rerender} = render(
      <Elements stripe={null}>
        <TestComponent />
      </Elements>
    );

    expect(container).toBeEmpty();

    rerender(
      <Elements stripe={mockStripe}>
        <TestComponent />
      </Elements>
    );

    expect(container).toHaveTextContent('expected');
  });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this and the next, lemme know what you think.

);
expect(result.current).toBe(null);

stripeProp = mockStripePromise;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here.

let stripeProp: any = null;
const wrapper = ({children}: any) => (
<Elements stripe={stripeProp}>{children}</Elements>
const TestComponent = () => {
Copy link
Contributor

@dweedon-stripe dweedon-stripe Jun 1, 2020

Choose a reason for hiding this comment

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

This actually seems overly round-about. The way you had it before is fine once I get my head around it. Especially if the old way is the blessed way of testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I'll leave it the original way then

@christopher-stripe christopher-stripe merged commit 39e8c36 into master Jun 1, 2020
@christopher-stripe christopher-stripe deleted the christopher/migrate-to-rtl branch June 1, 2020 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants