Nodejs.dev: Add more unit tests for Release components

[Bhavin789]

Summary

Hi

In the components folder, we have unit tests for almost all the components except for these three:

1. Release Cards
2. Release Table
3. Release Toggle

Was wondering if we are planning to add unit tests for these in near future? If we haven't thought about it yet, can I try to look into the same and share some possible solutions on this thread or contribute? Would be happy to help :smile:

Do let me know, thank you!

[marcustisater]

@Bhavin789 Great catch! Yes, feel free to add unit tests for these components. I believe there have been recently added ones for ReleaseDownloads. Looking forward to seeing your changes 👍

[manishprivet]

I'm pretty sure we've snapshot tests for atleast the Release table, and probably other two too, as I had to change it when the release table was changed

[Bhavin789]

Oh, but I can't see them in their respective folders :confused:

@manishprivet Can you please confirm if they are a part of any other folder? Can you please also confirm they were Release prefixed components? I can see some recent changes done in the DownloadReleases and DownloadTable components and their respective tests but none in any of the above I mentioned.

Thanks!

[manishprivet]

@Bhavin789 I take back my comment 🤦🏻‍♂️ I mistakenly took Release Table as DownloadTable component

Yeah the tests are missing the components you mentioned

[Bhavin789]

No worries @manishprivet :sweat_smile: . Thanks for highlighting though and confirming, it may have led to redundancy :sweat_smile:

[sbielenica]

Guys, not long ago I introduced @testing-library/react to the project so that'd be great if you used that in your new tests - you have example on how to use it here: https://github.com/nodejs/nodejs.dev/pull/951.

Another thing is that it's awesome when tests do more than just compare snapshot so I encourage you to test component interactions and logic when that's possible! 😄

[Bhavin789]

Absolutely @sbielenica, in PR #975 , all the tests now use @testing-library/react (have gotten away with react-test-renderer ) along with an additional library @testing-library/jest-dom which just provides some additional assertion helpers to write better assertion logic.

For the point on writing tests beyond snapshots, totally agree! The PR includes a few of them where we have used waitFor to check for DOM changes and then re-assert. Would recheck on where more could be included.

Would love to hear more suggestions to be included in the PR itself and thanks for sharing your thoughts here. Looking forward to more feedbacks. Thanks a lot!

[sbielenica]

I left one suggestion in your PR 😄

And about new tests - remember that you can always create new pull requests. I think it'll be easier for maintainers to check and accept new tests when there's only a few files changed as opposed to many, many changes. Your pull request is already quite big (and that's of course understandable as you changed plenty of snapshots to new lib).

[Bhavin789]

Sure @sbielenica, will definitely look into the primer setup that you suggested and try to blend it. Thanks :smile:

About the big PR situation, I am myself struggling with the scroll on the page :sweat_smile: , can imagine how tough it would be for the maintainers :cry: . Maybe we can break down this whole thing into phases and create smaller PRs which can be reviewed well and with confidence.

We have around ~25 test files that need to be updated as per the new lib. Maybe we can break down into batches of 5 for a better review process. A suggestion to achieve this:

1. The first PR would include majorly the setup as you suggested and a change in only 1-2 files.
2. The subsequent PRs can include incremental changes
3. The last PR removes the react-test-renderer from the package.json completely and make @testing-library/react as the standard for all tests.

Please let me know your thoughts on this. Thanks! cc// @benhalverson @marcustisater