chore: add unit tests #13
Conversation
mrderyk
force-pushed
the
chore/add_unit_tests
branch
6 times, most recently
from
ab0ba0f to
1f59564
Compare
mrderyk
force-pushed
the
chore/add_unit_tests
branch
from
1f59564 to
e01704b
Compare
mrderyk
force-pushed
the
chore/add_unit_tests
branch
2 times, most recently
from
7b01533 to
e01704b
Compare
There was a problem hiding this comment.
Thanks for adding these! It's great to have test coverage.
I had a few comments about test design, none of which are blockers. I think the general test design of testing low-level behavior on the dependencies and then asserting the ReactSearch component has abstract dependencies on the other components introduces some maintainability issues. Just to throw it out there, I think it'd be more maintainable for us to essentially combine all of the tests you've created into a suite of tests on just the ReactSearch component. Happy to discuss this at some point!
| .getByText(MOCK_SEARCH_RESULT.snippet.text) | ||
| .closest("a"); | ||
|
|
||
| expect(anchor).toHaveAttribute("href", MOCK_SEARCH_RESULT.url); |
There was a problem hiding this comment.
Nit: Generally I'd prefer to see as few assertions per test as possible and, if possible, no duplicate assertions. This assertion duplicates the one on 77. So if for some reason it fails, it will take a moment for the developer to understand that one of the failures is noise. Not a blocker, just food for thought.
There was a problem hiding this comment.
I would say that there's value in duplicate assertions if the context is sufficiently different. In this case, there's a different prop being passed to the component. A different prop almost always results in branching logic.
So we verify here that we still get the expected result even when given a different prop plus potential for branching logic.
| ); | ||
| let anchor = screen | ||
| .getByText(MOCK_SEARCH_RESULT.snippet.text) | ||
| .closest("a"); |
There was a problem hiding this comment.
I've found that traversing the DOM creates a brittle and artificial coupling with DOM structure. The solution I found that has worked well is to tag elements with data-testid attributes and then target them specifically.
| import { render, fireEvent, act, screen } from "@testing-library/react"; | ||
| import { ReactSearch } from "."; | ||
| import * as SearchInput from "./SearchInput"; | ||
| import * as SearchResult from "./SearchResult"; |
There was a problem hiding this comment.
Nit: We can make our interfaces more predictable by consistently preferring named exports over default exports. E.g. replacing this with:
import { SearchInput } from "./SearchInput";
import { SearchResult } from "./SearchResult";There was a problem hiding this comment.
Oh this is only for the test, in order to be able to use jest's spyOn syntax:
// Where object is the star import, and method is the named export (i.e. SearchInput)
jest.spyOn(<object>, <method>);
If you know of a better way to do this, would definitely love to know more!
| }); | ||
| }); | ||
|
|
||
| const renderSearch = () => { |
There was a problem hiding this comment.
Can we move this to the top of the file? As I was reading the code I got confused by the references to renderSearch. Having this context at the top would have helped me.
|
|
||
| renderSearch(); | ||
|
|
||
| expect(searchInputSpy).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
We're not really testing that ReactSearch renders SearchInput... just that it calls the SearchInput function. What do we really care about, though? It seems to me that we should really care about the behavior of RenderSearch. E.g. interacting with the search input and viewing the search results. Right?
There was a problem hiding this comment.
As we use functional components, checking that the function is called should be good enough to see that verify that it is rendered.
And then, because we test the SearchInput and SearchResult components separately, at this higher level we need only test that the functional components are called with the right props.
This is because:
- is composed of and instances of (which IMO now should probably be a component that accepts a selectedIndex prop
- We verify the behavior of through its own unit test
- We also verify the behavior of through its own unit test
I will agree however that this is a special case, since the navigation with up/down arrows is ultimately triggered by the input and is visually represented in search results. I can definitely follow up with a test for that.
IMO this is the tricky thing with unit tests, i.e. how far do you go without venturing into the area of integration tests. How far down the component hierarchy do you render, and at what point do you mock?
In my experience, what I've found effective is to approach it from the perspective of: "What if these child components were imported from an external library like Chakra or Mantine?" In most cases, the unit tests would just verify that the parent component renders them with the right props.
CONTEXT
As we maintain this repository, we'll need tests to help ensure we don't introduce bugs.
NOTE: This starts coverage out at around 75%. If we choose to, we can gate merging behind a coverage threshold in the future- around 80-85% would be a good balance between coverage and not being too strict. But for now, we can simply use coverage as a guide to see if we're starting to have major gaps.
CHANGES
SCREENSHOTS