Fork per test. #89

jamestalmage · 2015-12-09T09:28:55Z

Similar to #83,
This actually uses an AST transform to generate multiple files for forking and testing. This has a couple added benefits:

source-map support for the transform. The line numbers in the error messages match the original source.
no need for the weird _header file I was using in fork for every test in nyc-test #83, or separate helpers. The transform intelligently copies helper functions into each file.

jamestalmage · 2015-12-09T09:30:46Z

This currently runs sans-coverage (and passes). So it's a potential solution to #79 as well.

jamestalmage · 2015-12-09T11:16:42Z

Just added a self-coverage solution.

Tests can now be run with or without coverage enabled!! (#79 is fixed!)

It no longer uses tap --coverage, instead it uses a script to individually instrument the files with istanbul. It stores the coverage data a different global variable ___nyc_self_coverage___, and outputs to a directory called self_coverage.

This has the added benefit of not mixing in fixture coverage data into our coverage results.

The entire build process is implemented using npm build scripts. It's a little unwieldy. I think we should move to gulp or similar task runner.

jamestalmage · 2015-12-09T11:30:27Z

npm run dev

for non-coverage, de-obfuscated stack-trace goodness

jamestalmage · 2015-12-09T11:46:39Z

Below is the output of the first test file. Note that it is smart enough to only pull in the helper functions needed. The generated tests are stored as separate files inside the test dir. This has the added benefit of providing only like support for tests.. tap test/built-001-*.js runs test 1.

People evaluating this PR should actually clone it and run npm install && npm test to appreciate how it all works.

/* global describe, it */

require('source-map-support').install()
var _ = require('lodash')
var fs = require('fs')
var NYC

try {
  NYC = require('../index.covered.js')
} catch (e) {
  NYC = require('../')
}

var path = require('path')
var rimraf = require('rimraf')
var sinon = require('sinon')
var spawn = require('child_process').spawn
var fixtures = path.resolve(__dirname, './fixtures')
var bin = path.resolve(__dirname, '../bin/nyc')

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()
    })
  })
})

(source map comment omitted - but it is generated)

bcoe · 2015-12-09T17:18:30Z

curious to see how this work turns out, my main ask is that the flow continues to be;

clone nyc
run npm i.
run npm t.

So let's bake any build steps into pretest 👍

jamestalmage · 2015-12-09T17:40:51Z

That is still the process for builds with coverage (I do indeed use pretest). npm run dev just runs uncovered tests.

I would say this ready for preliminary review. The only thing that I think could be improved is the build. As stated I am thinking gulp. npm scripts would defer to that. Are you cool with that?

If you can, I would like you to at least clone it, run it locally and peek at the transformed files. I just want to make sure we don't feel that what I am doing is too magical before I dump a lot of extra time in it. I think it's a viable plan, but it is definitely out of the ordinary

novemberborn · 2015-12-09T18:29:21Z

The only thing that I think could be improved is the build. As stated I am thinking gulp. npm scripts would defer to that.

I'm partial to just using npm scripts.

jamestalmage · 2015-12-09T22:34:11Z

package.json

+    "dev": "npm run clean && npm run build && npm run run-tests",
+    "instrument": "node ./instrument-index.js",
+    "report": "istanbul report --include=self_coverage/*.json lcov text",
+    "run-tests": "tap -b ./test/built-*.js ./test/source-map-cache.js"


I'm partial to just using npm scripts.

Yeah, but this feels pretty close to being unmaintainable going forward.

novemberborn · 2015-12-10T11:26:48Z

build-tests.js

+'use strict'
+
+var fs = require('fs')
+var Path = require('path')


Out of interest, why the capital P?

Because I named another variable path down below. Normally I'd just rename the other variable, but path is the de facto variable name for your node path in AST transforms.

I've extracted the transform into it's own module anyways, so I can fix this up.

novemberborn · 2015-12-10T11:41:39Z

@jamestalmage what's outstanding on this?

jamestalmage · 2015-12-10T19:39:22Z

OK,
I think this is good to go.
There are still tests that I think need to be improved, but I think you have already tackled some of that in other PR's.

jamestalmage · 2015-12-11T03:15:28Z

With all the files this thing now generates, and especially because of this, we should probably start using the explicit files whitelist in package.json to ensure nothing gets accidentally deployed to npm.

novemberborn · 2015-12-11T12:31:36Z

There are still tests that I think need to be improved, but I think you have already tackled some of that in other PR's.

Could get this in first, then fix what's remaining.

With all the files this thing now generates, and especially because of this, we should probably start using the explicit files whitelist in package.json to ensure nothing gets accidentally deployed to npm.

