martyjs / marty

A Javascript library for state management in React applications
http://martyjs.org
MIT License
1.09k stars 77 forks source link

test-utils component specs throw on undefined `document` element #371

Closed aguestuser closed 8 years ago

aguestuser commented 8 years ago

Hi there. In general, I'm a huge fan of this Flux implementation, and absolutely LOVE the test-utils module. (By far the most elegant attempt at testing React apps that I've seen -- and I've been shopping around!).

There's only one problem: while I can get working tests for actions, stores, etc... My component tests are irreparably and inscrutably broken. I'm 99% confident the bug reduces to a failure to initialize the DOM document element, but am clueless as to why this is happening or how to fix the bug.

I hewed as closely as possible to the settings in the example tests project (specifically, this test when setting it up, but am stuck with a wall of inscrutable error messages and little room to test debugging hypotheses.

Hoping someone can help me out of the mire and that it might help point the way to a bug fix. Also happy to work on documenting how to avoid the problem for those who come after!

Here is the test in question (which places this component under test):

const sinon = require('sinon');
const chai = require('chai');
const sinonChai = require('sinon-chai');
chai.use(sinonChai);
const should = chai.should();
const React = require('react');
const Application = require('../../src/application');
const { RED, GREEN } = require('../../src/constants/Colors');
const { createStore, createApplication } = require('marty/test-utils');
const testTree = require('react-test-tree');

const GoButton = require('../../src/components/GoButton');

describe('GoButton Component', () => {

  const setup = (spy) => {
    const app = createApplication(Application, {
      stub: {
        goButtonStore: createStore({
          getColor: spy
        })
      }
    });
    return [app, spy];
  };

  describe('contents', () => {

    it('contains a tappable svg circle', () => {
      const spy = sinon.spy();
      const app = setup(spy)[0];
      const gb = testTree(<GoButton color={RED} />, { context: { app: app }});

      gb.svg.getAttribute('width').should.equal(220);
      gb.svg.getAttribute('height').should.equal(220);
      gb.circle.getAttribute('cx').should.equal(110);
      gb.circle.getAttribute('cy').should.equal(110);
      gb.circle.getAttribute('r').should.equal(110);
      gb.circle.getAttribute('fill').should.equal(RED);
    });
  });

});

When I run it using mocha like this:

mocha --recursive --compilers js:babel/register

I get the following error:

  1) GoButton Component Inner Component contains a tappable svg circle:
     ReferenceError: document is not defined
     at Context.<anonymous> (test/components/GoButton.spec.js:33:18)

Chasing the error through the debugger a bit, I wound up at this line in react-test-tree (namely: testTree.js:14) before execution terminated:

  var container = document.createElement('div');

This makes sense, because there's no browser in the equation, and we don't seem to be using jsdom or the like anywhere.

So I dug into the example test configs and realized they were using karma. Struggled a bit to set that up just so, and ran, producing the following error:

Chrome 44.0.2403 (Mac OS X 10.10.1) ERROR
  Uncaught Error: Invariant Violation: _registerComponent(...): Target container is not a DO
M element.
  at /var/folders/dl/vfr2j80d2dj47w4df8q4tz4r0000gp/T/2b96c2f769fa819a8f6003dfa1972fc3.brows
erify:41527:0 <- node_modules/react/lib/invariant.js:50:0

Googling around for that React error message a bit, it turns out you usually get it when the document element isn't defined when calling React.render.

Traced that to the fact that my karma.conf.js was including main.jsx, which calls React.render to a possibly undefined document. Noticed that the chat example app explicitly avoids calling React.render if it's a test environment. Added a similar guard, but to no effect, and a breakpoint inserted inside the code that should have not run if process.env.NODE_ENV was equal to 'test.' still ran.

Tried manually excluding this file in karma.conf.js. The Invariant Violation error goes away, but then I'm stuck with the even less helpful:

Chrome 44.0.2403 (Mac OS X 10.10.1) ERROR

In sum, I'm now running in circles, with increasingly few hypotheses to test, and not at all sure whether I'm observing the same problem in both testing environments. Happy to do more debugging and try to turn up more useful error logs if you'll point the way. REALLY can't emphasize enough how much I love this kit and very much want to use it and get other people to use it.

But I'm currently at my wit's end. Can anyone help?

PS: In case it's useful, here are some reference files:

{
  "name": "whereat-web",
  "version": "1.0.0",
  "description": "location sharing app for activists",
  "engines": {
    "node": "0.12.x",
    "npm": "2.11.x"
  },
  "main": "main.jsx",
  "scripts": {
    "dev": "npm run dev-serve | npm run dev-build",
    "dev-build": "webpack-dev-server --devtool eval --progress --colors --hot --content-base build --port 8090 --config webpack_dev_config",
    "dev-serve": "http-server -p 8080 ./build/dev",
    "prod-build": "NODE_ENV=production webpack -p --config webpack_prod_config.js",
    "test": "karma start",
    "test-mocha": "mocha --recursive --compilers js:babel/registert",
    "test-debug": "node-inspector --web-port=9000 & mocha --recursive --compilers js:babel/register --debug-brk"
  },
  "repository": {
    "type": "git",
    "url": "git@github.com:the-learning-collective/whereat-web.git"
  },
  "keywords": [
    "mobile",
    "web",
    "location",
    "socialJustice"
  ],
  "author": "@aguestuser",
  "license": "GPL-3.0",
  "bugs": {
    "url": "https://github.com/the-learning-collective/whereat-web/issues"
  },
  "homepage": "https://github.com/the-learning-collective/whereat-web",
  "dependencies": {
    "babel": "^5.6.23",
    "babel-core": "^5.4.7",
    "babel-loader": "^5.1.3",
    "babel-runtime": "^5.7.0",
    "bulk-require": "^0.2.1",
    "classnames": "^2.1.2",
    "css-loader": "^0.13.1",
    "expose-loader": "^0.7.0",
    "file-loader": "^0.8.3",
    "html-webpack-plugin": "^1.4.0",
    "image-webpack-loader": "^1.4.0",
    "imagemin": "^3.2.0",
    "immutable": "^3.7.3",
    "imports-loader": "^0.6.3",
    "jsx-loader": "^0.13.2",
    "less": "^2.5.1",
    "less-loader": "^2.2.0",
    "lodash": "^3.9.1",
    "marty": "^0.10.1",
    "node-libs-browser": "^0.5.0",
    "node-sass": "^3.1.2",
    "node-uuid": "^1.4.3",
    "postcss-loader": "^0.5.1",
    "react": "^0.13.3",
    "react-bootstrap": "^0.22.6",
    "react-hot-loader": "^1.2.7",
    "react-tappable": "^0.5.7",
    "react-tools": "^0.13.3",
    "style-loader": "^0.12.2",
    "superagent": "^1.2.0",
    "url-loader": "^0.5.5",
    "webpack": "^1.9.10",
    "webpack-dev-server": "^1.8.2",
    "xml-loader": "^1.0.0"
  },
  "devDependencies": {
    "babelify": "^6.1.3",
    "bulkify": "^1.1.1",
    "chai": "^2.3.0",
    "chai-as-promised": "^5.0.0",
    "jsx-require-extension": "^0.2.0",
    "karma": "^0.13.3",
    "karma-browserify": "^4.2.1",
    "karma-chrome-launcher": "^0.2.0",
    "karma-cli": "^0.1.0",
    "karma-mocha": "^0.2.0",
    "karma-spec-reporter": "0.0.20",
    "lessify": "^1.0.1",
    "mocha": "^2.2.5",
    "mocha-babel": "^3.0.0",
    "react-test-tree": "^0.2.2",
    "sinon": "^1.15.4",
    "sinon-chai": "^2.8.0"
  },
  "browserify": {
    "transform": [
      "babelify",
      "lessify",
      "bulkify"
    ]
  }
}
module.exports = function(config) {
  config.set({
    basePath: '',
    frameworks: ['mocha', 'browserify'],
    files: [
      //'test/setup.js',
      'src/**/*.js',
      'src/**/*.jsx'
    ],
    preprocessors: {
      'test/setup.js': ['browserify'],
      'src/**/*.js': ['browserify'],
      'src/**/*.jsx': ['browserify']
    },
    browserify: {
      bare: true,
      debug: true
    },
    reporters: ['spec'],
    port: 9876,
    colors: true,
    logLevel: config.LOG_DEBUG,
    autoWatch: true,
    browsers: ['Chrome'],
    singleRun: false
  });
};
aguestuser commented 8 years ago

ps: it's perhaps worth adding that this is the line that the tests throw on:

const gb = testTree(<GoButton color={RED} />, { context: { app: app }});
aguestuser commented 8 years ago

PPS: if it would be more helpful to move this to the react-test-tree issues page, please let me know, and I'd be happy to do that! :smile:

aguestuser commented 8 years ago

So... happy to report, I solved my own problem. Still at a complete and total loss as to how to get this to work with karma. But the good news is you don't need to!

If any other hapless Googler stumbles on this sometime in the future, you can ditch the entire karma approach and run the marty test-utils and their dependency on react-test-tree completely within mocha, provided you use the jsdom npm package and export it properly into the global.window object along the lines of this excellent post.

With one caveat, you need to put React into global scope for react-test-tree to pickup. That's easily accomplished by modifying the code for dom.js required in mocha.opt from what's provided in the post to the following:

var jsdom = require('jsdom');
var React = require('react');

var doc = jsdom.jsdom('<!doctype html><html><body></body></html>');
var win = doc.defaultView;
global.document = doc;
global.window = win;
propagateToGlobal(win);

// from mocha-jsdom https://github.com/rstacruz/mocha-jsdom/blob/master/index.js#L80
function propagateToGlobal (window) {
  for (let key in window) {
    if (!window.hasOwnProperty(key)) continue;
    if (key in global) continue;

    global[key] = window[key];
  }
  global.React = React;
}

for my part: this is the commit that fixed it and this is the passing test. huzzah! and hopefully this helps somebody else down the line.

:heart: :octopus:

taion commented 8 years ago

You're doing something very odd. You should try to get a basic test up and running in Karma and Chrome first - I don't have time to pull your repo right now, but it looks like it'd be a Karma config issue, since that code should be running in the browser, where document is set.

That environment check is not due to the React.render call - it's to avoid doing the other setup in that block, which is not relevant in the context of a test.

aguestuser commented 8 years ago

thanks for taking a look, @taion! i realize this is a super hacky fix (and very unseemly to be tossing values into the global namespace like that!), BUT... I lost maybe 8 hours yesterday trying to get the karma setup to work, whereas this fix worked right away and got me back to focusing on what i needed to be focusing on: writing passing tests and closing tickets -- which is what i care about, particularly since i'm on production deadline. (the "it just works" metric is a pretty powerful one in that context!)

if i were to try to dig back into the karma battle, i'd probably start by trying to use webpack instead of browserify for the compile step (since i've got a compile config for webpack that i know works in both my dev and prod environments). however, i'm very nervous doing that, since the only docs i have to go off of are john's example tests, and i had such a doozy of a time trying to get them to work copying his config (karma.conf.js and package.json) nearly verbatim.

to my (very humble and very appreciative that this framework even exists in the first place!) eye, it seems like this is a good example of a corner case that could use a bit more directly and carefully addressed in the examples and/or docs. if i'm able to figure it out (help appreciated!), i'd gladly contribute a PR to the docs to help other people like me figure this out first time around.

