felquis / react-credit-cards-2

Beautiful credit cards for your payment forms - community edition ⚡️
https://ovvwzkzry9.codesandbox.io/
MIT License
62 stars 9 forks source link

Ported from enzyme to react-testing-library #18

Closed vomaksh closed 1 month ago

vomaksh commented 1 year ago

Details mentioned in #17

vomaksh commented 1 year ago

Sorry, diff of index.spec.js is a mess.

felquis commented 9 months ago

I'm reviewing your PR #18 @ginruh, thank you for sending and sorry for this 3 months delay.

PS: please let me know if you can do these changes, or if I should go over them by myself!

Your PR looks great, I think we should replace

expect(rccsCard).toHaveTextContent("valid thru");

with

expect(
      rccsCard.querySelector(".rccs__expiry__valid").textContent
    ).toStrictEqual("valid thru");

I think toHaveTextContent is too permissive, and toStrictEqual more strict, more similar to the strictness of the tests currently written.

I suggest you to go through each it() compare with the previews one, and check className by className it strictly render what's expected it to render.

Taking this one as example:

it should render the card front

BEFORE:

  it("should render the card front", () => {
    const front = wrapper.find(".rccs__card--front");

    expect(front.length).toBe(1);
    expect(front.find(".rccs__card__background").length).toBe(1);
    expect(front.find(".rccs__number").length).toBe(1);
    expect(front.find(".rccs__number").text()).toBe("•••• •••• •••• ••••");
    expect(front.find(".rccs__name").length).toBe(1);
    expect(front.find(".rccs__name").text()).toBe("YOUR NAME HERE");
    expect(front.find(".rccs__expiry").length).toBe(1);
    expect(front.find(".rccs__expiry__valid").text()).toBe("valid thru");
    expect(front.find(".rccs__expiry__value").text()).toBe("••/••");
    expect(front.find(".rccs__chip").length).toBe(1);
    expect(front.find(".rccs__issuer").length).toBe(1);
  });

AFTER:

it("should render the card front", () => {
    renderCreditCards();

    const rccs = screen.getByTestId("rccs");
    const rccsCard = within(rccs).getByTestId("rccs__card");

    expect(rccsCard).toHaveTextContent("•••• •••• •••• ••••");
    expect(rccsCard).toHaveTextContent("YOUR NAME HERE");
    expect(rccsCard).toHaveTextContent("valid thru");
    expect(rccsCard).toHaveTextContent("••/••");
  });

SUGGESTED REWRITTEN:

it("should render the card front", () => {
  renderCreditCards();

  const rccs = screen.getByTestId("rccs");
  const rccsCard = within(rccs).getByTestId("rccs__card");

  expect(rccsCard.querySelector(".rccs__number").textContent).toStrictEqual("•••• •••• •••• ••••");
  expect(rccsCard.querySelector(".rccs__name").textContent).toStrictEqual("YOUR NAME HERE");
  expect(rccsCard.querySelector(".rccs__expiry__valid").textContent).toStrictEqual("valid thru");
  expect(rccsCard.querySelector(".rccs__expiry__value").textContent).toStrictEqual("••/••");
});
vomaksh commented 9 months ago

Thanks @felquis for your feedback. I tried to fix the issues you mentioned.

In the above recommendation, toStrictEqual was asked to be used but I found it overkill for our usage since we are dealing with primitive types. For primitive types such as string, toBe is fine.

felquis commented 1 month ago

Thank you for all this quality contribution!