👍

jamestalmage · 2015-12-11T16:18:59Z

OK, just updated package.json.

  "files": [
    "index.js",
    "bin/*.js",
    "lib/*.js",
    "!**/*covered.js"
  ]

Before:

$ tar -tf $(npm pack) | grep -v node_modules

package/package.json
package/.npmignore
package/README.md
package/build-self-coverage.js
package/build-tests.js
package/index.js
package/bin/nyc.js
package/LICENSE.txt
package/.babelrc
package/.idea/.name
package/.idea/encodings.xml
package/.idea/jsLibraryMappings.xml
package/.idea/misc.xml
package/.idea/modules.xml
package/.idea/nyc.iml
package/.idea/vcs.xml
package/.idea/watcherTasks.xml
package/.idea/workspace.xml
package/.travis.yml
package/lib/self-coverage-helper.js
package/lib/source-map-cache.js
package/CHANGELOG.md
package/nyc-4.0.1.tgz
package/test/nyc-test.js
package/test/source-map-cache.js
package/test/fixtures/package.json
package/test/fixtures/_generateCoverage.js
package/test/fixtures/coverage.js
package/test/fixtures/not-loaded.js
package/test/fixtures/sigint.js
package/test/fixtures/sigterm.js
package/test/fixtures/.istanbul.yml

After:

$  tar -tf $(npm pack) | grep -v node_modules
package/package.json
package/README.md
package/index.js
package/bin/nyc.js
package/LICENSE.txt
package/lib/self-coverage-helper.js
package/lib/source-map-cache.js
package/CHANGELOG.md

Note that I've used grep to remove the bundled dependency noise from the output above. The bundled dependency is included in both cases, you do not need to list it in the files array (I have verified that).

jamestalmage · 2015-12-11T16:27:36Z

Hmm, oddly enough tar -tf $(npm pack) does not list *.covered.js files even when they are in the compressed bundle. I checked by manually unzipping the result of npm pack, and the .covered.js files do get included in the bundle unless I add the negate line to the files array. Seems like a bug in tar's console output to me.

TL; DR; - I have double and triple checked my changes to package.json#files, and I'm almost certain I've done it correctly.

jamestalmage · 2015-12-11T17:27:32Z

Could get this in first, then fix what's remaining.

I agree. Let's do that.

bcoe · 2015-12-12T19:30:52Z

@jamestalmage just testing this out, really slick!

A couple things:

I'm excited to see if isolating tests helps solve some of the issues I was seeing on Windows, would it be possible for you to rebase this branch with master.
let's move the built test files into a separate ./test/build/ folder, that way I can minimize it in my text-editor and interact with the existing test files.

jamestalmage · 2015-12-12T20:39:50Z

would it be possible for you to rebase this branch with master.

Done

let's move the built test files into a separate ./test/build/ folder, that way I can minimize it in my text-editor and interact with the existing test files.

The reason I did not do that was so that require paths still looked right in the main file you were editing.

If you move them into build then you've got to do require('../fixtures/foo.js')

We could solve this by moving test sources into test/src/, directory structure could be

test/ 
    src/ - original test sources
    build/ - built tests
    fixtures/ - maintains relative path from both 'src' and 'build'
    lib/ - future use for test helper files - maintains relative paths

jamestalmage · 2015-12-12T21:08:48Z

That last commit reorganizes the directory structure as I suggested in my previous comment.

jamestalmage · 2015-12-12T21:10:07Z

not sure why the last commit caused coverage to drop, but I've got to head out for a couple hours. Will take another look later tonight

jamestalmage · 2015-12-12T23:43:53Z

Found it, I missed an update to one of the npm scripts.

bcoe · 2015-12-14T16:29:18Z

@jamestalmage this is looking great, feel free to merge this and your other test refactor work when you feel it's ready.

jamestalmage · 2015-12-14T17:39:59Z

@jamestalmage this is looking great, feel free to merge this and your other test refactor work when you feel it is ready.

It is basically ready to merge (I will likely squash the history a bit first), but I am not a collaborator.

This adds a build step to the test process that rewrites `test/nyc-test.js` into multiple files with one test per file. Since taps forks for each test file, this provides a pristine environment for testing our various system hooks. squashed commits: Working test suite without coverage tweak build script

add back coveralls report

squashed commits: rename instrument-index to build-self-coverage (more accurate description) add leading `.` to self_coverage output directory minor fixups to npm scripts fix `npm run report` - it was using the old coverage folder name move built tests to test/build/, originals sources to test/src fix test script

Fork per test. Closes #80 Closes #85

jamestalmage · 2015-12-14T23:38:11Z

