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.

Sign up for GitHub

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

Add reset, clean up before every test #121

Merged
merged 2 commits into from
Dec 29, 2015
Merged

Add reset, clean up before every test #121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
62532de
add reset method.
jamestalmage Dec 29, 2015
9ed4446
clear the temp directory and cache before every run
jamestalmage Dec 29, 2015
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
2 changes: 1 addition & 1 deletion bin/nyc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ if (process.env.NYC_CWD) {
} else if (argv._.length) {
// wrap subprocesses and execute argv[1]
var nyc = (new NYC())
nyc.cleanup()
nyc.reset()
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the if (!process.env.NYC_CWD) clause in cleanup() is causing some confusion here. I think it ensures child processes don't perform any cleanup, but maybe that guard should be in this file and not in the implementation.

TBH I'm not entirely sure what the expected behavior is and why it impacts --all.

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue with all was that it was calling cleanup(), which deleted directories, but did not recreate them.

but maybe that guard should be in this file and not in the implementation.

I think I agree with you (I considered that as well). I just wanted to avoid an additional change creeping into this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue with all was that it was calling cleanup(), which deleted directories, but did not recreate them.

Where did it call cleanup()? That's the part I'm not understanding, I suppose.

I just wanted to avoid an additional change creeping into this PR.

Sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Where did it call cleanup()?

Right there in that diff. Now instead it calls reset, which deletes and recreates the directory.

The issue is that all immediately and synchronously writes a "preample" for every included file to disk, before continuing with the test, so the folder needs to exist, even though we haven't yet forked a process.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! OK, resetting before running anything makes sense.


if (argv.all) nyc.addAllFiles()
if (!Array.isArray(argv.require)) argv.require = [argv.require]
Expand Down
9 changes: 6 additions & 3 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,6 @@ function NYC (opts) {
// require extensions can be provided as config in package.json.
this.require = arrify(config.require || opts.require)

this._createDatastoreDirectories()

this.transform = this._createTransform()

this.sourceMapCache = new SourceMapCache()
Expand Down Expand Up @@ -220,10 +218,15 @@ NYC.prototype.clearCache = function () {
rimraf.sync(this.cacheDirectory())
}

NYC.prototype._createDatastoreDirectories = function () {
NYC.prototype.createTempDirectory = function () {
mkdirp.sync(this.tempDirectory())
}

NYC.prototype.reset = function () {
this.cleanup()
this.createTempDirectory()
}

NYC.prototype._wrapExit = function () {
var _this = this

Expand Down
43 changes: 30 additions & 13 deletions test/src/nyc-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,23 +27,29 @@ var sinon = require('sinon')
var isWindows = require('is-windows')()
var spawn = require('win-spawn')
var fixtures = path.resolve(__dirname, '../fixtures')
var projectDir = path.resolve(__dirname, '../..')
var projectTempDir = path.join(projectDir, '.nyc_output')
var projectCacheDir = path.join(projectDir, 'node_modules', '.cache', 'nyc')
var fixtureTempDir = path.join(fixtures, '.nyc_output')
var fixtureCacheDir = path.join(fixtures, 'node_modules', '.cache', 'nyc')
var bin = path.resolve(__dirname, '../../bin/nyc')

// beforeEach
rimraf.sync(projectTempDir)
rimraf.sync(fixtureTempDir)
rimraf.sync(projectCacheDir)
rimraf.sync(fixtureCacheDir)
delete process.env.NYC_CWD

require('chai').should()
require('tap').mochaGlobals()

describe('nyc', function () {
describe('cwd', function () {
function afterEach () {
delete process.env.NYC_CWD
rimraf.sync(path.resolve(fixtures, '../nyc_output'))
}

it('sets cwd to process.cwd() if no environment variable is set', function () {
var nyc = new NYC()

nyc.cwd.should.eql(process.cwd())
afterEach()
})

it('uses NYC_CWD environment variable for cwd if it is set', function () {
Expand All @@ -52,7 +58,6 @@ describe('nyc', function () {
var nyc = new NYC()

nyc.cwd.should.equal(path.resolve(__dirname, '../fixtures'))
afterEach()
})
})

Expand Down Expand Up @@ -159,6 +164,7 @@ describe('nyc', function () {
var nyc = new NYC({
cwd: process.cwd()
})
nyc.reset()
nyc.wrap()

var check = require('../fixtures/check-instrumented')
Expand All @@ -171,13 +177,15 @@ describe('nyc', function () {
module._compile(fs.readFileSync(filename, 'utf8'), filename)
})

// the `require` call to istanbul is deferred, loaded here so it doesn't mess with the hooks callCount
require('istanbul')

var nyc = new NYC({
cwd: process.cwd()
})
nyc.reset()
nyc.wrap()

// the `require` call to istanbul is deferred, loaded here so it doesn't mess with the hooks callCount
require('istanbul')
// install the custom require hook
require.extensions['.js'] = hook

Expand Down Expand Up @@ -222,7 +230,9 @@ describe('nyc', function () {
it('does not output coverage for files that have not been included, by default', function (done) {
var nyc = (new NYC({
cwd: process.cwd()
})).wrap()
}))
nyc.wrap()
nyc.reset()

var reports = _.filter(nyc._loadReports(), function (report) {
return report['./test/fixtures/not-loaded.js']
Expand Down Expand Up @@ -274,6 +284,7 @@ describe('nyc', function () {
var nyc = new NYC({
cwd: process.cwd()
})
nyc.reset()

fs.writeFileSync('./.nyc_output/bad.json', '}', 'utf-8')

Expand All @@ -300,6 +311,8 @@ describe('nyc', function () {
cwd: process.cwd(),
reporter: reporters
})
nyc.reset()

var proc = spawn(process.execPath, ['./test/fixtures/child-1.js'], {
cwd: process.cwd(),
env: process.env,
Expand Down Expand Up @@ -413,14 +426,15 @@ describe('nyc', function () {
describe('addAllFiles', function () {
it('outputs an empty coverage report for all files that are not excluded', function (done) {
var nyc = (new NYC())
nyc.reset()
nyc.addAllFiles()

var reports = _.filter(nyc._loadReports(), function (report) {
return ap(report)['./test/fixtures/not-loaded.js']
})
var report = reports[0]['./test/fixtures/not-loaded.js']

reports.length.should.equal(enableCache ? 2 : 1)
reports.length.should.equal(1)
report.s['1'].should.equal(0)
report.s['2'].should.equal(0)
return done()
Expand All @@ -429,7 +443,10 @@ describe('nyc', function () {
it('tracks coverage appropriately once the file is required', function (done) {
var nyc = (new NYC({
cwd: fixtures
})).wrap()
}))
nyc.reset()
nyc.wrap()

require('../fixtures/not-loaded')

nyc.writeCoverageFile()
Expand All @@ -438,7 +455,7 @@ describe('nyc', function () {
})
var report = reports[0]['./not-loaded.js']

reports.length.should.equal(enableCache ? 2 : 1)
reports.length.should.equal(1)
report.s['1'].should.equal(1)
report.s['2'].should.equal(1)

Expand Down