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.

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

describeWithDOM enhancement #126

Merged
merged 3 commits into from
Jan 21, 2016
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,8 @@ node_modules
# Jetbrains IDEs
.idea

# vim swp files
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just like .idea above, this also seems like something that can't hurt, but should be in your global gitignore if you use vim, rather than spilling out into every project you contribute to :-/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually took a note from the above .idea line and thought it would be fine to add it in, will remove

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no worries either way, i'd prefer neither but i don't care that much

*.swp

/build
_book
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@
"check": "npm run lint && npm run test:all",
"build": "babel src --out-dir build",
"test:watch": "mocha --compilers js:babel/register --recursive src/**/__tests__/*.js --watch",
"test:all": "npm run react:13 && npm test && npm run react:14 && npm test",
"test:describeWithDOMOnly": "mocha --compilers js:babel/register --recursive src/**/__tests__/describeWithDOM/describeWithDOMOnly-spec.js",
"test:describeWithDOMSkip": "mocha --compilers js:babel/register --recursive src/**/__tests__/describeWithDOM/describeWithDOMSkip-spec.js",
"test:all": "npm run react:13 && npm test && npm run test:describeWithDOMOnly && npm run test:describeWithDOMSkip && npm run react:14 && npm test && npm run test:describeWithDOMOnly && npm run test:describeWithDOMSkip",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm confused. Why exactly were these tests needed to be added separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We didn't technically need it for .skip, but I felt like having it be consistent would be the way to go. We do need a separate run for the .only tests though, otherwise it will skip the rest of the test suite. Happy to change things up a bit to be less verbose if you have an idea though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that's fine. I'm not 100% sure we even need to add tests for describeWithDOM, but since they're already added it's better than nothing.

"react:clean": "rimraf node_modules/react node_modules/react-dom node_modules/react-addons-test-utils",
"react:13": "npm run react:clean && npm i react@0.13",
"react:14": "npm run react:clean && npm i react@0.14 react-dom@0.14 react-addons-test-utils@0.14",
Expand Down
2 changes: 1 addition & 1 deletion src/__tests__/ReactWrapper-spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ import {
mount,
render,
ReactWrapper,
describeWithDOM,
} from '../';
import { describeWithDOM } from '../describeWithDOM';
import { describeIf } from './_helpers';
import { REACT013 } from '../version';

Expand Down
4 changes: 3 additions & 1 deletion src/__tests__/Utils-spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,11 @@ import {
mapNativeEventNames,
} from '../Utils';
import {
describeWithDOM,
mount,
} from '../';
import {
describeWithDOM,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/\t/ /g please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woops, thought I caught those, will update

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb eslint should fail on these, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

theoretically, but i haven't gotten around to these rules yet, so i don't know how they're configured :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually why I thought I got them all, the linter wasn't yelling at me anymore, so it doesn't appear to be catching them everywhere, because I was getting warnings in other places

} from '../describeWithDOM';

describe('Utils', () => {

Expand Down
19 changes: 19 additions & 0 deletions src/__tests__/describeWithDOM/describeWithDOMOnly-spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { expect } from 'chai';
import { describeWithDOM } from '../../describeWithDOM';

describe('describeWithDOM', () => {
describe('.only()', () => {
describeWithDOM.only('will skip all tests not called with only', () => {
it('will run only this test', () => {
expect(true).to.equal(true);
});
});

describeWithDOM('will not call other tests', () => {
it('will not run this test', () => {
// purposefully failing test that won't be called
expect(true).to.equal(false);
});
});
});
});
19 changes: 19 additions & 0 deletions src/__tests__/describeWithDOM/describeWithDOMSkip-spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { expect } from 'chai';
import { describeWithDOM } from '../../describeWithDOM';

describe('describeWithDOM', () => {
describe('.skip()', () => {
describeWithDOM.skip('will skip tests called with skip', () => {
it('will not run this test', () => {
// purposefully failing test that won't be run
expect(true).to.equal(false);
});
});

describeWithDOM('will still call describeWithDOM tests without .skip', () => {
it('will run this test', () => {
expect(true).to.equal(true);
});
});
});
});
47 changes: 47 additions & 0 deletions src/describeWithDOM.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
let jsdom;

try {
require('jsdom'); // could throw
jsdom = require('mocha-jsdom');
} catch (e) {
// jsdom is not supported...
}

export function describeWithDOM(a, b) {
describe('(uses jsdom)', () => {
if (typeof jsdom === 'function') {
jsdom();
describe(a, b);
} else {
// if jsdom isn't available, skip every test in this describe context
describe.skip(a, b);
}
});
}

function only(a, b) {
describe('(uses jsdom)', () => {
if (typeof jsdom === 'function') {
jsdom();
describe.only(a, b);
} else {
// if jsdom isn't available, skip every test in this describe context
describe.skip(a, b);
}
});
}

function skip(a, b) {
describe('(uses jsdom)', () => {
if (typeof jsdom === 'function') {
jsdom();
describe.skip(a, b);
} else {
// if jsdom isn't available, skip every test in this describe context
describe.skip(a, b);
}
});
}

describeWithDOM.only = only;
describeWithDOM.skip = skip;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in order to eliminate duplication, you could do smth like: (pseudo code)

function testSuite(register, title, fn) {
  describe('(uses jsdom)', () => {
    if (typeof jsdom === 'function') {
      jsdom();
      register(a, b);
    } else {
      // if jsdom isn't available, skip every test in this describe context
      describe.skip(a, b);
    }
  });
}

function only(title, fn) {
  testSuite(describe.only, title, fn);
}

function skip(title, fn) {
  testSuite(describe.skip, title, fn);
}

function describeWithDOM(title, fn) {
  testSuite(describe, title, fn);
}

20 changes: 0 additions & 20 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,28 +9,8 @@ import { renderToStaticMarkup } from './react-compat';
* @class Enzyme
*/

let jsdom;
try {
require('jsdom'); // could throw
jsdom = require('mocha-jsdom');
} catch (e) {
// jsdom is not supported...
}

export let sinon = Sinon.sandbox.create();

export function describeWithDOM(a, b) {
describe('(uses jsdom)', () => {
if (typeof jsdom === 'function') {
jsdom();
describe(a, b);
} else {
// if jsdom isn't available, skip every test in this describe context
describe.skip(a, b);
}
});
}

export function useSetStateHack() {
let cleanup = false;
before(() => {
Expand Down