i think it's REALLY important!

so many highly skilled, thoughtful react developers i know blow right through and never write tests because the toolchain is uncommonly complex and its so easy to visually confirm changes with hot-loader, etc... this toolkit is a SHINING example of a team that's figured out how to make testing really frictionless, recovering the sort of ease that will encourage people to actually be disciplined in their testing practices. it's a huge selling point IMHO to distinguish marty from other flux impls, and -- if documented properly -- could go a long way toward counteracting a trend i've observed (at least anecdotally) of undertesting react code.

for all those reasons, i think it's a great contribution to the ecosystem, and i'd love to help in any way i can to make the test kit as well documented and accessible as possible.

in the meantime, i'll mull over your suggestion that i risk another lost day to figure out karma. (the advantages of testing in an actual browser env and cross-testing across multiple browsers aren't lost on me.) most importantly: thanks for responding! i know we're all busy and it can be really hard to give attention to stuff like this on OSS projects, and it always means a lot to me whenever people do. :)

taion commented 8 years ago

You can verify pretty easily in a basic Karma setup that you have access to document from your tests. I really can't speak to what specifically is happening in your case, but it does seem like an issue with your testing environment.

aguestuser commented 8 years ago

Right, but I don't actually think it's an issue with access to document. That was the problem with the pure mocha strategy (solved with jsdom, which provides a document object, which I injected into the global namespace.) The whole problem I've hit with the Karma stack is that I first got (1) the invariant error message, then (2) an error with no message at all(!!!). With nothing else to go on, it seems like an inadvisable direction to point my energies... Perhaps a pointer on how to dig into the call stack would be a good starting place? A suggestion on where to insert a debug point? Will work on it on my own and try to come back with more helpful breadcrumbs, but that's the main thrust of my reluctance: no bread crumbs to chase! An intelligent debugger's nightmare! ;)

aguestuser commented 8 years ago

PS, out of pure curiosity, @taion: why do you think the jsdom approach is not worth pursuing? Granted it's hacky, and as you say "very odd." But one could say that the very idea of the virtual DOM itself is "very odd," (insofar as it is roundabout) and because of that initially came under attack by people who said it could never be performant (since proved wrong). And without the "very odd" virtual DOM, we would have neither React nor Marty.

Isn't jsdom essentially just doing a similar trick of abstracting over the DOM so we can get some benefits we want (in this case faster/simpler testing)? Is there perhaps a reason why it's a fairly popular alternative to Karma among at least some folks who know their testing?

Given all that, I wonder: why -- in objective technical terms (correctness, performance, maintainability, etc...) -- do you think it's a bad approach to testing? Not sure I see the drawbacks... And want to make sure that other people who might find themselves in a hard place like I did and might stumble on this thread in their efforts to fix things aren't needlessly steered away from a solution that might help them.

Also completely willing to change my mind! :) (And reading up on karma with webpack with that express possibility in mind.)

taion commented 8 years ago

The point of your tests is to ensure that your code works. What does "works" mean for client-side front-end code? It means that it runs on your users' browsers.

When you use Karma with an actual browser, you're running the tests in the browser, and picking up any bugs that might happen there. That's essentially the point of Karma - if all you want is jsdom, then you don't need anything beyond just Mocha.

But generally if you're testing anything on the DOM, then you'd want to make sure your code works in browsers, with all of their quirks.

aguestuser commented 8 years ago

fair enough. i'm (almost 100%) sold. solid argument from correctness. sure i can ensure my logic is correct on the DOM with jsdom, but if i'm concerned with correctness in the environment in which my program will actually be run (and why wouldn't i be?), then i probably want a test bracket big enough to cover browsers. (unless i'd rather find out post-release that some small part of the code doesn't work on IE-blah-blah... which i don't!) will dig into trying to get a webpack/karma config setup and document any gotchas or workarounds here. thanks again for engaging. :D