User CRUD API tests

[lirantal]

No description provided.

[lirantal]

Yay we're almost 10% higher in code coverage!!! :)

[mleanos]

Awesome! We're getting there! :) Great job!

[ilanbiala]

What's this extra block?

[lirantal]

Good catch, probably a left over from all the tests, I removed it.

[lirantal]

@ilanbiala - useful comments as always, thanks for the code review.
Let me know what do you suggest we do about the e-mails - which even if we fix for Travis, chances are high that it won't work on common machines (just as it doesn't work on my own even), without proper setup.

[ilanbiala]

Try to group functionality/features and use beforeEach to DRY the user.save code.

[lirantal]

the user.save() part don't need to apply to all tests so we can't just throw that in beforeEach, and there's no sense to wrap it with any other function as it's anyway just going to get a callback.

I was thinking maybe wrapping the user signing-in in a function but it looks more readable and extensible as is.

[ilanbiala]

@lirantal What specifically doesn't work locally when you test it?

[lirantal]

@ilanbiala the forgot functionality fails due to not being able to send e-mail, so it responds with a 400 error, specifically here: https://github.com/meanjs/mean/blob/master/modules/users/server/controllers/users/users.password.server.controller.js#L87

The tests are written in a way that validates that even after we get a 400 response the database is inspected to confirm there is indeed the reset token etc. When it's failing with that 404, it is failing for not being able to send the email, which if you look at the above controller code then otherwise it just sends the e-mail ok.

I think testing the e-mail being sent is beyond the scope of the tests so I don't want to introduce testing emails sent out.

[ilanbiala]

@lirantal I'm pretty sure it errors because the API keys aren't even set. Have you looked at the error message from it, because Nodemailer doesn't throw an error for not being able to actually send an email, it just follows send and forget if I remember correctly.

[lirantal]

Good to know, I'll try it.
If that will work then it means we can also cover both branches in the code (the successful email and the failed one due to bad config)

[lirantal]

@ilanbiala even when I comment the options property in test.js config I'm getting the same error.

[ilanbiala]

@lirantal what do you mean comment? You need to provide that is what I'm saying for Nodemailer not to error. What is the specific error you get?

[lirantal]

@ilanbiala ok I understand the confusion now.
I don't think we should be making sure that nodemailer is able to send the email because that's beyond the scope of the API tests. If we were able to confirm that everything in the code was tested successfully up until the point where nodemailer fails to send an e-mail because no e-mail settings were configured then this is ok IMO.

[ilanbiala]

@lirantal that's fine, but we can make sure we don't get an error because of Nodemailer, whether it actually sends or not. What is the specific error you see when you assert those 400s?

[bastianwegge]

A little hint for email testing, the following is our implementation. We use sinon to mock the function that actually sends the email and verify a callcount for each recipient. Maybe it's useful to you:

describe('Mailer Controller:', function() {
    beforeEach(function(done) {
        done();
    });

    describe('sendMail()', function() {
        beforeEach(function() {
            transport = {
                name: 'testsend',
                version: '1',
                send: function(data, callback) {
                    callback();
                }
            };
            mailer.setTransport(transport);
        });

        it('should be defined', function() {
            mailer.sendMail.should.not.equal(undefined);
        });

        it('should try to send email', function(done) {
            sinon.stub(transport, 'send').yields(null, 'tere tere');

            mailer.sendMail('empty', {
                to: 'test@example.com'
            }, null, function(err, info) {
                should.not.exist(err);
                transport.send.callCount.should.equal(1);
                info.should.equal('tere tere');
                transport.send.restore();
                done(err);
            });
        });
    });
});

[lirantal]

@ilanbiala "Failure sending email" is the exact error that is being returned, specifically here, by this call: https://github.com/meanjs/mean/blob/master/modules/users/server/controllers/users/users.password.server.controller.js#L87

I believe this assertion is just fine because it tests our API and that everything is ok, the other way the if statement can end there is with success.

[ilanbiala]

@lirantal I'm referring to this err variable https://github.com/meanjs/mean/blob/master/modules/users/server/controllers/users/users.password.server.controller.js#L80

[lirantal]

@ilanbiala it throws err:

{ [Error: self signed certificate] code: 'DEPTH_ZERO_SELF_SIGNED_CERT' }

[lirantal]

@ilanbiala I can workaround the email problem by adding the following options to nodemailer transport:

tls: {
        rejectUnauthorized: false
      }

Do you suggest I add it as the default options in config/env/test.js?

[ilanbiala]

@lirantal yeah that sounds fine, unless we can get a free certificate and still keep it secure somehow.

[lirantal]

@ilanbiala even though with the tls config it works fine locally on my machine (probably because I have some mail server configured there), on travis it fails due to:

{ [Error: connect ECONNREFUSED]
  code: 'ECONNREFUSED',
  errno: 'ECONNREFUSED',
  syscall: 'connect' }

returned by nodemailer itself.
This goes back to the problem that we can not assure that there is indeed a mail server that will work for nodemailer config to complete our API with 200 successful response. Do you think there's anything else we can try except changing back to asserting 400?

[lirantal]

@ilanbiala per my previous comment - I'm going to refactor the tests back to assert for a 400 "failed sending tests" issue as it seems that nodemailer is not environment-agnostic and will require some kind of email environment setup to be able to return 200 for tests. Since it is not the purpose of the test to check an email was sent I don't see it as a problem.
Let me know if you think otherwise or would like to suggest an alternate method for testing this.

[lirantal]

@ilanbiala I reverted back the changes so that we still assert a 400 due to unable to reliably being able to test an email sending option end-to-end. Since we worked out all other issues I'll go ahead and merge this PR once the build is complete so we can have a broader safety net of code coverage for tests.

[Maniselvam006]

Hi guys I am using MEAN stack in my application with AngularJS as my front-end.....my problem is if i forget the password i were enter the user name and submitted, but i got error like "Failure sending email" is there any solution to solve this....I'M new to meanjs... so please help to solve this issue....forgot password page is not working.....thanks...

[vaucouleur]

@Maniselvam006 You probably did not set up your email preferences:
https://github.com/meanjs/mean/blob/master/config/env/development.js#L62

Next time please open an issue, or ask on Gitter. Good luck with meanjs, I hope you will like it !