Skip to content

Use co-mocha for tests #19

Closed
Closed
@techniq

Description

@techniq

Would you be opposed if I converted all the tests to use co-mocha? I was looking to submitting some pull requests and thought the specs were a little difficult to follow with all the Promise flow and thought it would help their readability greatly.

For example, here is the current create.spec.js using mocha and Promise then/catch

describe('DSSqlAdapter#create', function () {
  it('should create a user in a sql db', function () {
    var id;
    return adapter.create(User, {name: 'John'}).then(function (user) {
      id = user.id;
      assert.equal(user.name, 'John');
      assert.isDefined(user.id);
      return adapter.find(User, user.id);
    })
      .then(function (user) {
        assert.equal(user.name, 'John');
        assert.isDefined(user.id);
        assert.equalObjects(user, {id: id, name: 'John', age: null, profileId: null});
        return adapter.destroy(User, user.id);
      })
      .then(function (user) {
        assert.isFalse(!!user);
        return adapter.find(User, id);
      })
      .then(function () {
        throw new Error('Should not have reached here!');
      })
      .catch(function (err) {
        assert.equal(err.message, 'Not Found!');
      });
  });
});

and the same test using co-mocha with yielding promises

describe('DSSqlAdapter#create', function () {
  it('should create a user in a sql db', function* () {
    var createUser = yield adapter.create(User, {name: 'John'});
    var id = createUser.id;
    assert.equal(createUser.name, 'John');
    assert.isDefined(createUser.id);

    var findUser = yield adapter.find(User, createUser.id);
    assert.equal(findUser.name, 'John');
    assert.isDefined(findUser.id);
    assert.equalObjects(findUser, {id: id, name: 'John', age: null, profileId: null});

    var destoryUser = yield adapter.destroy(User, findUser.id);
    assert.isFalse(!!destoryUser);

    try {
      var findUser2 = yield adapter.find(User, id);
      throw new Error('Should not have reached here!');
    } catch(err) {
      assert.equal(err.message, 'Not Found!');
    }
  });
});

I'll finish and submit the change as a PR, but didn't want to make this kind of change without your consent :).

As for the PRs I was thinking about tackling

  • Implement Filter across relation (joins) #18 (but if you can knock it out quicker, that would be awesome)
  • Consolidate the relation handling inside find() and findAll() (basically extract it into a processRelations()
  • I'd like to find a way use MySQL's spatial queries but still get the benefits of js-data-sql's relations (primarily with and putting the nested objects together).
    • One idea I was thinking about having findAll detect if the second parameter (current params) is an instance of a knex query builder and use that query to process instead of the one returned from filterQuery(). For example: sqlAdapter.findAll(Race, query, { with: 'address' })
    • Another idea was to just export the new processRelations() and use it directly. For example: sqlAdapter.processRelations(Race, yield sqlAdapter.query(...), { with: 'address' }) although I haven't thought on this API very much as to what would work best.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions