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

Open
wants to merge 1 commit into
base: master
from

Conversation

@boneskull
Copy link
Member

boneskull commented May 30, 2018

This is an initial attempt at a "functional" interface; those of us who prefer using arrow functions will hopefully appreciate this. I've called it the func UI; here's how it looks:

Usage

'use strict';

describe('behavior', suite => {
  // just like this.timeout()
  suite.timeout(200);

  it('should work synchronously', test => {
    expect(1 + 1, 'to be', 2);
    expect(2 + 2, 'to be', 4);
    // test.done() must be called unless a Promise is returned
    test.done();
  });

  it('should work asynchronously', test => {
    expect(1 - 1, 'to be', 0);
    expect(2 - 1, 'to be', 1);
    process.nextTick(function() {
      test.done();
    });
  });

  it('should work with a Promise', async () => {
    expect(1 - 1, 'to be', 0);
    expect(2 - 1, 'to be', 1);
    // test.done() needn't be called
  });

  it('should work with context methods', async test => {
    test.timeout(400);
    expect(1 - 1, 'to be', 0);
    expect(2 - 1, 'to be', 1);
  });
  
  afterEach(hook => {
    // hooks have same behavior
    hook.timeout(200);
    hook.done();
  });

  afterEach(async () {
    // and also support Promises
  });
});

Implementation Details

Mocha makes assumptions about how Runnables (Tests, Hooks) are executed; what function context they are called with, and what parameters. To address this, I needed to decouple some parts of Mocha's core.

So, I've introduced a new concept: the Behavior. We can think of this as a collection of mixins which may apply (currently) to Suites and Runnables. These mixins introduce alternative behavior for certain methods within those objects. This is not the mythical plugin API, but it could be a step in that direction.

There are currently a select few places where the Behaviors are delegated to, and I've broken out the default functionality into a Behavior (the Default one, fwiw).

Due to the way Mocha's Suites configure their children (Suites, Hooks, and Tests), in which all children "inherit" the configuration of their parent Suite, an Interface (bdd, tdd, qunit, etc.) only need configure the Behavior on the Root Suite. This configuration propagates throughout all Suites and children thereof.

This means an Interface can provide an alternate Behavior. It's not the only way to provide an alternate Behavior, but seems like a reasonable one; see the simplicity of lib/interfaces/func.js. (Maybe the interface is where the Functional Behavior should live?)

A Behavior may have one or more methods and needn't implement all of them. The default methods will always be present, and if multiple behaviors are configured, then the last one wins (think of how Object.assign() works).

Ultimately, I'm not married to this API. I'm open to considering an event-driven model, as long as the scope doesn't blow up. But once it's merged, we are married to it, so it behooves us to be confident about what we're doing.

See #1856, #2657, probably others.

@boneskull boneskull added the feature label May 30, 2018
@boneskull

This comment has been minimized.

Copy link
Member Author

boneskull commented May 30, 2018

lol, guess I can't use Object.assign... I'll fix that.

@boneskull

This comment has been minimized.

Copy link
Member Author

boneskull commented May 30, 2018

this needs

  • docs
  • an integration test to see what happens when e.g. test.done() is called in a Promise-returning Test
  • more unit tests, probably
@wavebeem

This comment has been minimized.

Copy link

wavebeem commented May 30, 2018

imo this is exactly what i would expect an arrow-func-centric Mocha to look like!

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 30, 2018

Coverage Status

Coverage decreased (-0.6%) to 89.381% when pulling b96abc9 on experimental/bdd2 into 0d95e3f on master.

@eight04

This comment has been minimized.

Copy link

eight04 commented May 30, 2018

Is it possible to omit explicit ctx.done() for sync tests/hooks? Suppose that the test is sync if the return value is not a promise? If ctx.done() is required for sync tests, all existed tests have to be modified to switch from bdd to func:

describe("...", () => {
-  it("...", () => {
+  it("...", t => {
    // some asserts...
+    t.done();
  });
});
@boneskull

This comment has been minimized.

Copy link
Member Author

boneskull commented May 31, 2018

if we didn’t require it then how would we know a test is async? if we supported a test, done signature, then every async test that didn’t need to use methods on test would have test anyway.

probably just as effective to useasync keyword instead of test.done() ..

@wavebeem

This comment has been minimized.

Copy link

wavebeem commented May 31, 2018

@eight04

This comment has been minimized.

Copy link

eight04 commented May 31, 2018

Something like this?

describe("...", () => {
  it("...", t => {
    t.async(true);
    setTimeout(t.done, 200);
  });
});
Copy link
Member

Bamieh left a comment

I really love this feature. I still did not figure out how the done parameter is being passed to the tests, I do not see the implementation anywhere.

/**
* Default Mocha behavior
*/
exports.Default = {

This comment has been minimized.

Copy link
@Bamieh

Bamieh May 31, 2018

Member

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 😄 ).

This comment has been minimized.

Copy link
@boneskull

boneskull May 31, 2018

Author Member

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?

This comment has been minimized.

Copy link
@outsideris

outsideris Jun 2, 2018

Member

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

*/
run: function(done) {
var result = this.fn.call(this.ctx);
if (result && typeof result.then === 'function') {

This comment has been minimized.

Copy link
@Bamieh

Bamieh May 31, 2018

Member

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.

This comment has been minimized.

Copy link
@boneskull

boneskull May 31, 2018

Author Member

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

This comment has been minimized.

Copy link
@boneskull

boneskull May 31, 2018

Author Member

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

This comment has been minimized.

Copy link
@boneskull

boneskull May 31, 2018

Author Member

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...

* @private
* @returns {*} Whatever the behavior method returns
*/
Runnable.prototype.callBehavior = function(name) {

This comment has been minimized.

Copy link
@Bamieh

Bamieh May 31, 2018

Member

not sure about backward compatibility but can't we use (name, ...args)?

This comment has been minimized.

Copy link
@boneskull

boneskull May 31, 2018

Author Member

No, Mocha is written in ES5 😄

@@ -0,0 +1,43 @@
'use strict';

var Promise = require('es6-promise');

This comment has been minimized.

Copy link
@Bamieh

Bamieh May 31, 2018

Member

why is this needed? Promises are supported for all node versions we support (anything above 4)

This comment has been minimized.

Copy link
@boneskull

boneskull May 31, 2018

Author Member

IE11

This comment has been minimized.

Copy link
@boneskull

boneskull May 31, 2018

Author Member

though, I don't think this test actually runs in the browser. it should.

@Bamieh

This comment has been minimized.

Copy link
Member

Bamieh commented May 31, 2018

I wonder if I can use this feature to pass a client to the functions (wd client for native testing in my projects).

@boneskull

This comment has been minimized.

Copy link
Member Author

boneskull commented May 31, 2018

@wavebeem We do check for a Promise return value, but not all async behavior is Promise-based.

@Bamieh I'm not entirely sure what you're asking, but if I had to guess, it's here. We call shouldRunWithCallback to determine which of the two methods (run or runWithCallback) we call. Mind you, this is not applicable to the func interface, which (currently) will always pass a single parameter.

@eight04 Is there a precedent for that--does another test framework do it that way? I'm basing my implementation on stuff like tape, which requires t.end() (or t.plan(), which we can't support) to be called unless a Promise is returned.

This is why I mentioned it'd actually be easier to use the async keyword for otherwise-synchronous tests:

it('should synchronously assert', t => {
  assert(true);
  t.done(); // might as well alias t.end() to this if we do it
});

is effectively:

it('should synchronously assert', async t => {
  assert(true);
});

FWIW, the Jasmine/Jest strategy seems to actually make timers Promise-returning or synchronous (not sure which) by monkeypatching them. I don't want that to be a choice Mocha makes for its users; libraries like Sinon can be added to make timers synchronous, if needed.

* @this {Runnable}
* @returns {boolean} `true` if we should run the test fn with a callback
*/
shouldRunWithCallback: function() {

This comment has been minimized.

Copy link
@boneskull

boneskull May 31, 2018

Author Member

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...

@eight04

This comment has been minimized.

Copy link

eight04 commented Jun 1, 2018

Is there a precedent for that--does another test framework do it that way?

No. Actually, I think (test, done) is better. It matches the old interface nicely.

The original purpose of passing the context as the first argument is to solve the problem that users can't access .skip(), .timeout() when using arrow functions. I don't think people would like to change all tests into async function just to gain the ability to access .skip(), .timeout() for maybe one or two sub-tests.

I prefer (test, done). Reasons:

  1. All existed sync tests don't have to be modified.
  2. All existed promise-based async tests don't have to be modified.
  3. All existed callback-based async tests just need to change the signature. (done -> (t, done))
  4. arrow-mocha users can migrate to the new UI without modification.

If we still want to keep async as the default, it is probably worth adding an autoend option that would automatically done() the test when the return value is not a promise, just like current mocha.

Copy link
Member

outsideris left a comment

I like this approach.

afterEach(function() {
return Promise.resolve();
});
});

This comment has been minimized.

Copy link
@outsideris

outsideris Jun 2, 2018

Member

These tests don't be run.
We should add it in package-scripts.js like:

func: {
  script: test('func', '--ui func test/interfaces/func.spec'),
  description: 'Run Node.js Functional interface tests',
  hiddenFromHelp: true
},
/**
* Default Mocha behavior
*/
exports.Default = {

This comment has been minimized.

Copy link
@outsideris

outsideris Jun 2, 2018

Member

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

};
var result = this.fn.call(null, this.ctx);
if (result && typeof result.then === 'function') {
this.resetTimeout();

This comment has been minimized.

Copy link
@outsideris

outsideris Jun 2, 2018

Member

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.

@plroebuck plroebuck referenced this pull request Sep 24, 2018
4 of 4 tasks complete
@boneskull

This comment has been minimized.

Copy link
Member Author

boneskull commented Dec 11, 2018

This is pretty darn stale. I still like the idea, but will need more iteration after v6.

@boneskull

This comment has been minimized.

Copy link
Member Author

boneskull commented Dec 11, 2018

@outsideris I think I agree that Functional is a misnomer. Contextless might be more appropriate... though it's a mouthful.

* @param {Function} done - Callback
*/
run: function(done) {
this.ctx.done = function(err) {

This comment has been minimized.

Copy link
@boneskull

boneskull Dec 11, 2018

Author Member

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

@noelmace

This comment has been minimized.

Copy link

noelmace commented Aug 18, 2019

@boneskull do you think there is a chance you work on this approach again? Could you explain why it was postponed / what's currently blocking it? I would be happy to help if I can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.