Refactor SyntheticClipboardEvent tests to only use the public API #11365
Conversation
| event.clipboardData = data; | ||
| } | ||
|
|
||
| element.dispatchEvent(event); |
There was a problem hiding this comment.
@gaearon Does it make any huge difference on our tests using the Event constructor rather than the old-fashioned initEvent way when we want to create events?
Actually, how is Fiber dealing with these two approaches today?
There was a problem hiding this comment.
I don't know if there's any difference. React just subscribes with addEventListener. If that works with both approaches in jsdom, let's use whichever is simpler.
There was a problem hiding this comment.
That was my first attempt, but I keep getting this error:
TypeError: Argument to dispatchEvent must be an Event
There was a problem hiding this comment.
@SadPandaBear Not sure if it's possible to use the Event constructor in this case, since I have to mock the clipboardData. Any ideas?
There was a problem hiding this comment.
I was thinking on doing something like:
var event = new ClipboardEvent(type, { bubbles: true });
if(data) event.clipboardData = data;
element.dispatchEvent(event);But unfortunately ClipboardEvent is not supported. 😢
| React = require('react'); | ||
| ReactDOM = require('react-dom'); | ||
|
|
||
| document = jsdom(''); |
There was a problem hiding this comment.
Why do we need this? Don't we already have a document global?
There was a problem hiding this comment.
To reset changes done on document.body, replaced it by document.body.innerHTML = ''.
| />, | ||
| container, | ||
| ); | ||
| document.body.appendChild(div); |
There was a problem hiding this comment.
Shouldn't we add the container to body instead? This detaches the div from the original container.
| React = require('react'); | ||
| ReactDOM = require('react-dom'); | ||
|
|
||
| document.body.innerHTML = ''; |
There was a problem hiding this comment.
Let's remove container from body at the end of each test instead? Or, better, declare variable once, and remove/add it before and after each test.
| }); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('EventInterface', () => { | ||
| it('normalizes properties from the Event interface', () => { | ||
| var target = document.createElement('div'); | ||
| var syntheticEvent = createEvent({srcElement: target}); |
There was a problem hiding this comment.
It looks like this test specifically verifies that the synthetic event still determines the target even if only srcElement was set. Your test doesn't verify this.
|
I removed some abstraction from the test. This allowed me to actually test the IE8 code branch (which the original test did). Hope this helps! |
|
Sure, it makes sense. Thanks! |
…cebook#11365) * Refactor SyntheticClipboardEvent tests to only use the public API * Replace local document creation by document body reset on each test case execution * Set up and tear down container separately * Tweak test assertion logic for clarity * Remove simulate abstraction and create events directly * Ensure the test covers IE8 behavior * Verify that persistence works
This is related to #11299
I've reimplemented the previous tests using only the public API, but I'm uncertain about their coverage. It doesn't check things like
setDataandgetDatabehavior on theclipboardData(DataTransfer) object, for instance. We would actually rely on implementation mocks to test these things anyway, since there's no implementation ofRangeonjsdomandDataTransferdoesn't have a constructor.