aaronjohnweiss / ascndr

Climbing tracked.
Apache License 2.0
3 stars 0 forks source link

Add unit tests #106

Open agilaz opened 1 year ago

agilaz commented 1 year ago

WIP

ngattusohw commented 1 year ago

I havent worked much with snapshot testing, but imo I think snapshot testing is separate from unit testing, and just an extra layer of confidence. So I think this is a great first pass, but ideally when seeing if the app renders, you also have tests that check for each visible component for the user, such as visible text and links and images.

In terms of unit tests themselves, I try to live by these principles from react testing library (I have also used enzyme and this can be done with the enzyme lib as well, though I have a personal preference). Which means that you want to unit test your components as your user would see them. so if at all possible, try to query the components being rendered by their text, then next best is by the role of their tags, then classname, then data-id prop. It's good you're using mount instead of shallow render bc thats getting the closest you can to the actual dom with enzyme.

You cover a lot of use cases with this testing for data changing and then being reflected in the components from clicks and onload which is awesome, and in future prs I would add more visual confirmation that these clicks happened besides that firebase was called with xyz such as checking for the right href props in links - basically any user visible differences that clicking around cause, such as clicking the accordion arrow on the session page.

agilaz commented 1 year ago

Regarding snapshot testing I agree, it's really just smoke testing but it's decent "bang for your buck" type testing so I tend to favor it especially in a first pass.

As for those testing principles, that's definitely fair. I tend to get lazy with my selectors and setting some test id is the shortest way around that, but those can end up polluting the code a bit.

As far as assertions in the tests that mutate state, that's a good point; it should be fairly straightforward to check the dom in addition to checking firebase.