Proof of concept of a more sustainable coverage approach. #147

m-a-r-c-e-l-i-n-o · 2016-04-28T19:25:50Z

The popular karma-coverage does not work as expected for me, mainly because Istanbul will try to instrument es6/es7 files and fail. As mentioned here, the workaround would be to use karma-babel-preprocessor, but there are two issues with this approach:

This requires the user to make sure "karma-babel-preprocessor" is using the same babel configuration/presets as their jspm setup, as well as additional configuration for source mapping.
There are other transpilers/customization without a solid solution.

Here lies a proof of concept for allowing jspm to do all the heavy lifting with practically no additional configuration. This approach has been successfully used in node-jspm-jasmine — I helped author it. I'm not suggesting that this be merged blindly, but do open room for discussion. If there is a better approach, please do let me know!

Notice that the "reporter.js" file, along with its local file dependencies, are nearly identical to the one in the karma-coverage project, with almost all modifications occurring inside here.

maxwellpeterson-wf · 2016-05-11T20:10:14Z

src/adapter.js

@@ -47,6 +47,19 @@
            System.bundles = [];
        }

+        var systemInstantiate = System.instantiate;


Could you add a comment somewhere in this new block of code explaining what it is doing/why? Looks like it replaces paths of the coverage files to include the /base/ path?

@maxwellpeterson-wf Not a problem, I'll add the comments, but I'll also leave you with a quick explanation for now. That code is converting the paths from a system ('file://') format into urls. Including /base/ is just one aspect of that, and it should probably not be hard coded. This part is necessary because the Istanbul instrumentation happens on the system, but the execution happens on the browser. In other words, say file "x" is on your system. Before Karma launches the browsers, file "x" is prepossessed and instrumented by Istanbul, we then take that source and store in an object for the browser to access. But when the browser gets it, all the "urls" are system paths (i.e. file:///home/project/x.js). So we first need to extract only the unique portion (i.e "project/x.js" - the fileKey) to retrieve the source of the file from the object and then convert the paths in the source's content, whose dependencies still has it's "urls" in system file format, to actual urls (i.e. http://localhost:8080/base/project/x.js). I hope that clarifies a bit.

maxwellpeterson-wf · 2016-05-11T20:12:41Z

I think this is looking great, thanks for such a substantial contribution!

@evanweible-wf want to take a look?

evanweible-wf · 2016-05-11T20:32:27Z

Yeah looks good to me as well!

m-a-r-c-e-l-i-n-o · 2016-05-12T19:34:58Z

@maxwellpeterson-wf @evanweible-wf Thanks for the positive feedback! I'll look into what needs to be cleaned up and update this pr with a release candidate for you guys to test in the next few days/weeks.

sergei-maertens · 2016-06-07T10:31:02Z

this is looking great - is there any ETA on a RC?

evanweible-wf · 2016-06-07T14:25:50Z

@sergei-maertens sorry, I hadn't noticed that @m-a-r-c-e-l-i-n-o had rebased. I'll take another look today!

sergei-maertens · 2016-06-07T14:31:27Z

After two days of diving into way too much source code I have a working coverage with ES6 + React/JSX set up, so I can continue and there's no hurry, but this does seem cleaner and saner than our current setup :-)

m-a-r-c-e-l-i-n-o · 2016-06-07T15:26:28Z

@evanweible-wf @sergei-maertens I was waiting (but also had other responsibilities) on this pr to move forward, and it was finally accepted a few days ago. I'll be updating this pr with a much cleaner version by tomorrow, it's pretty much done.

evanweible-wf · 2016-06-07T15:28:15Z

@m-a-r-c-e-l-i-n-o sounds great!

m-a-r-c-e-l-i-n-o · 2016-06-07T19:18:46Z

Updated this pr with a decent release candidate. It passed the unit and lint tests, and runs just fine on one of my local projects. I've done no isolated tests, but it should be fine for now.

@maxwellpeterson-wf @evanweible-wf @sergei-maertens Please pull down this pr locally and let me know if any of you have issues. Couple of things to note:

Since we are using the reporters from "karma-coverage", all the "coverageReporter" settings are valid for "karma-jspm" as well, use it the same way you would with "karma-coverage".
Include "jspm" in reporters, preprocessors, and frameworks.
I had to install "karma-coverage" directly from their github repo since npm hasn't been updated yet.
I had to install "remap-istanbul" directly from a fork in my github since I'm still waiting on this pr.
If you see a folder on your "html" report called "no-source-map", it means that one or more of your files was transpiled without a sourcemap. You'll need to make sure your jspm sourcemap generator is enabled. (It should be enabled by default.)
Here's what my karma config file looks like:

module.exports = function(config) {
    config.set({
        basePath: '',
        frameworks: [ 'jspm', 'jasmine' ],
        plugins: [
            'karma-jspm',
            'karma-jasmine',
            'karma-chrome-launcher'
        ],
        jspm: {
            stripExtension: false,
            config: 'jspm.config.js',
            packages: 'jspm_packages',
            loadFiles: [
                'src/client/**/*.js',
                'src/shared/**/*.js'
            ]
        },
        proxies: {
            '/src': '/base/src',
            '/node_modules': '/base/node_modules',
        },
        preprocessors: {
            'src/client/!(test)/*.js': [ 'jspm' ],
            'src/shared/**/*.js': [ 'jspm' ]
        },
        // optionally, configure the reporter
        coverageReporter: {
            type : 'html',
            dir : process.cwd(),
            subdir: 'coverage'
        },
        reporters: [ 'progress', 'jspm' ],
        port: 9876,
        colors: true,
        debug: false,
        logLevel: config.LOG_INFO,
        autoWatch: false,
        browsers: [ 'Chrome' ],
        singleRun: true,
        concurrency: Infinity
    })
}

sergei-maertens · 2016-06-07T19:20:58Z

I will try it out asap.
On Jun 7, 2016 21:18, "Marceli.no" notifications@github.com wrote:

Updated this pr with a decent release candidate. It passed the unit and
lint tests, and runs just fine on one of my local projects. I've done no
isolated tests, but it should be fine for now.

@maxwellpeterson-wf https://github.com/maxwellpeterson-wf @evanweible-wf
https://github.com/evanweible-wf @sergei-maertens
https://github.com/sergei-maertens Please pull down this pr locally and
let me know if any of you have issues. Couple of things to note:

Since we are using the reporters from "karma-coverage
https://github.com/karma-runner/karma-coverage", all the
"coverageReporter" settings are valid for "karma-jspm" as well, use it the
same way you would with "karma-coverage".

Include "jspm" in reporters, preprocessors, and frameworks.

I had to install "karma-coverage" directly from their github repo
since npm hasn't been updated yet.

I had to install "remap-istanbul
https://github.com/SitePen/remap-istanbul" directly from a fork in
my github since I'm still waiting on this pr
Graceful handling of invalid/non-existent sourceMappingURL SitePen/remap-istanbul#62.

If you see a folder on your "html" report called "no-source-map", it
means that one or more of your files was transpiled without a sourcemap.
You'll need to make sure your jspm sourcemap generator is enabled. (It
should be enabled by default.)

Here's what my karma config file looks like:

module.exports = function( config ) {
config.set( {
basePath: '',
frameworks: [ 'jspm', 'jasmine' ],
plugins: [
'karma-jspm',
'karma-jasmine',
'karma-chrome-launcher',
'karma-coverage'
],
jspm: {
stripExtension: false,
config: "jspm.config.js",
browser: "jspm.browser.js",
loadFiles: [
'src/client//!(bundle|reloader).js',
'src/client/test/!(coverage)//.js',
'src/shared/__/.js',
'app.prod.config.js',
'app.dev.config.js'
]
},
reporters: [ 'progress', 'jspm' ],
preprocessors: {
'src/shared/*/.js': [ 'jspm' ]
},
port: 9876,
colors: true,
logLevel: config.LOG_INFO,
autoWatch: true,
browsers: [ 'Chrome' ],
singleRun: true,
concurrency: Infinity,
coverageReporter: {
type : 'html',
dir : 'src/client/test',
subdir: 'coverage'
}
} )
}

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#147 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AFQ01j8rYsqUk60R6ZDbF3wR9s22Pt0Wks5qJcQYgaJpZM4ISP1c
.

m-a-r-c-e-l-i-n-o · 2016-06-30T15:17:37Z

Any thoughts on this, guys?

aviary-wf · 2016-07-09T15:22:03Z

Raven

Number of Findings: 0

evanweible-wf · 2016-07-11T14:26:14Z

@maxwellpeterson-wf @trentgrover-wf

maxwellpeterson-wf · 2016-07-11T14:41:05Z

This is looking good. @sergei-maertens did you get a chance to try it out?

sergei-maertens · 2016-07-11T14:52:47Z

Oh shuck - I completely forgot about this since we're using our fork at the
moment. I don't see any problems though.
On Jul 11, 2016 4:41 PM, "Max Peterson" notifications@github.com wrote:

This is looking good. @sergei-maertens
https://github.com/sergei-maertens did you get a chance to try it out?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#147 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AFQ01tHfe0f9vbwxXRzYSsYMoaCH-FnDks5qUlYCgaJpZM4ISP1c
.

m-a-r-c-e-l-i-n-o · 2016-07-11T19:18:22Z

@evanweible-wf @maxwellpeterson-wf @trentgrover-wf Hold off on this merge, I'm going to use it in a pretty complex project in the next few days, it'll be a good chance to more thoroughly test and patch this baby, if needed.

maxwellpeterson-wf · 2016-07-11T19:28:45Z

@m-a-r-c-e-l-i-n-o great, thanks!

rmconsole-wf · 2016-08-10T20:01:34Z

@m-a-r-c-e-l-i-n-o This pull request has merge conflicts, please resolve.

cyrixmorten · 2016-09-13T14:10:08Z

@m-a-r-c-e-l-i-n-o Tried out using your branch with your version of remap-istanbul as you described above, and it seems to be almost working.

I get the following error: [reporter-wrapper.coverage]: [TypeError: Cannot read property 'tempDirectory' of undefined]

After uncommenting rimraf.sync(rootConfig.client.jspm.coverage.tempDirectory) in reporter.js, I am able to get a coverage report output.

We have a setup that only does coverage when verifying the whole project, so my suspicion is that the error might occur when coverage is disabled. Haven't dived that deeply into it yet, just wanted to give some feedback :)

m-a-r-c-e-l-i-n-o · 2016-09-13T14:52:16Z

@cyrixmorten Thank you for the feedback, I appreciate it!

After uncommenting rimraf.sync(rootConfig.client.jspm.coverage.tempDirectory) in reporter.js, I am able to get a coverage report output.

You mean it works after you comment that line out? (As oppose to uncommenting, since it should already be uncommented.)
This is the line you are referring to, right?

And I think you are right about this error being thrown only when coverage is disabled, this property would likely not be set if the coverage preprocessor hasn't ran.

cyrixmorten · 2016-09-13T14:59:12Z

Yeah, I meant that i commented it out :-)

Though I might have drawn an early conclusion on to what extend it was working.. I got the coverage console output, but not the files written to coverage folder.

I realised that I had forgotten to add jspm to preprocessors explaining the lack of output files.

This however unfortunately spawned a new wall of errors in the line of:

13 09 2016 16:37:20.449:ERROR [preprocessor.coverage]: { [Error: Line 1: Unexpected token )] lineNumber: 1, description: 'Unexpected token )', index: 17 }
Error: Line 1: Unexpected token )
    at constructError (/Users/cyrixmorten/Development/Schneider/designer/client/node_modules/esprima/esprima.js:2407:21)
    at createError (/Users/cyrixmorten/Development/Schneider/designer/client/node_modules/esprima/esprima.js:2426:17)
    at unexpectedTokenError (/Users/cyrixmorten/Development/Schneider/designer/client/node_modules/esprima/esprima.js:2500:13)
    at throwUnexpectedToken (/Users/cyrixmorten/Development/Schneider/designer/client/node_modules/esprima/esprima.js:2505:15)
    at consumeSemicolon (/Users/cyrixmorten/Development/Schneider/designer/client/node_modules/esprima/esprima.js:2620:13)
    at parseExpressionStatement (/Users/cyrixmorten/Development/Schneider/designer/client/node_modules/esprima/esprima.js:4223:9)
    at parseStatement (/Users/cyrixmorten/Development/Schneider/designer/client/node_modules/esprima/esprima.js:4760:24)
    at parseStatementListItem (/Users/cyrixmorten/Development/Schneider/designer/client/node_modules/esprima/esprima.js:3989:16)
    at parseScriptBody (/Users/cyrixmorten/Development/Schneider/designer/client/node_modules/esprima/esprima.js:5490:25)
    at parseProgram (/Users/cyrixmorten/Development/Schneider/designer/client/node_modules/esprima/esprima.js:5506:16)
    at Object.parse (/Users/cyrixmorten/Development/Schneider/designer/client/node_modules/esprima/esprima.js:5690:23)
    at Object.Instrumenter.instrumentSync (/Users/cyrixmorten/Development/Schneider/designer/client/node_modules/istanbul/lib/instrumenter.js:459:31)
    at SystemJSNodeLoader.CoveragePreprocessor.systemJS.instantiate (/Users/cyrixmorten/Development/Schneider/designer/client/node_modules/karma-jspm/src/preprocessor.js:97:66)
    at /Users/cyrixmorten/Development/Schneider/designer/client/node_modules/jspm/node_modules/systemjs/dist/system.src.js:432:33
    at tryCatchReject (file:///Users/cyrixmorten/Development/Schneider/designer/target-test/client/jspm_packages/system-polyfills.src.js:1188:30)
    at runContinuation1 (file:///Users/cyrixmorten/Development/Schneider/designer/target-test/client/jspm_packages/system-polyfills.src.js:1147:4)
    at Fulfilled.when (file:///Users/cyrixmorten/Development/Schneider/designer/target-test/client/jspm_packages/system-polyfills.src.js:935:4)
    at Pending.run (file:///Users/cyrixmorten/Development/Schneider/designer/target-test/client/jspm_packages/system-polyfills.src.js:826:13)
    at Scheduler._drain (file:///Users/cyrixmorten/Development/Schneider/designer/target-test/client/jspm_packages/system-polyfills.src.js:102:19)
    at Scheduler.drain (file:///Users/cyrixmorten/Development/Schneider/designer/target-test/client/jspm_packages/system-polyfills.src.js:67:9)
    at nextTickCallbackWith0Args (node.js:453:9)
    at process._tickCallback (node.js:382:13)

and

Potentially unhandled rejection [748] TypeError: Cannot read property 'src' of null
    at addToError (/Users/cyrixmorten/Development/Schneider/designer/client/node_modules/jspm/node_modules/systemjs/dist/system.src.js:111:80)
    at linkSetFailed (/Users/cyrixmorten/Development/Schneider/designer/client/node_modules/jspm/node_modules/systemjs/dist/system.src.js:700:21)
    at /Users/cyrixmorten/Development/Schneider/designer/client/node_modules/jspm/node_modules/systemjs/dist/system.src.js:500:9
    at tryCatchReject (file:///Users/cyrixmorten/Development/Schneider/designer/target-test/client/jspm_packages/system-polyfills.src.js:1188:30)
    at runContinuation1 (file:///Users/cyrixmorten/Development/Schneider/designer/target-test/client/jspm_packages/system-polyfills.src.js:1147:4)
    at Rejected.when (file:///Users/cyrixmorten/Development/Schneider/designer/target-test/client/jspm_packages/system-polyfills.src.js:968:4)
    at Pending.run (file:///Users/cyrixmorten/Development/Schneider/designer/target-test/client/jspm_packages/system-polyfills.src.js:826:13)
    at Scheduler._drain (file:///Users/cyrixmorten/Development/Schneider/designer/target-test/client/jspm_packages/system-polyfills.src.js:102:19)
    at Scheduler.drain (file:///Users/cyrixmorten/Development/Schneider/designer/target-test/client/jspm_packages/system-polyfills.src.js:67:9)
    at nextTickCallbackWith0Args (node.js:453:9)
    at process._tickCallback (node.js:382:13)

To be fair it is a fairly big project that I try to test and I am new to the whole world of jspm, so may very well be a configuration problem on my end.

Had it working with our previous setup that was based on npm + browserify.

cyrixmorten · 2016-09-13T15:20:20Z

Eureka!! :-D

Think the problems were entirely on my end. My final discovery was that I had failed to remove all traces of karma-coverage.

After removing it from plugins and reporters, I successfully got a full coverage report with files and everything 👍

As mentioned, we conditionally enable coverage, which now looks like this:

function setupCoverage() {
    if (!options.coverage) {
        console.log(' * coverage: disabled');
        return;
    }
    console.log(' * coverage: enabled');

    // coverage reporter generates the coverage
    defaultConfig.reporters = [ 'progress', 'dots', 'junit', 'jspm'];

    // source files, that you wanna generate coverage for
    // do not include tests or libraries
    // (these files will be instrumented by Istanbul)
    var src = path.join(target.srcFolder, '/**/!(*spec).js');

    defaultConfig.preprocessors[src] = ['jspm'];

    defaultConfig.junitReporter = {
        outputFile : path.join(target.coverageFolder, 'client/unit.xml'),
        suite : ''
    };


    // optionally, configure the reporter 
    defaultConfig.coverageReporter = {
        dir: path.join(target.coverageFolder),
        reporters : [
            { type: 'cobertura', subdir: '.', file: 'cobertura-coverage.xml' }, // (xml format supported by Jenkins)
            { type: 'html', subdir: '.' },
            { type: 'text'},
            { type: 'text-summary'}
        ]
    };
}

With frameworks and plugins as follows:

    frameworks : ['jspm', 'mocha', 'chai-as-promised', 'sinon-chai'],
    plugins: [
        'karma-jspm',
        'karma-chrome-launcher',
        'karma-mocha',
        'karma-chai-as-promised',
        'karma-sinon-chai',
        'karma-junit-reporter',
        'karma-sourcemap-loader',
        'karma-ng-html2js-preprocessor'
    ],

m-a-r-c-e-l-i-n-o · 2016-09-13T15:23:58Z

@cyrixmorten Yay! Glad you resolved it -- it did look like a configuration issue. So that first issue "TypeError: Cannot read property 'tempDirectory' of undefined" is fairly straight forward to address, and I'll see if I can do it today or tomorrow.

cyrixmorten · 2016-09-13T15:34:55Z

@m-a-r-c-e-l-i-n-o Yeah, well actually I did not need to alter anything in karma-jspm after the final correction.

It may however be a nice gesture to throw an error if there are karma-coverage references in the configuration since it apparently does not play well together with this library.

Our code is by the way an angular project of ~4k loc with 107 unit/midway tests.

Just to note that it is clearly capable of handling coverage of more than trivial examples.

…strumentation for systemjs/jspm/transpiler projects, provide new reporter using remap-istanbul and modified adapter for attaching instrumentation

m-a-r-c-e-l-i-n-o · 2016-10-25T21:58:33Z

See #178 for a simpler, likely more efficient solution.

m-a-r-c-e-l-i-n-o mentioned this pull request Apr 29, 2016

Make sure all temp files are removed under all circumstances. CanopyTax/node-jspm-jasmine#17

Closed

maxwellpeterson-wf reviewed May 11, 2016
View reviewed changes

m-a-r-c-e-l-i-n-o force-pushed the master branch from 0f541b8 to 394d5ef Compare May 14, 2016 12:48

m-a-r-c-e-l-i-n-o mentioned this pull request May 16, 2016

Adding event callbacks for "writeReport" and "onExit" functions karma-runner/karma-coverage#228

Merged

m-a-r-c-e-l-i-n-o force-pushed the master branch 3 times, most recently from d0fa726 to b543659 Compare June 7, 2016 19:00

m-a-r-c-e-l-i-n-o force-pushed the master branch from b543659 to 2bf1cb3 Compare July 9, 2016 15:22

m-a-r-c-e-l-i-n-o force-pushed the master branch 4 times, most recently from 13c92f2 to ade90eb Compare July 10, 2016 18:36

rmconsole-wf added the Merge Conflicts label Aug 10, 2016

Release candidate for sustainable coverage approach

5514084

m-a-r-c-e-l-i-n-o force-pushed the master branch from ade90eb to 5514084 Compare September 6, 2016 15:11

rmconsole-wf removed the Merge Conflicts label Sep 6, 2016

malaupa mentioned this pull request Sep 16, 2016

Another approach for code coverage measurement #178

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proof of concept of a more sustainable coverage approach. #147

Proof of concept of a more sustainable coverage approach. #147

m-a-r-c-e-l-i-n-o commented Apr 28, 2016

maxwellpeterson-wf May 11, 2016

m-a-r-c-e-l-i-n-o May 12, 2016

maxwellpeterson-wf commented May 11, 2016

evanweible-wf commented May 11, 2016

m-a-r-c-e-l-i-n-o commented May 12, 2016

sergei-maertens commented Jun 7, 2016

evanweible-wf commented Jun 7, 2016

sergei-maertens commented Jun 7, 2016

m-a-r-c-e-l-i-n-o commented Jun 7, 2016

evanweible-wf commented Jun 7, 2016

m-a-r-c-e-l-i-n-o commented Jun 7, 2016 •

edited

Loading

sergei-maertens commented Jun 7, 2016

m-a-r-c-e-l-i-n-o commented Jun 30, 2016

aviary-wf commented Jul 9, 2016

evanweible-wf commented Jul 11, 2016

maxwellpeterson-wf commented Jul 11, 2016

sergei-maertens commented Jul 11, 2016

m-a-r-c-e-l-i-n-o commented Jul 11, 2016

maxwellpeterson-wf commented Jul 11, 2016

rmconsole-wf commented Aug 10, 2016

cyrixmorten commented Sep 13, 2016

m-a-r-c-e-l-i-n-o commented Sep 13, 2016

cyrixmorten commented Sep 13, 2016

cyrixmorten commented Sep 13, 2016

m-a-r-c-e-l-i-n-o commented Sep 13, 2016

cyrixmorten commented Sep 13, 2016 •

edited

Loading

m-a-r-c-e-l-i-n-o commented Oct 25, 2016

Proof of concept of a more sustainable coverage approach. #147

Are you sure you want to change the base?

Proof of concept of a more sustainable coverage approach. #147

Conversation

m-a-r-c-e-l-i-n-o commented Apr 28, 2016

maxwellpeterson-wf May 11, 2016

Choose a reason for hiding this comment

m-a-r-c-e-l-i-n-o May 12, 2016

Choose a reason for hiding this comment

maxwellpeterson-wf commented May 11, 2016

evanweible-wf commented May 11, 2016

m-a-r-c-e-l-i-n-o commented May 12, 2016

sergei-maertens commented Jun 7, 2016

evanweible-wf commented Jun 7, 2016

sergei-maertens commented Jun 7, 2016

m-a-r-c-e-l-i-n-o commented Jun 7, 2016

evanweible-wf commented Jun 7, 2016

m-a-r-c-e-l-i-n-o commented Jun 7, 2016 • edited Loading

sergei-maertens commented Jun 7, 2016

m-a-r-c-e-l-i-n-o commented Jun 30, 2016

aviary-wf commented Jul 9, 2016

Raven

evanweible-wf commented Jul 11, 2016

maxwellpeterson-wf commented Jul 11, 2016

sergei-maertens commented Jul 11, 2016

m-a-r-c-e-l-i-n-o commented Jul 11, 2016

maxwellpeterson-wf commented Jul 11, 2016

rmconsole-wf commented Aug 10, 2016

cyrixmorten commented Sep 13, 2016

m-a-r-c-e-l-i-n-o commented Sep 13, 2016

cyrixmorten commented Sep 13, 2016

cyrixmorten commented Sep 13, 2016

m-a-r-c-e-l-i-n-o commented Sep 13, 2016

cyrixmorten commented Sep 13, 2016 • edited Loading

m-a-r-c-e-l-i-n-o commented Oct 25, 2016

m-a-r-c-e-l-i-n-o commented Jun 7, 2016 •

edited

Loading

cyrixmorten commented Sep 13, 2016 •

edited

Loading