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

Migrate tests to Mocha #163

Merged
merged 11 commits into from Oct 28, 2014

Conversation

Projects
None yet
2 participants
@danvk
Copy link
Contributor

danvk commented Oct 28, 2014

The name says it all. High order bits:

  • I'm not using proxyquire because its @global feature mucks with the node.js module cache & fundamentally breaks React, which expects to only be loaded once.
  • We apply the JSX transformer to anything with an @jsx in it. I tried renaming all our components to .jsx, but this made node-inspector less pleasant.
  • It's still helpful to mock out BioDalliance and the AttributeCharts. I did this via the preprocessor and a global list of React Components to mock. Mocks are now opt-in.
  • Since we're no longer using Jasmine, I switched to sinon for stubs and assert for tests. We can use something fancier like chai if you prefer.

Tests run a lot faster now, and hopefully less flakily!

Fixes #128

@@ -0,0 +1,11 @@
// Via http://www.asbjornenge.com/wwc/testing_react_components.html
module.exports = function(markup) {
if (typeof document !== 'undefined') return;

This comment has been minimized.

@ihodes

ihodes Oct 28, 2014

Member

What's going on with this check?

This comment has been minimized.

@danvk

danvk Oct 28, 2014

Contributor

I'd guess it's there to make this a no-op in a real browser.

This comment has been minimized.

@ihodes

ihodes Oct 28, 2014

Member

Ah I see the attribution now, makes sense.

@@ -5,7 +5,7 @@ var _ = require('underscore'),
d3 = require('d3/d3'),
React = require('react'),
utils = require('../utils');
var d3BarChart = require('./d3.bar-chart');
var d3BarChart = require('./d3.bar-chart')(d3);

This comment has been minimized.

@ihodes

ihodes Oct 28, 2014

Member

Hmm, so we purposely didn't do it this way before; any reason we need an API change for these mocha tests?

This comment has been minimized.

@danvk

danvk Oct 28, 2014

Contributor

I don't remember why we avoided doing it this way. I made the change because d3.bar-chart assumes that d3 is defined globally, which isn't very compatible with require. I can revert if you prefer, it just seems odd to have a requirement like that for otherwise very Node-y code.

This comment has been minimized.

@ihodes

ihodes Oct 28, 2014

Member

Yeah definitely better to not have it global; the ideal solution is to require(d3), but this works too, and since we're going to trash d3.bar-chart at some point in the not-too-distant future, this doesn't really bother me.

@@ -2,6 +2,7 @@
"use strict";

var _ = require('underscore'),
d3 = require('d3/d3'),

This comment has been minimized.

@ihodes

ihodes Oct 28, 2014

Member

Will we need to enforce requiring d3/d3 whenever we want to use d3?

This comment has been minimized.

@danvk

danvk Oct 28, 2014

Contributor

I reverted this change. It works either way. Yay Mocha!

This comment has been minimized.

@ihodes

ihodes Oct 28, 2014

Member

That is awesome.

@ihodes

This comment has been minimized.

Copy link
Member

ihodes commented Oct 28, 2014

Awesome! LGMT % travis + a few comments.

danvk added some commits Oct 28, 2014

ihodes added a commit that referenced this pull request Oct 28, 2014

Merge pull request #163 from hammerlab/mocha2
Migrate tests to Mocha

@ihodes ihodes merged commit 3b79063 into master Oct 28, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@ihodes ihodes deleted the mocha2 branch Oct 29, 2014

@danvk danvk referenced this pull request Jan 21, 2015

Closed

Running tests in the browser #139

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment