xolvio / rtd

DEPRECATED: The Test Runner for Meteor
164 stars 37 forks source link

Test coverage incorrectly reported for 'action' property in Iron Router #135

Open jrgrafton opened 10 years ago

jrgrafton commented 10 years ago

http://d.pr/i/7z03

As you can see above the coverage for 'action' in routes.js is reported as missing even though it has been executed as part of the selenium acceptance tests.

May it be that Istanbul is unable to capture coverage for 'server' methods in Iron Router?

samhatoum commented 10 years ago

Hey. You want to exclude any 3rd party software from the code coverage

jrgrafton commented 10 years ago

Hey Sam,

Thanks for getting back to me. So I completely agree with the exclusion of third party libraries, although in this case since Iron Router is performing some non trivial 301 redirect it would be great to have it covered.

Is there any way to get Istanbul to cover these additional execution blocks? Maybe push the logic back into a file in /server/?

samhatoum commented 10 years ago

My bad, I didn't really understand the issue. I see you have the file in /app/lib/router.js which means it's being covered. Are you sure you're hitting these lines with selenium? This is either the a bug in the coverage report not coming back in time, or that the code is not hitting it. Can you check with some console.logs and see if the selenium run really touches that part?

jrgrafton commented 10 years ago

Hey Sam,

No worries, I'm pretty new to Meteor so just trying to get my head around exactly to expect myself!

I've just rerun the tests with logging. As you can see here the logs were definitely output during Selenium testing, however the coverage still reports them as not having been executed.

Could it be that Istanbul is simply unable to report coverage for the action function in Iron Router?

EDIT: Maybe this has something to do with the fact that the 301 server redirect is navigating context away from the page before Istanbul can post back code coverage?

samhatoum commented 10 years ago

Quite possibly. The first thing to check is if the code is being instrumented, which I would imagine it is. Could you have a look at .../shortly/production/build/mirror_app/lib/router.js and see if the file has a global coverage object? If it does, it means your last statement is probably true, as the coverage would be being reported but is not being posted back when the redirect happens , which we'd have to patch/fix. But let's be sure first.

jrgrafton commented 10 years ago

Looks like the global coverage object exists.

Seems like it could be quite a difficult fix though if the redirect happens inside an Iron Router server action - interested to hear how this could be resolved!

samhatoum commented 10 years ago

Naaahh, this is JS and it's easy :)

We'd have to patch the iron-router method for the mirror app. So if this is the normal flow in the Iron Router server:

ironRouterServer.doRedirect(<the args>) {
    //do some stuff
}

We would wrap that method by doing this in our setup code:

oldDoRedirect = ironRouterServer.doRedirect;
ironRouterServer.doRedirect = function(<the args>) {
    // post back coverage here
    oldDoRedirect(<the args>);
}

This allows us to insert some steps into the normal flow and that's where we'd do the postback coverage before serving up the 301.

Fancy giving that a try?

samhatoum commented 10 years ago

@cmather you may be mildly interested in this

jrgrafton commented 10 years ago

Well I gave it a quick go, by copying the postCoverage function to the routes.js file I'd expect the first three lines of the action function to be picked up - yet it appears that it's still not registering.

samhatoum commented 10 years ago

Interesting. Next thing to check is of the post back is being received. Perhaps you can look in the network inspector of chrome/Firefox?

jrgrafton commented 10 years ago

Post back seems to be happening but coverage still isn't being captured, strangely enough even when I add it to the exclusion list it still reports no coverage

instrumentationExcludes: ['**/router.js', '**/packages/**', '**/3rd/**', 'fixture.js', 'fixture.coffee'],
samhatoum commented 10 years ago

This is odd. Do you think you could replicate this in an isolated project? That would help me fix it