-
Notifications
You must be signed in to change notification settings - Fork 470
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
added fireEvent util #13
Conversation
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.
Wow! This is so great!
Looks like this is missing some coverage
If you could open coverage/lcov-report/index.html
locally and see what we're missing then add some tests to cover those that'd be super. Thank you!
src/__tests__/events.js
Outdated
describe('Clipboard Events', () => { | ||
;['copy', 'paste'].forEach(eventName => { | ||
it(`fires ${eventName}`, () => { | ||
const node = document.createElement('input') |
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 code is very similar throughout the test file. Could we extract it to a "testEvent" function?
function testEvent({eventName, elementType}) {
const node = document.createElement(elementType)
const spy = jest.fn()
node.addEventListener(eventName.toLowerCase(), spy)
fireEvent[eventName](node)
expect(spy).toHaveBeenCalledTimes(1)
}
Then most of the tests could be dramatically simplified to something like:
describe('Clipboard Events', () => {
;['copy', 'paste'].forEach(
eventName => {
it(`fires ${eventName}`, () => {
testEvent({eventName, elementType: 'input'})
})
},
)
})
Alternatively it may even not be a bad idea to abstract it further:
const eventTypes = [
{type: 'Clipboard', events: ['copy', 'paste'], elementType: 'input'},
{
type: 'Composition',
events: ['compositionEnd', 'compositionStart', 'compositionUpdate'],
elementType: 'input',
},
// etc...
]
eventTypes.forEach(({type, events, elementType}) => {
describe(`${type} Events`, () => {
events.forEach(eventName => {
it(`fires ${eventName}`, () => {
const node = document.createElement(elementType)
const spy = jest.fn()
node.addEventListener(eventName.toLowerCase(), spy)
fireEvent[eventName](node)
expect(spy).toHaveBeenCalledTimes(1)
})
})
})
})
I think doing this will make it easier to add cases and know which tests are for one-off situations 👍 It will also make this file shorter probably :)
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.
Whoops, meant that review to be "Request changes" 😅
src/__tests__/helpers/react.js
Outdated
@@ -0,0 +1,13 @@ | |||
import ReactDOM from 'react-dom' | |||
|
|||
const node = document.body.appendChild(document.createElement('div')) |
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.
Is this okay to be done in the module scope rather than in a function?
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'll move the document.body.appendChild
call to mount
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.
And also add the mirroring removeChild to unmount then?
src/events.js
Outdated
@@ -0,0 +1,352 @@ | |||
// fallback to Event |
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.
const Event = global.Event
missing?
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.
Also, we should probably do:
const g = typeof window === 'undefined' ? global : window
And then reference g
instead. That way this runs in the browser properly as well.
src/__tests__/events.js
Outdated
}) | ||
}) | ||
|
||
describe('React Events', () => { |
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.
May be we need to have these test cases in react-testing-library
? I'm just wondering because, anyway people are gonna use dom-testing-library
for several frameworks (angular
,preact
,vue
etc), do we have to add those frameworks testing here in future too?
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.
Also by moving it to react-testing-library
, I guess people have good chance of looking at these test cases and understanding how to test events with react
. Want to see, what others think..
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.
Also moving it to react-testing-library will move the respective package dependencies that don't make sense in just the dom-testing-library
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, also what we have created in src/__tests__/helpers/react.js
is already part of react-testing-library
(render
,unmount
).
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.
Personally I think it'd be better if we could avoid react in here and have a simple render method that creates DOM nodes manually. It doesn't need to be complicated.
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.
makes sense, i can open a PR to react-testing-library
as well. there are a couple events we'll need to fallback to triggering synthetic events for (change
, select
, mouseenter
, mouseleave
) so I'll add that logic there
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.
Perfect. Thanks a bunch @jomaxx. This is so great!
@jomaxx Could you please clarify about synthetic events? How does Cypress handles these events from within the browser? |
@sompylasar let's move discussion about synthetic events to testing-library/react-testing-library#48 |
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 awesome! Thanks so much for adding this!
Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the |
🎉 This PR is included in version 1.3.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
* Improving API's for testing. * Improving Apis * Adding all contributions * Fixing wrong url * Fixing review comments & making colorful assertions :) * Removing unwanted changes * Fixing review comments * removing unwanted comments * Adding test cases for the coverage * removing commented code and making few changes to the contribution file * Updating the readme * Making line break changes * Update README.md
What: added fireEvent utility
Why: #5
How: src/events.js
Checklist: