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

Ported from enzyme to react-testing-library #18

Merged
merged 7 commits into from
Aug 29, 2024

Conversation

vomaksh
Copy link

@vomaksh vomaksh commented Sep 27, 2023

Details mentioned in #17

@vomaksh
Copy link
Author

vomaksh commented Sep 27, 2023

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

@felquis
Copy link
Owner

felquis commented Jan 19, 2024

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("••/••");
  });
  • checks that it render a single .rccs__expiry__valid element, I don't think this is necessary and we can remove it as you did
  • check that .rccs__expiry__valid strictly render the correct prop, you applied expect(rccsCard).toHaveTextContent("valid thru"); I think it's too permissive, I'd rather ask you to replace it with something like this:
expect(
      rccsCard.querySelector(".rccs__expiry__valid").textContent
    ).toStrictEqual("valid thru");

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
Copy link
Author

vomaksh commented Jan 21, 2024

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 felquis merged commit b9036ba into felquis:master Aug 29, 2024
@felquis
Copy link
Owner

felquis commented Aug 29, 2024

Thank you for all this quality contribution!

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.

2 participants