Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Covering code loaded with jsdom and systemjs #529

Closed
unional opened this issue Mar 2, 2017 · 15 comments
Closed

Covering code loaded with jsdom and systemjs #529

unional opened this issue Mar 2, 2017 · 15 comments

Comments

@unional
Copy link

unional commented Mar 2, 2017

Please use the template provided below, when reporting bugs:

Expected Behavior

Able to walk through the code loaded by:

// pseudo code simplified
new Promise(resolve =>{
  jsdom.env({
    scripts: [ 'systemjs' ],
    done(err, win) {
      resolve(win)
    }
  })
}).then(win => {
  const sys = win.SystemJS
  sys.config({ ... })
  return sys;
}).then(async sys => {
  const target = await sys.import('target')
  // do testing
})

Observed Behavior

Not covered (pretty much expected)
Is there a way to configure nyc to trace code like this?

Bonus Points! Code (or Repository) that Reproduces Issue

Forensic Information

Window 2012 R2

node: 7.5.0
npm: 4.0.5

@bcoe
Copy link
Member

bcoe commented Mar 4, 2017

@unional would you be able to provide a minimal repo that would let us dig into jsdom/systemjs support?

I'm sure there are approaches that we can use to support this.

@unional
Copy link
Author

unional commented Mar 4, 2017

Repo: https://github.com/unional/issues/tree/nyc-systemjs

Steps:

git clone https://github.com/unional/issues.git
cd issues
git checkout nyc-systemjs
npm install
npm run coverage

Thanks for looking into this! 🌷

In the repo, you will see foo.js is covered abut boo.js is not.

foo.js is tested without jsdom + systemjs, while boo.js is.

@bcoe
Copy link
Member

bcoe commented Mar 22, 2017

@unional sorry it's taken so long to get to this (put out a new major release of yargs, and have been doing some work around monorepos to try to consolidate some of the work on istanbuljs).

Thanks for providing this reproduction!

@JaKXz
Copy link
Member

JaKXz commented Mar 29, 2017

@unional I took a quick look at your repo and it looks like #176 (comment) might help. Could you try giving that a shot and reporting back?

@unional
Copy link
Author

unional commented Mar 29, 2017

Sure I can give it a try, but what does it solve? It seems like a solution to get coverage with mocha and webpack. But I do get coverage already with ava and nyc. It's just that when loading through jsdom and systemjs the coverage are not recorded.

@unional
Copy link
Author

unional commented Apr 6, 2017

I have created a tool domture that makes testing using jsdom a bit easier.
It using the technique described here thus has the same problem.

I still don't know if this can be fixed because I notice that ava can't pickup the dependency graph either.

What I mean is this:

// src/a.ts
export function foo() { ... }
// src/a.spec.ts
// load `foo` thru jsdom/systemjs

when I run ava in watch mode and the file a.ts is changed, tests in a.spec.ts is not rerun.
Normally it would because ava detects a.spec.ts has dependency of a.ts

@sergueyarellano
Copy link

Same problem here.

Any updates on the issue @unional ?

Any alternative to systemjs for loading amd modules with path, shim configs, etc?
I'm going to try requirejs, but don't know if it's gonna work in node...

@sergueyarellano
Copy link

Finally I tried requirejs in node after struggling with systemjs coverage hooks.

I get lots of Shim config not supported in Node, may or may not work when importing libraries like jquery

Although you can configure the node environment as follows:

const { JSDOM } = require('jsdom');
const dom = new JSDOM(`<!DOCTYPE html>`);
window = dom.window;
document = window.document;

Object.keys(window).forEach(function (key) {
  if (!global[key]) {
    global[key] = window[key];
  }
});

cheers

@unional
Copy link
Author

unional commented Jul 18, 2017

@sergueyarellano what you did is essentially what jsdomify and browser-env does, and is explicitly discouraged by the jsdom team.

@bcoe bcoe added the triaged label Aug 1, 2017
@bcoe
Copy link
Member

bcoe commented Aug 1, 2017

@unional @sergueyarellano me thinks to get this working an additional plugin would be required for jsdom. What you basically want is that when JSDom loads a block of JavaScript, you should hook into this step and run the JavaScript through the istanbul instrumenter -- I think you'd probably be better to do this in an additional helper module rather than nyc itself.

Could be a fun project (and I'd happily link to and document it in nyc and on the istanbul website).

@felixfbecker
Copy link

@unional have you tried what @bcoe suggested? Very interested in something like domture (was about to write it myself when I saw yours) but need coverage support.

@unional
Copy link
Author

unional commented Sep 29, 2017

@felixfbecker have not get the chance to work on @bcoe idea. The idea sounds good. Need to see if there is such a hook and create it if needed. Do you want to give it a try?

@morris
Copy link

morris commented Jan 12, 2019

Here's what I use right now to have coverage on any script run by jsdom. It just maps URLs encountered in vm.runInContext to local files:

const webroot = "public";
const originalRunInContext = vm.runInContext;

vm.runInContext = function(code, context, options) {
  // this should probably only match on localhost, still WIP
  if (options && options.filename && options.filename.match(/^https?:\/\//i)) {
    let filename = url.parse(options.filename).path;
    if (filename.match(/\/$/)) filename += "index.html";
    // this is optional but gives coverage on inline <script> tags
    if (options.lineOffset > 0) {
      filename += "." + options.lineOffset + ".js";
      fs.writeFileSync(path.resolve(path.join(webroot, filename)), code);
    }

    return originalRunInContext.call(vm, code, context, {
      ...options,
      filename: path.resolve(path.join(webroot, filename))
    });
  }
  return originalRunInContext.call(vm, code, context, options);
};

Needs nyc's sourceMap and hookRunInContext set to true. Does not work with --all, though.

@JaKXz JaKXz removed the wontfix label Jan 12, 2019
@istanbuljs istanbuljs deleted a comment from stale bot Jan 12, 2019
@stale
Copy link

stale bot commented Mar 13, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 13, 2019
@stale stale bot closed this as completed Mar 20, 2019
@jfoclpf
Copy link

jfoclpf commented Feb 16, 2020

is this issue solved? How can I cover a file loaded by JSDOM?

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

No branches or pull requests

8 participants