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

initial implementation of "functional" interface #3399

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
170 changes: 170 additions & 0 deletions lib/behavior.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
'use strict';

/**
* Built-in behaviors.
* These provide hooks into modifying Mocha's behavior for different use cases.
*/

var utils = require('./utils');

/**
* Default Mocha behavior
*/
exports.Default = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename this from Default to something else?

  • It is a little confusing with es6 default modules.
  • Whenever we want to change the default behavior to another one, we have to refactor 2 places now.
  • A semantic name indicating what it does is more meaningful IMO. noCtxAsArg or something in this context (pun intended 😄 ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, probably a good idea to rename this, or just use module.exports and put the Functional behavior within the func interface... what do you think of that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it Funtional is meaningful? How about Contextual?
I'm not sure that it is functional thing.

Suite: {
/**
* Runs the suite; called by `Suite.create()`. Calls its callback fn
* with the Suite as its context.
* @this {Suite}
*/
run: function() {
// FUTURE: async suites
this.fn.apply(this);
}
},
Runnable: {
/**
* Determines whether or not we should provide a nodeback parameter and
* expect it to be called
* @this {Runnable}
* @returns {boolean} `true` if we should run the test fn with a callback
*/
shouldRunWithCallback: function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, IMO, this is too specific.

What may be better is supporting a function which answer the question "what arguments should we pass to the execution function?", and another "what context should the execution function run with?"

Obviously we're going to need to document this specification...

return Boolean(this.fn && this.fn.length);
},
/**
* Runs the Runnable synchronously, or potentially returning a Promise
* @param {Function} done - Callback
*/
run: function(done) {
var result = this.fn.call(this.ctx);
if (result && typeof result.then === 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this code is just moved, but if we wrap the result in Promise.resolve(result).then(...) instead of doing this check, we'll always have the returned value treated as a promise, without the need for the extra hassle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would require adding a Promise polyfill to the production bundle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I'm not necessarily offering an opinion on that; I'm just telling you why Mocha historically does it this way)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually really annoyed about IE11/ES5, so maybe we can just write Mocha in ES6 going forward and pipe the distfile thru Babel...

this.resetTimeout();
result.then(
function() {
done();
// Return null so libraries like bluebird do not warn about
// subsequently constructed Promises.
return null;
},
function(reason) {
done(
reason || new Error('Promise rejected with no or falsy reason')
);
}
);
} else {
if (this.asyncOnly) {
return done(
new Error(
'--async-only option in use without declaring `done()` or returning a promise'
)
);
}
done();
}
},
/**
* Runs the Runnable, passing a nodeback function, which must be called to
* complete the test
* @param {Function} done - Callback
*/
runWithCallback: function(done) {
var result = this.fn.call(this.ctx, function(err) {
if (
err instanceof Error ||
Object.prototype.toString.call(err) === '[object Error]'
) {
return done(err);
}
if (err) {
if (Object.prototype.toString.call(err) === '[object Object]') {
return done(
new Error('done() invoked with non-Error: ' + JSON.stringify(err))
);
}
return done(new Error('done() invoked with non-Error: ' + err));
}
if (result && utils.isPromise(result)) {
return done(
new Error(
'Resolution method is overspecified. Specify a callback *or* return a Promise; not both.'
)
);
}
done();
});
}
}
};

/**
* Provides a test context in a functional manner for use with
* lambdas/arrow functions.
* All Runnables must either return a promise or call `runnable.done()`, where
* `runnable` is the first parameter to the Runnable's callback (`fn`)
*/
exports.Functional = {
Suite: {
/**
* Runs the Suite. Calls its `fn` with NO context and the suite itself
* as the first parameter.
* @this {Suite}
*/
run: function(opts) {
this.fn.call(null, this);
}
},
Runnable: {
/**
* Determines whether or not we should provide a nodeback parameter and
* expect it to be called; always false
* @this {Runnable}
* @returns false
*/
shouldRunWithCallback: function() {
return false;
},

/**
* Runs the Runnable expecting a call to ctx.done() or a Promise
* @param {Function} done - Callback
*/
run: function(done) {
this.ctx.done = function(err) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't love this monkeypatching; Context should have a method which accepts a callback and assigns it to its own done method.

if (
err instanceof Error ||
Object.prototype.toString.call(err) === '[object Error]'
) {
return done(err);
}
if (err) {
if (Object.prototype.toString.call(err) === '[object Object]') {
return done(
new Error('done() invoked with non-Error: ' + JSON.stringify(err))
);
}
return done(new Error('done() invoked with non-Error: ' + err));
}
done();
};
var result = this.fn.call(null, this.ctx);
if (result && typeof result.then === 'function') {
this.resetTimeout();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this line should ve moved above if statement.
Because tests just exit if test.done() is not called if synchronous tests. Also we should fix the error message for func ui.

result.then(
function() {
done();
// Return null so libraries like bluebird do not warn about
// subsequently constructed Promises.
return null;
},
function(reason) {
done(
reason || new Error('Promise rejected with no or falsy reason')
);
}
);
}
}
}
};
7 changes: 4 additions & 3 deletions lib/interfaces/common.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ module.exports = function(suites, context, mocha) {
*/
runWithSuite: function runWithSuite(suite) {
return function run() {
suite.run();
suite.runIfRoot();
};
},

Expand Down Expand Up @@ -104,12 +104,13 @@ module.exports = function(suites, context, mocha) {
var suite = Suite.create(suites[0], opts.title);
suite.pending = Boolean(opts.pending);
suite.file = opts.file;
suite.fn = opts.fn;
suites.unshift(suite);
if (opts.isOnly) {
suite.parent._onlySuites = suite.parent._onlySuites.concat(suite);
suite.appendExclusiveToParent();
}
if (typeof opts.fn === 'function') {
opts.fn.call(suite);
suite.callBehavior('run', opts);
suites.shift();
} else if (typeof opts.fn === 'undefined' && !suite.pending) {
throw new Error(
Expand Down
24 changes: 24 additions & 0 deletions lib/interfaces/func.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
'use strict';

var bddInterface = require('./bdd');
var behavior = require('../behavior');

/**
* Functional BDD-style interface:
*
* describe('Array', () => {
* describe('#indexOf()', () => {
* it('should return -1 when not present', () => {
* // ...
* });
* it('passes ctx as param', test => {})
* it('uses async as second param , )
* });
* });
*
* @param {Suite} suite Root suite.
*/
module.exports = function funcInterface(suite) {
suite.behavior(behavior.Functional);
bddInterface(suite);
};
1 change: 1 addition & 0 deletions lib/interfaces/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@ exports.bdd = require('./bdd');
exports.tdd = require('./tdd');
exports.qunit = require('./qunit');
exports.exports = require('./exports');
exports.func = require('./func');
Loading