Skip to content
This repository has been archived by the owner on Dec 8, 2024. It is now read-only.

support jspm System.import #498

Closed
stoffeastrom opened this issue Dec 8, 2015 · 13 comments
Closed

support jspm System.import #498

stoffeastrom opened this issue Dec 8, 2015 · 13 comments

Comments

@stoffeastrom
Copy link

Seems like I can't get coverage when using jspm System.import in node

foo.spec.js

import {Loader as loader} from "jspm";
let System = loader();

describe("Foo", () => {
  let Foo, foo;
  before(() => {
    return System.import("foo").then(m => { Foo = m.Foo; });
  });
  beforeEach(() => {
    foo = new Foo();
  });
  it("should have a `bar`", () => {
    expect(foo.getBar()).to.equal("bar");
  });
});
./node_modules/istanbul/lib/cli.js cover ./node_modules/mocha/bin/_mocha

The tests run but I get the No coverage information was collected, exit without writing coverage information

I guess there isn't a hook for the System.import?

@gotwarlost
Copy link
Owner

No, there is no hook. Would it be possible for you to put up a github repo with the code, package.json and build scripts if any so I can take a look at if it can be supported easily?

Or just give me the contents of package.json so I use the correct deps etc.

@stoffeastrom
Copy link
Author

The repo is located here

See below link for minimal repo

@typhonrt
Copy link

Greets folks... I too spent far too many hours banging my head against this issue. I have created a very concise test repo with bare minimum JSPM setup along with a vanilla ES6 test for comparison:
/~https://github.com/typhonjs-demos-test/istanbul-jspm-coverage-example

Let me know if there is anything I can do to help as it will be great to get a hook or Istanbul to play nice with JSPM tests.

Thanks...

@stoffeastrom
Copy link
Author

Nice. I moved away from using jspm in the test. It's a bit slow but I still think it's a valid use case. Since you have a minimal repo I will remove my link.

@typhonrt
Copy link

@guybedford has chimed in on a possible hook implementation for Istanbul for JSPM / SystemJS in this issue:
systemjs/systemjs#992

I've moved the test case example to here:
/~https://github.com/typhonjs-demos-test/istanbul-jspm-coverage-example

@gotwarlost
Copy link
Owner

Hi guys, is there anything you need me to do here or are you handling this on the other side?

@typhonrt
Copy link

Well.. @gotwarlost I haven't quite figured out where the initial suggestion @guybedford offered fits into the scheme of things. It seems Guy provided an example of how to replace the translate function of SystemJS with one that can interface / be instrumented with Istanbul. I know little about how Istanbul works in this regard. It's possible that one could add this instrumented translate function in the test itself if there is a way to register code w/ Istanbul. I'm sure there is, but perhaps either you or Guy can weigh in on how to proceed. Thanks for taking a look.

@guybedford
Copy link

I've been working on some workflows here, and while the vm.runInThisContext approach would work for SystemJS, it wouldn't be able to run coverage against the untranspiled source of SystemJS which would probably be the desired workflow. The best way to instrument SystemJS is thus just to add a custom translate hook which wraps the source code in instrumentation.

If Istanbul wanted to offer direct support this would be possible by providing an option like { systemLoader: loaderInstance } where it could then do the hooking into the loader itself, but in my workflows so far it works just as well to add a custom translate hook manually.

