RFC: syntax sugar for mocha tests calling multiple async functions

rfc.js

// Original code
it('does something complex', function(done) {
  async.waterfall(
    [
      function setupSchema(cb) {
        db.setupSchema(/*...*/, cb);
      },
      function createTestUser(cb) {
        db.createUser({ name: 'a user name' }, cb);
      }
      function act(data, cb) {
        this.data = data;
        db.users.edit(this.data.UserId, { name: 'new name' }, cb);
      },
      function assert(cb) {
        db.users.findById(this.data.userId, function(user) {
          expect(user.name).to.equal('new name');
          cb();
        }
      }
    ],
    done);
});

// Proposed API:
it('does something complex', [
  function setupSchema(cb) {
    db.setupSchema(/*...*/, cb);
  },
  function createTestUser(cb) {
    db.createUser({ name: 'a user name' }, cb);
  }
  function act(data, cb) {
    this.data = data;
    db.users.edit(this.data.UserId, { name: 'new name' }, cb);
  },
  function fetchNewData(cb) {
    db.users.findById(this.data.userId, cb);
  },
  function assert(user, cb) {
    expect(user.name).to.equal('new name');
    cb();
  }
]);

rmg: where are the setupSchema, createTestUser, etc called? Are the names predefined?
ritch: I'm pretty sure those are just for the sake of naming the functions. Not required, just nice to do.

Ritchie is correct, the names are not required. It's just a good practice to avoid anonymous functions, so that error stack traces are easier to read.

What would be even more awesome, if there was an easy way how to automatically save callback arguments into test object.

// instead of 
it('works', [
  db.createUser.bind(db, { name: 'a user name' }),
  function act(data, cb) {
    this.data = data;
    db.users.edit(this.data.UserId, { name: 'new name' }, cb);
  },
  //etc.
]);

// write
it('works', [
  db.createUser.bind(db, { name: 'a user name' }),
  function act(data, cb) {
    db.users.edit(this.data.UserId, { name: 'new name' }, cb);
  },
  //etc.
]);

Maybe that is a good use for mandatory function names:

it('works', [
  // setup{Name} -> the first cb argument is saved as this.{name}
  function setupUser(cb) { db.createUser(db, { name: 'a user name' }, cb); },
  // etc.
]);

I am not sure if this is not getting too auto-magical, i.e. if it will be still easy to understand what is going on by somebody new to the code.

it('does something complex', [
  db.setupSchema.bind(db, /*...*/),
  function setupUser(cb) { db.createUser.bind(db, { name: 'a user name' }, cb) },
  function act(cb) {
    db.users.edit(this.user.Id, { name: 'new name' }, cb);
  },
  function setupUpdatedUser(cb) {
    db.users.findById(this.user.userId, cb);
  },
  function assert(user, cb) {
    expect(this.updatedUser.name).to.equal('new name');
    cb();
  }
]);

This should be implemented in a way that preserves the behavior of it for each step function. Specifically:

• you should be able to ommit a callback for sync functions (usually for access to this without a useless var self).
• throwing inside a function should throw to the test
• each step should have its own timeout()
• etc

Now that I think about it, this could almost be implemented recursively.

// doesnt handle waterfall (arg passing)
function newIt(desc, fn) {
Array.isArray(fn)
  ? fn.forEach(newIt.bind(this, 'sub test'))
  : oldIt.apply(this, arguments)
  ;
}

Well, the problem I am trying to solve, is all about argument passing. Besides that, individual tests (it blocks) should be independent and share as little state as possible.

each step should have its own timeout()

Why isn't the timeout shared by the whole test good enough?

you should be able to ommit a callback for sync functions (usually for access to this without a useless var self).

Hmm, that looks useful. However, how would you distinguish between these two functions?

function step2(dataFromStep1) {
 // sync
}
function step3(cb) {
  // async
}

throwing inside a function should throw to the test

👍

So, this basically allows the following code to be omitted:

async.waterfall.bind(
 [  /* ... remains unchanged .... */ ]
);

And assumes waterfall is more common than parallel (I could believe that), and by omitting the async.waterfall(), means you really have to know mocha in order to understand what it means to pass an array of function to mocha. I don't find it really compelling. I don't mind composeable functions, particularly when they are easily composeable (in this case moch and async follow same "done as last arg to fn, err as first arg to done" convention).

A couple of edits to this code above:

it('works', [
  // setup{Name} -> the first cb argument is saved as this.{name}
  function setupUser(cb) { db.createUser(db, { name: 'a user name' }, cb); },
  // etc.
]);

To make this less magical, it could be implemented as:

it('works', [
  // setup{Name} -> the first cb argument is saved as this.{name}
  // function name not required (but encouraged)
  function () {
    db.createUser(db, { name: 'a user name' }, this.set('user'));
  },
  function () {
    assert(this.user);
  }
]);

it.set = function() {
  var names = arguments;
  return function() {
    var args = Array.prototype.slice.call(arguments, 0);
    var err = args.shift();
    if(err) throw err; // is there a `test.error()` function?

    args.forEach(function(arg, i) {
      var name = names[i];
      if(name) this[name] = arg;
    }.bind(this));
  }
}

Oh, I see now. I didn't notice the [] and just assumed it was all in a function body.

Why isn't the timeout shared by the whole test good enough?

If each step can timeout, you can more easily see what is causing the complex test to be slow. This would only really be true if you used named functions. Also it gives you a more realistic threshold for timing out. Given n steps I would assert that n * 2 seconds is much more reasonable for timing out than always 2 seconds no matter the complexity of the test.

Hmm, that looks useful. However, how would you distinguish between these two functions?

Good point. You can't without making this a bit too magical.

Given the savings is just hiding a call to async.waterFall(), I'm not sure I see the value.

That aside, I think I would prefer all of the setup be in a before() or accessed by mocks/helpers from within the test. Starting with a style like this:

describe('some feature set', function() {
  var db = ...;

  before(function(doneSetup) {
    async.waterfall([
      function setupSchema(cb) {
        db.setupSchema(/*...*/, cb);
      },
      function createTestUser(cb) {
        db.createUser({ name: 'a user name' }, cb);
      }],
      doneSetup);
  }

  it('updated the user', function(done) {
    db.users.edit(db.data.UserId, { name: 'new name' }, function() {
      db.users.findById(db.data.userId, function() {
        assert.equal(db.user, 'new name');
        done();
      });
    });
  });
});

Given that as a starting point, I'd rather see a simpler way to mock the DB or set the data in a mocked DB that some helper gives us out of the box in this style:

describe('some feature set', function() {

  usingDB(setupDB, function(db) {

    it('updated the user', function(done) {
      db.users.edit(db.mockedUserId, { name: 'new name' }, function() {
        db.users.findById(db.mockedUserId, function(err, user) {
        assert.equal(user, 'new name');
        done();
      });
    });

  });

  function setupDB(db, doneSetup) {
    async.waterfall([
      function setupSchema(cb) {
        db.setupSchema(/*...*/, cb);
      },
      function createTestUser(cb) {
        db.createUser({ name: 'a user name' }, function(err, user) {
          db.mockedUserId = user.id;
          cb()
        });
      }],
      doneSetup);
  }
});

Thank you all for valuable comments. Because the main reason behind the high number of waterfall steps is the code for building models (as Ryan pointed out), I decided to simplify that piece.

This is my current code:

it('sends notification to the correct device', function(done) {
  var context = {};
  async.waterfall([
    function arrange(cb) {
      context.pushManager = new PushManager();
      var builder = new TestDataBuilder();
      builder
        .def('application', Application, {
          pushSettings: { stub: { } }
        })
        .def('anotherDevice', Device, {
          appId: builder.ref('application.id'),
          deviceType: 'another-device-type'
        })
        .def('device', Device, {
          appId: builder.ref('application.id'),
          deviceType: mockery.deviceType
        })
        .def('notification', Notification)
        .buildToContext(context, cb);
    },

    function act(cb) {
      context.pushManager.pushNotificationByRegistrationId(
        context.device.id,
        context.notification,
        cb
      );
    },

    function assert(cb) {
      expect(mockery.firstPushNotificationArgs())
        .to.deep.equal([context.notification, context.device.deviceToken]);
      cb();
    }
  ], done);
});

How do you like that?

I'll leave the implementation in loopback-push-notification for now, later we can move it to a standalone module and write a blogpost on usage.

I have to agree that I'm on the "less magic" side, as @bajtos should know from my response to his email thread. That said, if you want to go down the "waterfall, building data as you go" model, let me shamelessly plug Stepdown: https://github.com/schoonology/stepdown . That's exactly what it does.