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

test: consolidate on tap test runner, stop running build step #997

Merged
merged 6 commits into from Feb 22, 2019

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Feb 22, 2019

In this pull request I've refactored tests to stop running a build step. to facilitate this there's now a helper called reset-state.js that resets global state between test runs.

I've also opted to move us back to one test runner (tap) now that this consolidation has happened. Here's how the tests are laid out now:

test/lib/reset-state.js, called between test runs to reset state.
test/nyc-index.js, tests mainly for methods in index.js.
test/nyc-integration.js, CLI integration tests (@JaKXz after we land this, I think this would be a great candidate for using snapshots like we did with c8).
test/process-args.js, tests for the process-args.js helper.

TODO:

  • coverage for process-args.js is missing for some reason I'm not sure how this could have ever worked because we don't instrument process-args, we will have coverage for this file once we switch our coverage approach.

Note:

I've commented out the test:

it('should include \'node_modules\' using exclude patterns')

which also seems to be failing on master, CC: @AndrewFinlay.

Future Work:

Once we land this, I'd like to switch us to using c8 shortly afterwards, and eliminate the other wonky build step we have in the codebase.

It would also be awesome to switch to snapshots for test-integration.js.

@coreyfarrell
Copy link
Member

I've commented out the test:

it('should include \'node_modules\' using exclude patterns')

which also seems to be failing on master, CC: @AndrewFinlay.

I just posted https://github.com/istanbuljs/nyc/commits/ci-test with a revert of feat: add support to exclude files on coverage report generation (#982). @AndrewFinlay was correct that his PR started failing once #982 was merged because travis-ci was testing the result of his branch merged with master.

@coreyfarrell coreyfarrell merged commit 5d5f340 into master Feb 22, 2019
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 96.648% when pulling 29ec1b9 on faster-tests into c017bf9 on master.

@JaKXz JaKXz deleted the faster-tests branch February 24, 2019 02:15
@JaKXz JaKXz mentioned this pull request Feb 27, 2019
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.

None yet

3 participants