Add Tests / Specs

[juliankrispel]

I'm finding it hard to believe that something is ready for production if there are no unit-tests.

There's a test folder in src/__tests__ but no tests.

To run jest you need to use node version 0.8 - 0.10, npm will give you a warning when installing jest-cli.

If we end up using this at rainforest I'd be up for writing a bunch of specs. Would you like to see a pr for that?

[brianreavis]

It's amazing the things that end up in production! Jokes aside, I'm in total agreement here. I can't speak for @JedWatson or @dcousens, but I would love to see a PR for this (especially before #227). I don't have any experience with jest, only karma+mocha+chai. Tests are tests though – @JedWatson / @dcousens How do you want to proceed here?

[JedWatson]

Hey @juliankrispel, tests / specs would be very welcome. Being honest I wrote this before I got my head around how to write tests for React components. src/__tests__ was helpfully submitted via a PR, but I haven't had a chance to return and flesh them out.

All my original use cases (manual tests) are represented in the example file, which was enough to confidently release what's out there, but I feel like we've gone well beyond that point.

I think @dcousens has some more specific ideas on how we should be testing components (specifically, not using jest) so I suspect he'll give some more info on that, but otherwise I agree with @brianreavis that tests are tests, and we should have them 😄

[dcousens]

@juliankrispel it would be awesome if you could add tests!
Whether you submit them with jest or other, any is welcome.

However, ideally, we found the following set up works for tests in our other modules:

package.json

  "devDependencies": {
    "gulp": "^3.9.0",
    "happiness": "^1.0.3",
    "jsdom": "^3.1.2",
    "mocha": "^2.2.5",
    "mocha-jsdom": "^0.4.0",
  },
  "scripts": {
    "lint": "happiness",
    "unit": "mocha --compilers js:babel/register",
    "test": "npm run lint && npm run unit",
    "watch": "gulp watch:lib"
  }

/* global describe, it, beforeEach */

var assert = require('assert');
var jsdom = require('mocha-jsdom');

var React = require('react/addons');
var TestUtils = React.addons.TestUtils;

describe('COMPONENT', function () {
    jsdom();

    var render;

    beforeEach(function () {
        render = TestUtils.renderIntoDocument(React.createElement(COMPONENT, ... props));
    });

    // ... tests
})

[juliankrispel]

@dcousens can you point me to 'one of your other modules'?

The thing I do like about jest is that it does mocking/encapsulation so well which is not something I've seen other frameworks provide.

However I'm not super opinionated about using one or the other framework, as long as there is decent coverage. I think the owner of the repository should decide which tool to use for unit tests. Let me know guys @JedWatson @dcousens

Cheers 🍻

[dcousens]

@juliankrispel react-context-example is a good example :)

[dcousens]

@juliankrispel I may just be inexperienced in the realm of react unit testing, but I haven't found the mocking useful enough to warrant the rest of jest's idiosyncrasies.

Always open to seeing why it might be useful though.

[craigdallimore]

Hi folks - I've just submitted a couple of jest tests for the <Value/> component here.

Happy to throw them out in favour of mocha, if that is preferred ... ?

[craigdallimore]

Hey, seeing as mocha is being discussed I've provided a mocha version too, based on what I could see in @dcousens 's example. I've included sinon and chai to fill in the mocking and pretty assertions that Jest otherwise would give us.

[JedWatson]

Thanks @craigdallimore, this is brilliant. I really appreciate the work you put into providing example of both Jest and mocha.

My personal preference is leaning towards the Jest PR, it is simple to follow and quite explicit. Not that the mocha one is much less so...

Does anybody else have a specific preference or input before I merge one of them?

[craigdallimore]

@JedWatson the pros, as I see them:

Jest

• Fewer dependencies

Mocha

• Can run tests in the browser, if we wish
• A more explicit reporter (for some reason, Jest will show an entire files worth of tests as 'one test')

