Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

run-with-cover should hook to `vm.runInThisContext` and `vm.hookCreateScript` as well #23

Closed
millermedeiros opened this Issue · 4 comments

2 participants

@millermedeiros

RequireJS uses vm.runInThisContext to evaluate the AMD modules when running on node.js. I think the cover command should add hooks to runInThisContext and createScript automatically or at least have an easy way to set it.

Since transformFn isn't exposed to other modules I ended up editing istanbul/lib/hook.js directly. Not the most elegant solution but worked on my scenario (loading specs and source code with RequireJS):

diff --git a/node_modules/istanbul/lib/command/common/run-with-cover.js b/node_modules/istanbul/lib/command/common/run-with-cover.js
index a9e6e88..b43726d 100644
--- a/node_modules/istanbul/lib/command/common/run-with-cover.js
+++ b/node_modules/istanbul/lib/command/common/run-with-cover.js
@@ -1,3 +1,6 @@
+// this file was edited by Miller Medeiros to support RequireJS on node.js
+// changes are marked with [mm]
+
 /*
  Copyright (c) 2012, Yahoo! Inc.  All rights reserved.
  Copyrights licensed under the New BSD License. See the accompanying LICENSE file for terms.
@@ -124,6 +127,11 @@ function run(args, commandName, enableHooks, callback) {
                 if (opts['self-test']) {
                     hook.unloadRequireCache(matchFn);
                 }
+
+                // ---------- added [mm] -------------------- //
+                hook.hookRunInThisContext(matchFn, transformer, hookOpts);
+                // ------------------------------------------ //
+
                 hook.hookRequire(matchFn, transformer, hookOpts);
                 process.once('exit', function () {
                     var file = path.resolve(reportingDir, 'coverage.json'),
@@ -161,3 +169,4 @@ module.exports = {
     run: run,
     usage: usage
 };
+
diff --git a/node_modules/istanbul/lib/hook.js b/node_modules/istanbul/lib/hook.js
index a22a30a..f5a28f6 100644
--- a/node_modules/istanbul/lib/hook.js
+++ b/node_modules/istanbul/lib/hook.js
@@ -1,3 +1,7 @@
+// this file was edited by Miller Medeiros to support RequireJS on node.js
+// changes are marked with [mm]
+
+
 /*
  Copyright (c) 2012, Yahoo! Inc.  All rights reserved.
  Copyrights licensed under the New BSD License. See the accompanying LICENSE file for terms.
@@ -130,6 +134,21 @@ function hookCreateScript(matcher, transformer, opts) {
     };
 }

+// ======== edited [mm] ======== //
+
+var originalRunInThisContext = vm.runInThisContext;
+
+function hookRunInThisContext(matcher, transformer, opts) {
+    opts = opts || {};
+    var fn = transformFn(matcher, transformer, opts.verbose);
+    vm.runInThisContext = function (code, file) {
+        var ret = fn(code, file);
+        return originalRunInThisContext(ret.code, file);
+    };
+}
+
+// ========================== //
+
 /**
  * unhooks vm.createScript, restoring it to its original state.
  * @method unhookCreateScript
@@ -142,6 +161,7 @@ function unhookCreateScript() {
 module.exports = {
     hookRequire: hookRequire,
     unhookRequire: unhookRequire,
+    hookRunInThisContext : hookRunInThisContext, // added [mm]
     hookCreateScript: hookCreateScript,
     unhookCreateScript: unhookCreateScript,
     unloadRequireCache: unloadRequireCache

Not doing a pull request since I'm focused on other tasks and don't have time to write the tests. Just a FYI that other hooks might be important on different scenarios.

I have it working on the amd-utils travis build, so it can be used as a reference, see tests/runner.js and tests/spec folder for details about my tests: https://github.com/millermedeiros/amd-utils/blob/master/tests/ (source code and tests are written in AMD and are running on node.js and browser).

Cheers.

@gotwarlost
Owner

Cool. Since you have a working implementation, it should be easy to put it back in istanbul with tests etc. I'm going to be relatively busy this week so will get to it by next weekend.

Sounds like you are not blocked right now, so this should be ok?

Let me know.

@millermedeiros

@gotwarlost I'm not blocked, moved on to other tasks. In fact it was very easy easy to make it work, took me under 30min. I thought it was going to be impossible or that I would need to call all the coverage methods manually - which seems to require a lot of boilerplate.

Istanbul is a very nice project and it already helped me to find issues on my own code. Keep the good work. Cheers.

@gotwarlost
Owner

Thank you and keep reporting the issues you find. It's people like you that help make the product better! I will add your name to the contributors list once I port these changes.

Thanks again.

@millermedeiros millermedeiros referenced this issue from a commit in millermedeiros/istanbul
@millermedeiros millermedeiros add hook.hookRunInThisContext and automatically call it on run-with-c…
…over for RequireJS support. see #23
1c6a0e1
@gotwarlost
Owner

Added a switch called --hook-run-in-context to the cover and test commands.

  --hook-run-in-context
          hook vm.runInThisContext in addition to require (supports
          RequireJS), defaults to false

Fix available in v0.1.27. Please let me know if it works for you.

@gotwarlost gotwarlost closed this
@kirill-konshin kirill-konshin referenced this issue in pocesar/grunt-mocha-istanbul
Closed

Allow to pass --hook-run-in-context #23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.