FIX: Invalid User Tests

[greaterweb]

There appear to be a number of issues present with the User model tests.

• invalidCredentials are actually valid (see validCredentialsEmailVerified)
• test using invalidCredentials should actually fail but it doesn't (broader problem, see below)
• use off .expect needs to be paired with logic in the .end callback (eg. if (err) return done(err);) when testing REST calls, this is not applied to all instances. to confirm these are failing, change one of the .expect values and note the test still passes
• could also extend to create API test where there are only REST tests, for example there is only a REST test for invalidCredentials and nothing specific for API call

Regarding the point of .expect usage, it's probably worth looking into any other REST unit tests to make sure they are correct in implementation.

A question in general. There is both vanilla assert statements and chai expect statements, which is the preferred approach for testing?

Additional tests to add

• check reset password fails without email address (API and REST)
• reset password over REST

[greaterweb]

@raymondfeng I need some help deciphering this test below, it looks like you were originally responsible for authoring it.

The wrong Content-Type test fails when updated as suggested in the original ticket.

    it('Login a user over REST with the wrong Content-Type', function(done) {
      request(app)
        .post('/users/login')
        .set('Content-Type', null)
        .expect('Content-Type', /json/)
        .expect(400)
        .send(validCredentials)
        .end(function(err, res) {
          if (err) {
            return done(err);
          }
          done();
        });
    });

  1) User User.login Login a user over REST with the wrong Content-Type:
     Error: expected 400 "Bad Request", got 200 "OK"

What is the desired behavior if Content-Type is set to null with the request? In doing some basic debugging, I'm getting a 200 with a valid accessToken as the res value.

[greaterweb]

Some troubleshooting information related to the wrong Content-Type test.

The combination of .set('Content-Type', null) and .send(validCredentials) produce request headers of:

{ 
  host: '127.0.0.1:57654',
  'accept-encoding': 'gzip, deflate',
  cookie: '',
  'user-agent': 'node-superagent/0.18.0',
  'content-type': 'application/json'',
  'content-length': 40 
}

Updating the send body to .send(JSON.stringify(validCredentials)) will set Content-Type to application/x-www-form-urlencoded and produce a 400 with message "username or email is required", this is likely closer to the intention of mismatching Content-Type value to request body.

Not sure if this is the actual 400 message that is desired but a 400 none the less. Is the intention that we get a generic 400 bad request or will this 400 be sufficient?

[greaterweb]

I've made some fixes to the issues noted and had a review of other tests making REST calls. All seems to be in good shape. Once I can get some confirmation on the test above I'll be sure to submit a pull request.

[raymondfeng]

@greaterweb Thank you for making efforts to improve the tests. Sorry for the delay as some of us were on holidays.

A few comments:

• Good catch to the invalidCredential
• Good catch to the end(), which will receive an error if the expected status code doesn't match
• For the content type test, we should set it to something like text/html

Can you create a pull request from your repo? It would be simpler to add inline comments with the PR.

[greaterweb]

@raymondfeng I rebased my fix branch with the current loopback master and created the pull request.

Please feel free to add your comments to the PR and I'll adjust accordingly.

[greaterweb]

Closing as PR has been merged.