Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add focusable test #65

Merged
merged 10 commits into from Apr 28, 2016
Merged

Add focusable test #65

merged 10 commits into from Apr 28, 2016

Conversation

jbinto
Copy link
Contributor

@jbinto jbinto commented Apr 19, 2016

@jbinto
Copy link
Contributor Author

jbinto commented Apr 20, 2016 •
edited

Asked on Reactiflux #testing channel.

[8:36 PM] Shados: the cheesy way is probably just to use Enzyme's .get to get the react element, then use ReactDOM.findDOMNode on it to get the native element, from which you can just trigger the event.
[8:36 PM] Shados: just a guess though.
[8:36 PM] Shados: but at that point you could just use TestUtil.renderIntoDocument and save yourself the trouble.
[8:36 PM] Shados: instead of fighting Enzyme

Tried this in bce54bb, no luck. I found a way to get at the HTMLInputElement, call click() on it, but the document.onClick event never fires.

@jbinto
Copy link
Contributor Author

jbinto commented Apr 20, 2016 

[12:54 PM] bruderstein: I've done this with jsdom before by creating the event, and then calling dispatchEvent
[12:55 PM] bruderstein: A bit heavy-handed maybe, but it tests the handler is really called.

This approach looks promising.

@jbinto jbinto force-pushed the feature/add_focusable_test branch from 5b2edbb to 8a73e36 Compare April 21, 2016 14:02
@jbinto
Copy link
Contributor Author

jbinto commented Apr 21, 2016 •
edited 

[1:24 PM] natelaws: 
const event = new Event('click', { target: menu });
window.dispatchEvent(event); 
[1:24 PM] natelaws: works pretty well for me
[1:27 PM] natelaws: maybe, hmmm
[1:36 PM] natelaws: bah, can't manually set target
[1:37 PM] bruderstein: I think you can call dispatchEvent on any node, if that helps
[1:39 PM] natelaws: yeah, but TestUtils.renderIntoDocument doesn't render into the window, so events don't bubble up to the window event listener
[1:43 PM] bruderstein: Sounds like you're going to need to render into the window then....  :(

Should be okay for us since we're using jsdom which has a window...

@SpiritBreaker226
Copy link
Member

JSDOM loads only at the start of the testing, meaning that if a test needs to add event listeners to the window, they cannot since window is already set. I'm thinking that is there a way of adding our own version of window object and test it from that

@jbinto
Copy link
Contributor Author

jbinto commented Apr 28, 2016

[...] meaning that if a test needs to add event listeners to the window, they cannot since window is already set [...]

Not quite true, the idea of the boilerplate in test_helper.js is to create the document and window objects, and expose them globally so it "feels" like a browser environment. Each test file and component would then be looking at the same global document and window objects. See enzyme's docs, they have a boilerplate setup for jsdom very similar to what we do: https://github.com/airbnb/enzyme/blob/master/docs/guides/jsdom.md

But you got me thinking about it, and I think I've finally solved it.

I did some tests with the following structure:

<Foo>
  <div>
    <input />
  </div>
</Foo>

I added event listeners to input, div, document and window.

webpackbin (e.g. real browser):

  • Clicking input bubbles up to div, document, window, as expected.

jsdom without enzyme:

  • "Clicking" input (via .click() or dispatchEvent) bubbles up to div, document, window, as expected.

jsdom with enzyme:

  • "Clicking" input (via .click() or dispatchEvent) bubbles up to div, but stops there.
  • If you call window.dispatchEvent directly (as opposed to input.dispatchEvent), the window's event listeners are in fact called. It's only an issue when trying to dispatch via input, that the bubbling "stops" before it reaches the window.

Something I wanted to try was inspecting the jsdom output, like how we do wrapper.debug(). I found this snippet somewhere on a jsdom issue:

document.documentElement.outerHTML  // documentElement is the <html> tag

Called at the top of a test file, this outputs:

<html><head></head><body></body></html>

So I tried mounting the component with enzyme, and checking the jsdom state:

<html><head></head><body></body></html>

WTF? Why isn't jsdom's internal structure updating??

I started looking at the enzyme source code, wondering where the call to ReactDOM.render was. Turns out, they just use the standard Facebook TestUtils.renderIntoDocument underneath the scenes.

A little more sleuthing and I find this comment, emphasis mine:

As far as react goes, enzyme's mount() uses TestUtils.renderIntoDocument internally which doesn't even attach itself to the main document, so it has pretty limited side effects that could affect other tests, but you still need to be conscious about some DOM APIs that will have side effects across tests.

Hmm. What does this mean? Looking at TestUtils.renderIntoDocument source (https://github.com/facebook/react/blob/master/src/test/ReactTestUtils.js?source=c#L79)

renderIntoDocument: function(instance) {
  var div = document.createElement('div');
  // None of our tests actually require attaching the container to the
  // DOM, and doing so creates a mess that we rely on test isolation to
  // clean up, so we're going to stop honoring the name of this method
  // (and probably rename it eventually) if no problems arise.
  // document.documentElement.appendChild(div);
  return ReactDOM.render(instance, div);
},

So the jsdom document we're giving to Enzyme is actually not what we think it is.

  • Enzyme (via TestUtils.renderIntoDocument) doesn't render to it statefully.
  • It leverages the document API, the static methods.
  • It creates ephemeral DOM nodes, and renders to those.
  • It doesn't actually mutate document, at least not purposefully.
  • It means for any given enzyme/mount test, the topmost/root element is a <div>, not window
  • In our tests/React component, we are adding event listeners to the DOM tree called document. Enzyme then uses document.createElement to create another tree detached from that document. Of course it doesn't bubble up.

Solution:

I was thinking about Enzyme's static rendering, but I don't think that would help because it's just basically allowing you to parse a DOM and traverse it with jquery-style selectors, there's no support for events.

We still want to use JSDOM, but not enzyme. Just ReactDOM.render into an element in a jsdom document, the same way you would for a real webpage. Use the standard DOM API methods like querySelectorAll and dispatchEvent to invoke events.

Since we'd be operating on React-rendered HTML, we won't be able to assert on React props like with Enzyme (e.g. we can't do expect(wrapper.prop('focused').to.be.true()), but we can assert on render, where we conditionally render something like <p>focused</p> and check for that using vanilla DOM API.

Rabbit holes I went down trying to solve this:

  • We were using throw new Error while running the test watcher to quickly debug issues, but inside an addEventListener callback, exceptions are silently caught! They can be handled via window.onerror, but it won't terminate the program the way we wanted), so it looked like our event handler code was never being reached.
  • I mistakenly assumed that event bubbling simply wasn't working at all, because my failing test went from input -> document -> window. It wasn't until I tried input -> div -> document -> window that I saw bubbling worked, just not for document/window.
  • Tinkering with { bubbles: true } and/or { cancelable: false } and/or hunting for stopPropagation() calls in Enzyme/React/whereever
  • Typing this before I forgot everything. "I would have written a shorter letter, but I did not have the time."

@jbinto
Copy link
Contributor Author

jbinto commented Apr 28, 2016 •
edited

@jasonstats Please revert ab2c73d and rebase this branch. The linting errors are because your local node_modules version of eslint is outdated newer than what's actually on this branch.

On develop, we have "no-underscore-dangle": ["off"] in our .eslintrc. When I upgraded to airbnb's 8.0 rules I decided to turn this off: 6db6eeb

If we want to revisit it, we should fix it everywhere.

@jbinto @SpiritBreaker226
WIP - not working
fe4a66f
@jbinto @SpiritBreaker226
another attempt: got Input DOM, still no event fires :(
2783de2
@jbinto @SpiritBreaker226
A few more tries, still no luck getting the event handler to even fire
eb03e93
@jbinto @SpiritBreaker226
Still no luck. (...)
4101925 
Figured out a few things:

1) Throwing from inside an event handler is silently caught and ignored.
   Events were in fact being dispatched.

2) In a vanilla JSDOM implementation, clicking the input bubbles up to
   the document and window.

   When React + Enzyme are brought into the mix, the bubbling from input
   stops working.
@jbinto @SpiritBreaker226
Working test example!
52462c3
@jbinto @SpiritBreaker226
fix linting
b38e47f
@jbinto jbinto changed the title (do not merge) Add focusable test Add focusable test Apr 28, 2016
@SpiritBreaker226 SpiritBreaker226 merged commit 5ff17e8 into develop Apr 28, 2016
@SpiritBreaker226 SpiritBreaker226 deleted the feature/add_focusable_test branch April 28, 2016 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Adding Testing & Coverage
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jbinto @SpiritBreaker226