-
Notifications
You must be signed in to change notification settings - Fork 6
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
Setup Testing #92
Setup Testing #92
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felipecadavid +1 from me. @sashathor I am curious to get your thoughts on the approach. We're using it for our Console and pretty happy with it, although there's a bit of boilerplate. Main goal is to be able to mock actual GraphQL responses and test the implementation against those.
@@ -374,6 +374,7 @@ export function Leaderboard(props: LeaderboardProps) { | |||
height={styles?.canvas?.height || defaultChartHeight} | |||
role="img" | |||
style={loadingStyles} | |||
data-testid="chart-canvas" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary? Can we not use some other selector than data-testid
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about using role
since it's the only property we can access I was just a little bit concerned that in a future we add another element with role="img"
.
But looking at the testing library suggestions about this I think we'll be fine, since they recommend using this as first option and if it's necessary you can pass it a name
as second argument getByRole('button', {name: /submit/i})
. Let me change it.
return res( | ||
ctx.data({ | ||
counter | ||
}) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍 Since we are here, how about expanding this to show
- Returning a value.
- Returning
null
due to lack of data (this will happen for AVERAGE, MIN, MAX, etc.). - Returning
null
due to error (this could happen due to timeout, permissions, etc.).
And going ahead and getting the tests for the second two into the <Counter> component's tests? I'm assuming they will just pass but, if they don't we can skip them and address in a followup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! I just added tests for those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done @felipecadavid 👍
@markandrus I think it's a good starting point.
I have a few comments regarding general architecture.
From my experience with a proper level of components abstraction, there is no need to have a separate folder for tests, e.g. the following works great in most cases:
Counter/
Counter.tsx
Counter.tests.ts
...
Also, for the sake of readability, it is good to keep things together when it's possible and reduce redundant levels of abstractions, e.g. if the mock data chunk is small it is fine to keep it inside the tests file. The same goes for types
, styles
, etc. Of course, some care is needed when it goes to dependency cycles – in this case, we may need to add the abstraction.
I'm a bit concerned by the amount of duplicate code but with the united ui-kit
architecture (I'm working on it) we will be able to improve it.
🚢 it!
Description of changes
This PR sets up the testing environment with MSW for TimeSeries, Leaderboard and Counter components
Checklist
Before merging to main:
Manually tested in React appsRelease notes