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

Moving to a mock for tests #244

Merged
merged 1 commit into from
Mar 5, 2016
Merged

Moving to a mock for tests #244

merged 1 commit into from
Mar 5, 2016

Conversation

mcwhittemore
Copy link
Contributor

Closes #235
Closes #200
Closes #197

This also makes it so permanent features aren't selectable with Draw.select().

@mcwhittemore mcwhittemore force-pushed the make-tests-work branch 4 times, most recently from 9c0ee9d to 6ed5b5c Compare March 4, 2016 17:30
@mcwhittemore mcwhittemore changed the title [DNM] moving to a mock for tests Moving to a mock for tests Mar 4, 2016
@mcwhittemore
Copy link
Contributor Author

Can someone take a look at this and make sure all is well?

/cc @kelvinabrokwa @drewbo @averas

@@ -12,8 +12,7 @@
]
},
"scripts": {
"test": "npm run lint && npm run prepublish",
"test-unit": "browserify test/*.test.js | tap-closer | smokestack | tap-status",
"test": "tape -r ./test/mock-browser.js -r babel-register test/*.test.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the removal of linting here intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. test-unit was the real tests but they couldn't run in circle so for circle we tested if we could build. I don't think we need to test this anymore, but if you have concerns about not testing the build in circle (its implicitly tested by publishing) I'm all ears.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's nice for making sure that PRs don't introduce linting problems to fail the tests on this

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on including npm run lint

@drewbo
Copy link
Contributor

drewbo commented Mar 4, 2016

@mcwhittemore LGTM other than the things noted. Should probably try to add back in the skipped tests at some point but not necessary for this PR

mcwhittemore added a commit that referenced this pull request Mar 5, 2016
@mcwhittemore mcwhittemore merged commit bab227c into master Mar 5, 2016
@mcwhittemore mcwhittemore deleted the make-tests-work branch March 5, 2016 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants