Create a controller folder and start some modularisation.

[rolandnsharp]

in process...

[ThomWright]

Marvellous! I was hoping get round to splitting up the request handlers into separate files. That was the reason I started writing the tests, to make sure I didn't break stuff!

Just a couple of questions:

• Would it be better to use a single assertion library, rather than both should and chai?
• Is there a reason you've chosen to refactor the async tests to use done() rather than returning the promise?

[rolandnsharp]

Thanks for writing those tests @ThomWright!, it's what finally motivated me to do some dirty work on this thing.

• It probably would be better. I know should and didn't want to break my flow by looking up the syntax for every assertion I wrote.
• I wanted to access the callback from the response so that I could test the response values. And again, that's just what I know. I've not really done much promising before. Can I return the question to you and ask why you are using promises?

Also, do you know why these tests have to run quite slowly? They should be lightening fast right?

I also can't get any of the tests to work in isolation using .only. Do you have any workarounds or could could you tell me a little about this test setup?

Thanks again for this.

[ThomWright]

No worries, I guess we can refactor to a single assertion lib later on.

Sure thing. Using promises feels most natural to me when doing async stuff. It avoids lots of nested callbacks. There should be a good few articles on why promises are preferable to callbacks. Also see supertest-as-promised for a bit of help.

Looking at 216af28 it looks like you want to assert that no error is returned. I thought that was checked for implicitly with the previous implementation. If an error is returned and not caught, then the test should catch it from the returned promise and fail the test. I think in using done you are bypassing this mechanism. Note: I could be mistaken!

If you want to do something explicit with the returned error, like log it, then maybe try something like this:

it('should return 201 Created', function() {
  return api.post('/user')
    .send(user)
    .expect(HttpStatus.CREATED)
    .catch(function(err) {
      console.log(err);
      throw err; // rethrow to fail the test
    });
});

or maybe:

it('should return 201 Created', function() {
  return api.post('/user')
    .send(user)
    .expect(HttpStatus.CREATED)
    .then(function(res) {
      console.log(res);
    }, function(err) {
      console.log(err);
      throw err; // rethrow to fail the test
    });
});

Unfortunately I'm as baffled as you are about why the tests run so slow. Even though tests sometimes send multiple HTTP requests it should still be quick.

I'm also not sure why only isn't working. I've never used it before. What happens when you try?

[rolandnsharp]

@ThomWright , yeah you're right regarding 216af28, I guess I was thinking of making the tests look consistent. I can revert back to later. I don't think there is any logic issue, only stylistic.

The main reason I wasn't using promises in this PR is that I'm trying to understand what is happening with these tests as much as I can so that I can diagnose the performance issue and they were confusing me. I'm happy to refactor it back afterwards. The primary concern I think is just to have test coverage and get all of that request logic out of the server.js file so that we can start to think about individual parts.

The tests are also slightly inconsistent, sometimes not passing for no apparent reason, I think there is some issue with some async test environment setup stuff. That's the primary test issue right now I think.

What do you think?

Do you think we can replace this package.json line:

  "pretest": "mongo jsonresume-tests --eval \"db.dropDatabase(); db.resume.insert({});\" >/dev/null && echo Using blank DB",

with a moncha before function?

RE .only: I'm getting Cannot call method 'collection' of undefined so the db instance mustn't be getting setup synchronously before the tests start.

One more thing: The first test api.get('/') always fails for me locally but not in Travis, ideas?

[ThomWright]

Hi @rolandnsharp I've raised a couple of issues for the test inconsistency #65 and slowness #66.

The api.get('/') test works for me - I'm using the Vagrant setup. What OS are you using? it.only(... is working for me too!

BTW, big fan of this PR. Exactly the sort of thing I was aiming for when I started the tests.

[rolandnsharp]

Tests are all working consistently for me now except that api.get('/') does not work locally and I havn't looked into it. I think using mocha's before function to drop and connect to the db fixed the consistency issue for me.

I'm glad you like it, there is more to come :) I was worried that you would be less than happy with what I did to your tests :)

I've looked into promises more, and while I understand the benefits I'm not sure that I'd use them in a simple open source project where callbacks and async work fine. I try to keep to standard libraries and popular coding styles. and yeah, promises are fairly popular now so I don't know.

Please feel free to submit your PR's in any fashion that you wish, but I hope you don't mind too much if I refactor a little.

Thanks for showing me another way though.