Dynamically imported code is not covered

[SimonSimCity]

Any idea of how dynamically imported code could be covered? I've now opened an issue on Meteor and asked for a hook to manipulate the code before it's sent to the client. I think this would already help this issue here 😉

See: meteor/meteor#9704

[SimonSimCity]

Another idea that came me just now would be to mock some methods of the https://nodejs.org/api/http.html#http_class_http_serverresponse object, which would go even deeper as the current implementation on Handlers.instrumentClientJs but would be specific to all versions that are compatible with Meteor 1.6.1 ...

[serut]

I don't get how the client fetch the chunk without getting hook by instrumentClientJs. Since that methods is called every times the client fetch anything from the server, it should be instrumented too. Are you sure that's not linked to the library webapp that replaces meteorhacks:picker ?

[SimonSimCity]

@serut because the format which it fetches there is not a normal GET request which requests a full file, but they're bundling import-requests together. I haven't investigated yet in detail how this works, but #68 - I think - is a good start towards letting the system do it's job and this plugin just hooking in.

This way, we just need to keep up with changes done in the dynamic-import client and don't need to worry about the server implementation.

[serut]

I've tryed to merge #68 but it breaks the client coverage. I will publish my sample application on serut/meteor-coverage-app-example with the new package first, then we will see how things can be done to fix this issue.

Don't you think that's more simple to ask babel to instrument the client JS for us ?

[SimonSimCity]

@serut that would be the easiest of all - but then we should also have it for the code on the server-side. To get https://github.com/istanbuljs/babel-plugin-istanbul coupled in here would be awesome and most-likely reduce the maintenance of this package a lot.

[serut]

Yes but we still need to collect source maps inside the package, otherwise the remap source code <-> executed code will stop working.

[SimonSimCity]

Could you please give me more details about this? As of what I see the process is like following:

1. The code (with injected istanbul-instrumenter) is loaded
2. Once the code is executed it saves something in a global variable
3. On action this library checks what should be ignored and saves the rest

As I understand it, the first action here (which this lib currently does by hooking into runInThisContext, which we fought for for so long - see istanbuljs/istanbuljs#99 - and the instrumentClientJs method for the client) could be replaced by simply adding this babel plugin, is that correct?

The rest should still work on if we push these hooks out and replace them by Babel, or am I missing out on something?

[serut]

Yeah it instruments the executed code and save it in the global variable (on client and server, then we transfer client coverage on server..). On report generation (except coverage, which export the object as it is), it remaps the executed code using source map to generate a report with coverage associated with the source code.

However, I don't find the line that does the remap job.

If we let babel instrument the code, do we still register source-map from server code ? On the client side, we can still let the instrumentClientJs behavior to check if there is a .map near the .js file

On action this library checks what should be ignored and saves the rest

We have two filters: what we should instrument, and what we should see in the exported report (we can exclude from report). Babel will instrument everything not in node_modules if I'm right.

[SimonSimCity]

When I read this (https://github.com/istanbuljs/babel-plugin-istanbul#source-maps) it sounds like the babel plugin does the remapping for you. This yields both for client and server, since both of them are compiled by babel.

Since the node_modules folder is not (yet) ran through babel, it won't get included in the coverage; but it's actually just a matter of time until meteor/meteor-feature-requests#6 will get resolved.

The babel plugin also allows for excluding or including files (see https://github.com/istanbuljs/babel-plugin-istanbul#ignoring-files) which we could take the current configuration for.

I'll try soon if we can't use the babel plugin here. If it really works as I thought, the changes on the Meteor core (meteor/meteor#9281) and the pushing for the changes on the istanbuljs library (istanbuljs/istanbuljs#99) wouldn't have been necessary ...

[serut]

I let you test and give me some feedback if there is some issue, but it should be fine now.