Native test coverage beta candidadte

[m-a-r-c-e-l-i-n-o]

Ironed out most of the issues discussed here. I think we're pretty close to a working release, just seeing if maybe we can get rid of the need for physical files. For the time being, please test this pull request on your system(s), I'm not feeling too confident about my simplistic normalization of the "file://" paths. It might fail on Windows or Mac... Not sure. Also had to create two more options (JS Api only, for now). Here's what my calling script looks like:

var nodeJspmJasmine = require( 'node-jspm-jasmine' )
nodeJspmJasmine.runTests( {
    // Provide a custom jasmine configuration without a jasmine.json.
    jasmineConfig: {
        "spec_dir": "src/test/specs",
        "spec_files": ["**/*.js"],
        "helpers": []
    },
    coverageFiles: ["src/shared/**/*.js"],
    coverageDir: './coverage-html'
}, function (err) {
    console.log( err );
} )

[joeldenning]

Oh nice, looks like you found a workaround for the problem where nothing was running after jasmine.execute(). I'll still look into that just for my own curiosity, and I'll also pull down these changes and see how they work for me.

[m-a-r-c-e-l-i-n-o]

Deff, and please share your findings if you solve the mystery. I needed a workaround like that either way since jasmine.execute() appears to be asynchronous. By the way, I'm using this as my app script:

export default function () {
    var x = 'hey'
    if ( false )
        x = 1

    return x
}

and you should see the Istanbul output be this:

[joeldenning]

@m-a-r-c-e-l-i-n-o looks great! Of the two projects that I tested, one of them generated coverage reports right out of the box! The other one I was able to track down the problem pretty quickly -- load.metadata.sourceMap is not always defined. The specific file that did not have the source map looked like this:

describe('Summary Table Helpers', () => {

});

I'm not totally sure why JSPM didn't create a source map for this file, but maybe because it's so simple that the transpilation step didn't alter the file enough to warrant a sourcemap?? That's just a guess though. I was able to solve the problem by being a little more defensive in the System.instantiate hook (see my other comment on that specific line).

Regarding your other question, I believe the file:// prefix should work just fine on all OS (I know it's fine on linux and mac, and this link says it should work fine on windows).

[joeldenning]

load.metadata.sourceMap is not always defined. I'm not totally sure why (see my other comment for my guesses), but how do you think we should handle those cases? Perhaps just skip files where systemjs does not generate a source map? Or maybe assume that it means that the source file itself should be referenced as the source map file?

[m-a-r-c-e-l-i-n-o]

I was just about to reply to your other comment, trying to keep up, lol. So here's what I was saying...

I had some doubts about the load.metadata.sourcemap because jspm only transpiles when it needs to, but then I reasoned that you have to export something if you are to test it, and if you export you must be using ES6, therefore there must always be a source map. With that in mind, notice how you had an issue with a file that has no exports, but also notice how that file should not be getting covered! LOL. Actual test files should not get covered, only app files, thus the option for defining your app files coverageFiles: ["src/shared/**/*.js"]. Instead of accommodating, we should reject these files. Not sure what the best approach is, but maybe we should just let it throw errors for files without sourcemaps, because it likely shouldn't be getting covered anyway and prompt the user to exclude them with the coverageFiles option. But this leaves me with a lingering feeling that maybe there are files that should be covered but don't have source map. I just can't think of any. Can you think of a file that is not a test file, but that can be tested, and that does not export anything therefore would not get transpiled with a source map?

Maybe we can only process files with exports (and thus have sourcemap), and have another option where the user can define exception files (if they even exist)? Or not provide an exception option, wait for a user to complain, and go from there...

[joeldenning]

There are plenty of people who use JSPM for files that are not ES6 modules (amd modules, umd modules, commonjs modules, typescript, or even just globals etc). Those modules have things that can be tested because they either export things (just not with the export keyword) or they expose globals that can be tested. In the case of amd, umd, globals, and commonjs, the files might not be transpiled but SystemJS just provides them a require function, a define function, etc.

Regarding what we should do in these cases, could we potentially just instrument the file with Istanbul, without having a source map that will be remapped? I'm not sure if remap-istanbul is okay with some files not having source maps, but my guess is that it would be fine??

[m-a-r-c-e-l-i-n-o]

=O okay, so there's a lot more that should be allowed than I thought. My biggest concern is just not letting a bunch of depedencies go through the coverage system. But yeah, would have to bypass "remap-istambul", not a big deal. Should be straightforward.

EDIT: On second thought, this is a little more involved, "remap-istambul" does care. I might have to go as far as mocking the source maps for files without any, was not thinking in terms of "batch mode" when I said "Should be straightforward". I'll get it done by tomorrow. Lmk what you want to do about my other comment so I can submit an update with all the changes.

[joeldenning]

Also, here's a list of some other things I think we could fine tune with coverage. If you'd prefer we could address these things after merging this pr in, or we could do it as part of this pr. Your call.

• Coverage should not include spec files or helper files. In the System.instantiate hook, we can just test the load.address against the glob defined in the jasmine configuration.
• I think coverage should be an opt-in feature. From the command-line, I think a --coverage flag would be perfect.
• When coverage is turned on, I think it should print the coverage reports to the console (just another reporter) in addition to the html report.
• --coverage-dir should be a command line option, like you pointed out earlier.
• Do we really need a coverageFiles option? Couldn't we just infer which files to run coverage on, since it will be everything besides the helper files and the spec files themselves? Or is there a use case for customizing which files to run coverage on?
• To stick with the convention I've seen in karma, jest, and other test-runners that produce coverage, I think the default directory for the coverage report should be called coverage instead of html-report.

Like I said, your call on if we want to do these things as separate pull requests or not.

[m-a-r-c-e-l-i-n-o]

Awesome about the file:// thing!

It's up to you on the pr update, I would say we update this pr request to address all things not having to do with the CLI interface. Then do a separate one for that.

• Coverage should not include spec files or helper files or library/dependency files <-- because of this, I found it easier to define what should be covered rather than what shouldn't. A ton of jspm modules goes through that instantiate hook, if you think it's enough to exclude the spec files, helper files, and anything in the "jspm_packages" folder (or whatever their package folder is), then I suppose it could work.
• Coverage opt-in feature for the JS Api would be "{ coverage: true }", correct?
• Printing the coverage reports to the console is interesting, I have to look into this one to see what the format is.
• I'm not 100% sure we need a coverageFiles option, I addressed my concerns in part in previous comments, we can change it, just let's make sure dependency files don't slip in. Lmk.
• Coverage report should be called coverage instead of html-report: easy change :)

[joeldenning]

I like that plan -- we'll do the cli portion afterwards.

• As I think about it more, I think you're right that it's easier just to specify which files to do coverage for. Also there is a good precedent for that with karma-coverage which has you specify which files you want to preprocess with the istanbul instrumenter.
• In terms of the JS API, I think that karma-coverage has a pretty decent API. I think we could modify it slightly since we're not dealing with karma. What I'm thinking is something that looks like the following (see corresponding karma-coverage api). Interested in your thoughts on it:

{
    // presence of the `coverage` property means that you want coverage. No need to put "coverage": true.
    coverage: {
      dir: 'coverage',
      files: 'src/**/*.js',
      // The text and text-summary reporters would *always* used
      reporter: 'html', // html is the only reporter we would start with support for
    }
}

• I think istanbul's text and text-summary reporters are the two we'd be interested in for printing to the console. We'd probably want both so that you can tell each file's coverage and the overall coverage. See how karma-coverage uses it, it looks like basically you just create the reporters pretty much the same way that you created the html reporter.

[joeldenning]

Oh also, as I thought about it more, I'm not totally sure if the file:// will actually work on windows (since I'm not sure if load.address will actually have a file:// in front of it that we can lop off), but I'm fine with crossing that bridge when we get there.

[m-a-r-c-e-l-i-n-o]

Okay, I've updated the pr. Couple of things to note:

• I couldn't produce a file without a source map, jspm provides sourcemaps for me even for files without exports, and I didn't insist further into other formats. So instead, I just deleted the source map from the load object to test. To your point earlier, I made some changes to how I was dealing with source maps, and just letting everything go through "istanbul-remap" with some source maps missing seems to be working okay. Please test on your end and let me know what happens. There is a far more robust approach for dealing with source-map-missing cases, but I think this approach should suffice for now.
• For the files: 'src/**/*.js', I also added the ability to pass in strings or an array of globs. This won't be applicable to the CLI, though, not sure if that's a concern. The more we improve this, the more I feel like this is morphing into a brand worthy test runner. Maybe at some point we should switch to a paradigm similar to Karma where the CLI takes in a config file.
• For the reporter: 'html', we could just support all of them (one at a time, unlike karma), the code is already setup that way. To your point about the text and text-summary, they all follow the same pattern with the dir option. If the user inputs reporter: 'json', it just works. Some of the reporters do have other optional parameters, but we don't need to support that yet.
• Since I'm now inlining the source maps, only one temp file is created for every file being covered (as oppose to two). I think we should stick with this approach for the long run and back out of this request. It just dawned on me that if we do everything in memory, we'll hit memory issues with larger number/content of files. Writing to disk is a little slower and can cause some inconveniences (mainly temp files not being cleaned up -- added cleanup on error to mitigate this possibility), but I think it's better than potentially running into memory issues. What do you think?

So yeah, that pretty much covers all our concerns for now, let me know if I missed something.

[joeldenning]

@m-a-r-c-e-l-i-n-o Thanks for all the help with this! There are still some things to do with it, but this is a great start. I'll publish a new version of node-jspm-jasmine now, and then hopefully have time today to follow up with some improvements.

[joeldenning]

Published as 2.1.0

[m-a-r-c-e-l-i-n-o]

@joeldenning Issues #8, #9, and #10 are trivial, let me know if you want me to take care of them. Would love a description of #11 for curiosity sake, and could potentially take care of it too (maybe the robust solution I hinted previously would be necessary). Issues #12 and #13 are simple, here if you need me. Lastly, can I get your confirmation to keep the temp file strategy (to avoid memory issues) and close this request?

[joeldenning]

@m-a-r-c-e-l-i-n-o I'm currently working on #8, #9, #13, and #14. But feel free to pick off any of the other ones. Also, I'll add some more detail to #11.

Regarding your other question, I'm fine with sticking with the temp file strategy. One thing with that, though, is that we'll have to make very sure that those files are deleted under all circumstances.

[m-a-r-c-e-l-i-n-o]

@joeldenning I can take care of #10 and I can look into #11 when you post more info. I'll leave #12 to you since it's along the same vein as #13. I'm going to open an issue for making sure that the temp files are deleted under all circumstances.