The problem really just comes down to sharing the wiring approach (and not just mashing tools together and wondering why they don't work!), I'm hoping to write a blog post on it soon.

@typhonrt
Copy link

@guybedford I'm not familiar with writing custom translate hooks for SystemJS. I look forward to your blog post. If you could share here an example of the translate hook that works with Istanbul that would be great.

My general thinking regarding sharing the wiring approach would be to create a github repo with the translate hook that could be pulled in as a dev dependency and imported in the test. This seems like it would work for distributing a standard solution.

@guybedford
Copy link

The translate hook I've used is available at /~https://github.com/guybedford/jspm-test-demo/blob/master/coverage.js#L25. Feel free to create a custom repo for yourself, I don't plan on maintaining anything around this workflow personally beyond just sharing the ideas as I just don't have the bandwidth.

@guybedford
Copy link

Note the above coverage approach is on the original ES6 sources so hits all the harmony edge cases in istanbul without using patches or a fork.

@typhonrt
Copy link

Thanks @guybedford for providing an API example of Istanbul / Mocha / SystemJS integration. I think I've got a good idea on one way forward with what you provided.

To @gotwarlost thanks for reviewing my thoughts below and providing any guidance. Apologies for the verbosity.

My question to @gotwarlost is if there is any way to further instrument Istanbul from within a Mocha test that may use JSPM / SystemJS like the Mocha test I provided / jspm.js. This pertains to when Istanbul is run from the command line with Mocha as the executable js file target for Istanbul.

From examining the Istanbul source for command line execution this seems like it is impossible to further instrument Istanbul from the Mocha executable instance as any instrumentation occurs before Mocha is invoked and there is no way to access Istanbul from a Mocha test in progress. It seems like it's necessary to use the API approach to include JSPM / SystemJS with Istanbul and Mocha like what @guybedford provided.

The reason why I'm inquiring on the command line execution approach is that I have a NPM module typhonjs-npm-scripts-test-mocha that invokes Istanbul / Mocha from the command line as an NPM script. This is fine for the ES6 NPM modules I'm releasing. It would be great of course to be able to instrument JSPM / SystemJS tests from the same arrangement.

I'm looking into how I can add a NPM script which will use the API invocation process to instrument JSPM / SystemJS tests. It seems not as ideal because the testing process is more opaque. IE one will have to declare the JSPM / SystemJS sources to instrument outside of the test and then proceed to load the same sources in the test itself; two separate stages to coordinate rather than containing everything in one declarative location inside the test itself. This may not be so bad as I'm storing metadata in a separate json file (example from a NPM module typhonjs-github-inspect-orgs .npmscriptrc).

So if there was a way to further instrument Istanbul from a command line execution inside of the Mocha test itself that would be ideal as it is more clear to the developer writing the tests.

Perhaps there is a possibility for creating an Istanbul plugin versus invoking the JSPM / SystemJS integration via direct API usage or as @guybedford mentioned previously: "If Istanbul wanted to offer direct support this would be possible by providing an option like { systemLoader: loaderInstance } ". I'm open to helping in developing any standard solution.

Regardless the goal here is to not have a two stage setup that forces a developer to declare JSPM / SystemJS sources loaded in advance and then make sure to load the same sources in the test itself.

@typhonrt
Copy link

After much gnashing of teeth I have a stable Istanbul instrumentation hook published on NPM that can be used from within a Mocha test kicked off by CLI Istanbul usage:

And an all inclusive module including the JSPM / SystemJS hook in addition to transpiling (Babel), docs (ESDoc + plugins for JSPM), ESLint (2+), testing (Mocha / Istanbul), NPM publish / prepublish detection / script runner, coverage upload (Codecov):

I have updated istanbul-jspm-coverage-example giving a concise demo showing coverage of SystemJS loaded code. In particular see TestJSPM.js

And here is the final instrumentation hook for System.translate instrumentIstanbulSystem.js

Tested on JSPM / SystemJS 0.16.x and 0.17.x.... Turns out I had to get a little clever as 0.16.x doesn't output source maps in System.translate (oi @guybedford took hours to figure that out!). Two Istanbul instrumenters are created; one for ES5 and one for ES6+ / ES Modules and instrumentation occurs before invoking the original System.translate function which has the benefit of not needing to use source maps. In the tests CJS and ESM code is covered.

A caveat with CLI Istanbul usage and "remote" instrumentation is that the run-cover / cover command will not fully complete the report with the SystemJS covered code. So there is a particular NPM script available mocha-istanbul-report.js that will run the cover command then follow up with the CLI report command which properly picks up SystemJS instrumented source code.

Up next... Get code coverage of headless browser tests of SystemJS using NightmareJS... Oi that is going to be a nightmare!

@gotwarlost Seemingly this issue can be closed. Thanks for your work on Istanbul!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants