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

WIP: use dynamic import to load es module tests. #3253

Closed
wants to merge 8 commits into from
7 changes: 7 additions & 0 deletions bin/_mocha → bin/_mocha.js
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ program
.option('-w, --watch', 'watch files for changes')
.option('--check-leaks', 'check for global variable leaks')
.option('--full-trace', 'display the full stack trace')
.option('--es-modules', 'load all test files as ECMAScript Modules')
.option('--compilers <ext>:<module>,...', 'use the given module(s) to compile files', list, [])
.option('--debug-brk', "enable node's debugger breaking on the first line")
.option('--globals <names>', 'allow the given comma-delimited global [names]', list, [])
Expand Down Expand Up @@ -399,6 +400,12 @@ if (program.fullTrace) {
mocha.fullTrace();
}

// --es-modules

if (program.esModules) {
mocha.esModules();
}

// --growl

if (program.growl) {
Expand Down
3 changes: 2 additions & 1 deletion bin/mocha
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
const spawn = require('child_process').spawn;
const path = require('path');
const getOptions = require('./options');
const args = [path.join(__dirname, '_mocha')];
const args = [path.join(__dirname, '_mocha.js')];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this necessary?


// Load mocha.opts into process.argv
// Must be loaded here to handle node-specific options
Expand Down Expand Up @@ -38,6 +38,7 @@ process.argv.slice(2).forEach(arg => {
break;
case '--gc-global':
case '--es_staging':
case '--experimental-modules':
case '--no-deprecation':
case '--no-warnings':
case '--prof':
Expand Down
62 changes: 58 additions & 4 deletions lib/mocha.js
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,34 @@ Mocha.prototype.loadFiles = function (fn) {
fn && fn();
};

/**
* Load registered files via es6 dynamic imports.
*
* Note: `eval` is used in this function or else mocha will not
* compile on older node versions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand where this is supposed to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new import() operator allows ECMAScript Modules to be loaded dynamically. The hope was that tests written as modules (i.e. tests including import obj from './script.js' statements) can be loaded into Mocha using dynamic imports without having to first use a transpiler to convert ECMAScript Modules into common js modules.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, but I'm unclear on why we're removing old versions of Node.js from the matrix.

Current support for modules in node is experimental. It is likely that mocha will want to wait untill node starts properly supporting modules before merging this pr.

if we're going to break backwards compatibility, then it'd have to wait until the oldest version of Node.js that supports modules is no longer maintained. that'll be a few years. so, that's why I'm trying to understand what I'm supposed to do with this...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't mean to add that change to the pull request, I will revert it.

The idea was that mocha could add a new CLI flag which would cause mocha to use the new import() instead of require() to load test files.

This new CLI flag could only be used on the most recent versions of node but mocha would work normally if this flag was not set.

The tests fail because I added a test for the new flag and travis/appveyor runs this test on old versions of node, I didn't know how to work around that.

*
* @api private
*/
Mocha.prototype.dynamicallyImportFiles = function (fn) {
var self = this;
var suite = this.suite;
Promise
.all(this.files.map(function (file) {
file = path.resolve(file);
suite.emit('pre-require', global, file, self);
return eval('import(file)')
.then(function (module) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably shouldn't use module

suite.emit('require', module, file, self);
suite.emit('post-require', global, file, self);
return module;
});
}))
.then(function() {
fn();
})
.catch(fn);
};

/**
* Enable growl support.
*
Expand Down Expand Up @@ -334,6 +362,17 @@ Mocha.prototype.fullTrace = function () {
return this;
};

/**
* Use dynamic imports to load tests as es modules.
*
* @return {Mocha}
* @api public
*/
Mocha.prototype.esModules = function () {
this.options.esModules = true;
return this;
};

/**
* Enable growl support.
*
Expand Down Expand Up @@ -532,9 +571,6 @@ Mocha.prototype.forbidPending = function () {
* @return {Runner}
*/
Mocha.prototype.run = function (fn) {
if (this.files.length) {
this.loadFiles();
}
var suite = this.suite;
var options = this.options;
options.files = this.files;
Expand Down Expand Up @@ -569,5 +605,23 @@ Mocha.prototype.run = function (fn) {
}
}

return runner.run(done);
if (this.files.length) {
if (this.options.esModules) {
this.dynamicallyImportFiles(function (error) {
if (error) {
console.log(error);
done(1);
} else {
runner.run(done);
}
});
} else {
this.loadFiles();
runner.run(done);
}
} else {
runner.run(done);
}

return runner;
};
55 changes: 55 additions & 0 deletions test/integration/es-modules.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
'use strict';

var run = require('./helpers').runMocha;
var assert = require('assert');

describe('esModules', function () {
describe('loading of es module tests', function () {
this.timeout(1000);
it('should load tests which use named exports', function (done) {
run('es-modules/named-export.fixture.mjs', ['--es-modules', '--experimental-modules', '--harmony-dynamic-import'], function (err, res) {
if (err) {
done(err);
return;
}
assert.equal(res.pending, 0);
assert.equal(res.passing, 1);
assert.equal(res.failing, 0);
assert.equal(res.code, 0);
done();
});
});

it('should load tests which use default exports', function (done) {
this.timeout(1000);
run('es-modules/default-export.fixture.mjs', ['--es-modules', '--experimental-modules', '--harmony-dynamic-import'], function (err, res) {
if (err) {
done(err);
return;
}
assert.equal(res.pending, 0);
assert.equal(res.passing, 1);
assert.equal(res.failing, 0);
assert.equal(res.code, 0);
done();
});
});
});

describe('loading of cjs tests when running with --experimental-modules and --dynamic-import', function () {
it('should load cjs test', function (done) {
this.timeout(1000);
run('es-modules/cjs-test.fixture.js', ['--es-modules', '--experimental-modules', '--harmony-dynamic-import'], function (err, res) {
if (err) {
done(err);
return;
}
assert.equal(res.pending, 0);
assert.equal(res.passing, 1);
assert.equal(res.failing, 0);
assert.equal(res.code, 0);
done();
});
});
});
});
3 changes: 3 additions & 0 deletions test/integration/fixtures/es-modules/cjs-module.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
'use strict';

exports.cjs = 'cjs';
8 changes: 8 additions & 0 deletions test/integration/fixtures/es-modules/cjs-test.fixture.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
var cjs = require('./cjs-module').cjs;


describe('testing common js require', function () {
it('should be able to require cjs modules', function () {
assert(cjs, 'cjs');
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import double from './module.mjs'

describe('testing imported function', function () {
it('imported value should double its argument', function () {
assert(double(5), 10);
});
});
3 changes: 3 additions & 0 deletions test/integration/fixtures/es-modules/module.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export const double = x => 2*x;

export default double;
7 changes: 7 additions & 0 deletions test/integration/fixtures/es-modules/named-export.fixture.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { double } from './module.mjs'

describe('testing imported function', function () {
it('imported value should double its argument', function () {
assert(double(5), 10);
});
});
13 changes: 13 additions & 0 deletions test/unit/mocha.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,19 @@ describe('Mocha', function () {
});
});

describe('.es-modules()', function () {
it('should set the es-modules option to true', function () {
var mocha = new Mocha(blankOpts);
mocha.esModules();
expect(mocha.options.esModules).to.equal(true);
});

it('should be chainable', function () {
var mocha = new Mocha(blankOpts);
expect(mocha.esModules()).to.equal(mocha);
});
});

describe('.growl()', function () {
it('should set the growl option to true', function () {
var mocha = new Mocha(blankOpts);
Expand Down