-
Notifications
You must be signed in to change notification settings - Fork 25
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
chore: add tests for react-* headless components #347
Conversation
Basic testing to verify the components in `Authenticator` call the expected API functions from the Keyring provider.
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit b0bbb59:
|
beforeEach(() => { | ||
// setup a DOM element as a render target | ||
container = document.createElement('div') | ||
document.body.appendChild(container) | ||
}) | ||
|
||
afterEach(() => { | ||
// cleanup on exiting | ||
unmountComponentAtNode(container) | ||
container.remove() | ||
}) | ||
|
||
test('CancelButton', async () => { | ||
const user = userEvent.setup() | ||
|
||
const cancelRegisterSpace = vi.fn() | ||
const contextValue: KeyringContextValue = [ | ||
keyringContextDefaultValue[0], | ||
{ ...keyringContextDefaultValue[1], cancelRegisterSpace } | ||
] | ||
act(() => { | ||
render(( |
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'm new to testing-library, but i like the look of it and its goals. From looking at some examples it looks like we should be able to avoid the beforeEach / afterEach boilerplate and maybe the act
step can go too? If we can get our tests looking more like this example that'd be rad https://github.com/kentcdodds/react-testing-library-course/blob/5abc1039d677767b8de5197c049bfd0428fa7420/src/__tests__/app-03.js#L12-L38
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.
a handy list of things to avoid https://kentcdodds.com/blog/common-mistakes-with-react-testing-library which can be used to extrapolate what should be possible
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 think it's a good direction! Is it possible to reduce the boilerplate?
@olizilla pointed me at https://kentcdodds.com/blog/common-mistakes-with-react-testing-library which helped me make this MUCH cleaner, woo hoo!
IT SURE IS - that blogpost is a gem - under its tutelage this is now much much cleaner |
I used these in earlier versions of these tests but don't now
added a test for the Uploader as well |
it would be nice to test that the uploads list renders something that makes sense, but that's going to require a bit of restructuring to let us mock out the `list` function
annnnd UploadsList |
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.
This is great!
We should do some learning around getByRole and use our tests to check that our components follow aria guidelines. It's not a replacement for a11y testing, but it's a baseline we can put in early to ensure we get the basics. No need to change anything in this PR, I'm just surfacing a thought that occurred as I read through.
Basic testing to verify the components in
Authenticator
,Uploader
, andUploadsList
call the expected API functions from their respective providers.