Landed. Thanks @bcoe!

This was referenced Dec 9, 2015

fork for every test in nyc-test #83

Closed

proposal: split into one test per file #82

Closed

novemberborn mentioned this pull request Dec 9, 2015

wip! use tap.spawn() for each test #85

Closed

2 tasks

jamestalmage mentioned this pull request Dec 9, 2015

the JSON output dir test does not test what we think it does #86

Closed

jamestalmage reviewed Dec 9, 2015
View reviewed changes

jamestalmage mentioned this pull request Dec 10, 2015

WIP - Babel 6 avajs/ava#221

Closed

bcoe mentioned this pull request Dec 10, 2015

Windows support #81

Closed

novemberborn reviewed Dec 10, 2015
View reviewed changes

jamestalmage force-pushed the recast-forking branch from 1bfc2df to 76d2030 Compare December 10, 2015 19:27

jamestalmage mentioned this pull request Dec 11, 2015

Recast forking with capture require #90

Merged

jamestalmage force-pushed the recast-forking branch from 2ba8fdf to c355787 Compare December 12, 2015 20:33

This was referenced Dec 14, 2015

Consider allowing custom test preprocessors avajs/ava#229

Closed

Needs tests. danez/pirates#5

Closed

jamestalmage added 5 commits December 14, 2015 18:31

add self coverage

ed07e70

add back coveralls report

apply standard style to build-tests.js

8d09861

extract forking-tap module

b4cc1e6

jamestalmage force-pushed the recast-forking branch from e632d65 to 3eb8cbf Compare December 14, 2015 23:33

jamestalmage added a commit that referenced this pull request Dec 14, 2015

Merge pull request #89 from jamestalmage/recast-forking

bec1f9e

Fork per test. Closes #80 Closes #85

jamestalmage merged commit bec1f9e into istanbuljs:master Dec 14, 2015

jamestalmage deleted the recast-forking branch December 14, 2015 23:37

jamestalmage mentioned this pull request Dec 14, 2015

nyc-test needs tap --coverage #79

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fork per test. #89

Fork per test. #89

jamestalmage commented Dec 9, 2015

jamestalmage commented Dec 9, 2015

jamestalmage commented Dec 9, 2015

jamestalmage commented Dec 9, 2015

jamestalmage commented Dec 9, 2015

bcoe commented Dec 9, 2015

jamestalmage commented Dec 9, 2015

novemberborn commented Dec 9, 2015

jamestalmage Dec 9, 2015

novemberborn Dec 10, 2015

novemberborn Dec 10, 2015

jamestalmage Dec 10, 2015

novemberborn commented Dec 10, 2015

jamestalmage commented Dec 10, 2015

jamestalmage commented Dec 11, 2015

novemberborn commented Dec 11, 2015

jamestalmage commented Dec 11, 2015

jamestalmage commented Dec 11, 2015

jamestalmage commented Dec 11, 2015

bcoe commented Dec 12, 2015

jamestalmage commented Dec 12, 2015

jamestalmage commented Dec 12, 2015

jamestalmage commented Dec 12, 2015

jamestalmage commented Dec 12, 2015

bcoe commented Dec 14, 2015

jamestalmage commented Dec 14, 2015

jamestalmage commented Dec 14, 2015

Fork per test. #89

Fork per test. #89

Conversation

jamestalmage commented Dec 9, 2015

jamestalmage commented Dec 9, 2015

jamestalmage commented Dec 9, 2015

jamestalmage commented Dec 9, 2015

jamestalmage commented Dec 9, 2015

bcoe commented Dec 9, 2015

jamestalmage commented Dec 9, 2015

novemberborn commented Dec 9, 2015

jamestalmage Dec 9, 2015

Choose a reason for hiding this comment

novemberborn Dec 10, 2015

Choose a reason for hiding this comment

novemberborn Dec 10, 2015

Choose a reason for hiding this comment

jamestalmage Dec 10, 2015

Choose a reason for hiding this comment

novemberborn commented Dec 10, 2015

jamestalmage commented Dec 10, 2015

jamestalmage commented Dec 11, 2015

novemberborn commented Dec 11, 2015

jamestalmage commented Dec 11, 2015

jamestalmage commented Dec 11, 2015

jamestalmage commented Dec 11, 2015

bcoe commented Dec 12, 2015

jamestalmage commented Dec 12, 2015

jamestalmage commented Dec 12, 2015

jamestalmage commented Dec 12, 2015

jamestalmage commented Dec 12, 2015

bcoe commented Dec 14, 2015

jamestalmage commented Dec 14, 2015

jamestalmage commented Dec 14, 2015