The ability to run tests using a browser/phantom might be valuable - I've had trouble with blur and focus events using JSDOM - as far as I know it doesn't recreate them faithfully (or at least, quite the same as a browser will). Does anyone happen to know a way to run jest tests in a browser?

Here is a discussion on JSDOM. jsdom/jsdom#533

[AlexKVal]

The project from the same niche https://github.com/react-bootstrap/react-bootstrap
uses Karma + Mocha + Chai + Sinon Phantom and Chrome browsers for testing.
https://github.com/react-bootstrap/react-bootstrap/blob/73c77053f843b1621efac41a77755f7f7c592bc8/package.json#L67-L76

It is pretty mature project and has a lot of tests already 😉
https://github.com/react-bootstrap/react-bootstrap/tree/73c77053f843b1621efac41a77755f7f7c592bc8/test

As experience has shown - in browser testing is essential.
And as a recent encounters also showed - because of isomorphic nature of React - 'on server' testing is also desirable.

That's my two cents 🍒

[craigdallimore]

While @JedWatson is deciding on which test runner to go with - how would you suggest we approach the <Select/> component? It is very configurable.

I'd hope that we don't go for one giant test file for all the permutations of configuration. Perhaps we might split it up by configuration option, e.g.

selectSearchable.js // tests the "searchable" option
selectClearValueText.js // test that "clear" label gets set

I'm not 100% sure this is the right idea :) but I'm looking for logical ways to break it up into small sets of focussed tests - to make it obvious where to find a test for a particular aspect, and to hopefully avoid duplicating effort.

[AlexKVal]

Totally agree on

don't go for one giant test file for all the permutations of configuration.

furthermore it is not possible to address all of them of course.

The idea is just to have a test (or a couple) for every main feature / prop.
To prevent regressions with functional changes and refactorings.

disabled and delimiter are of course must to test their functionality.

with the className, on the contrary side, it is enough to assert
that any class put through it is in place in the inner DOM component.

Also for starters it would be nice to assert that there is no any warnings in dev-console from React
for every normally used component: (links provided just for the idea)
test/index.js#L3-L14
react-bootstrap/react-bootstrap@842c19a

But before that it needs to assert that 'development' mode is 'enabled' in tests for React library,
because in 'production' it doesn't throw any warnings at all.
react-bootstrap/react-bootstrap@37f0fe0

[craigdallimore]

@AlexKVal

Also for starters it would be nice to assert that there is no any warnings in dev-console from React
for every normally used component

Great idea.

[dcousens]

Maybe its just me, but all points above aside, jest just felt incredibly slow.

The mocha approach also felt way more composable, which is great for clarity and overall understanding.

[JedWatson]

Just saw this, which is also quite interesting: http://simonsmith.io/unit-testing-react-components-without-a-dom/

The general consensus seems to be using Mocha as the test framework and bypassing jest, which I'm quite happy to go with so I'll merge #247 now.

Thanks again @craigdallimore for the "competing" PRs, and everyone for the input!

[craigdallimore]

You are welcome! @JedWatson

[craigdallimore]

Maybe its just me, but all points above aside, jest just felt incredibly slow.

@dcousens not just you! jestjs/jest#116

[dcousens]

Just saw this, which is also quite interesting: http://simonsmith.io/unit-testing-react-components-without-a-dom/

That is very nice indeed :)

[craigdallimore]

Hiya, just noticed this commit which bumps JSDOM to 5.x. I was a bit surprised to find the tests failing!

As it turns out, newer versions of JSDOM will only work in iojs, so at present tests will only pass when run on iojs.

Here is a PR which will return JSDOM to a version that will enable the tests to pass on nodeJS.

[JedWatson]

Sorry about that - I pretty regularly bump dependencies, except ones I know to watch for... JSDOM is now one of those.

The mistake is obvious in hindsight, but my workflow went "merge tests" / "update everything" / "run tests"... normally a good way to catch problems with updated packages but it backfired pretty badly in this case! 😬

Anyway, this seems safe to close now, and we can get on with increasing the test coverage.