Seed options - logResults

[mleanos]

Added an options object to the database seed configuration. Currently, the only option implemented is logResults; set to 'true' by default.

Setting the logResults option to false in the core configuration server test suite.

The initial reason for this option is to disable the console output of the seed db events, while running the test coverage. However, this can be useful in other cases. For instance, if the user doesn't want the passwords displayed in the console where they may be comprimised.

[codydaig]

@mleanos I'd prefer this be an option in the env config.

[mleanos]

@codydaig I originally thought the same thing. However, we'd have to explicitly set the option to be specifically for when the tests run.

For example:

seedDb: {
  logResultsDuringTests: process.env.logResultsDuringTests || false
}

Maybe using options at the seed config, and env config would work.. that way someone could set the option in the env config if they want it applied everywhere. Or they could leave the env config as the default of true, and then set it using the options parameter for the seed config.

The key thing is that we need to provide a way to mute the console messages when the test suite is running vs when the db is being seeded.

I think I still prefer having the options parameter. But I'm not opposed to having a "global" env config as well.

[mleanos]

Another thing to keep in mind, is that the Seed Db may, eventually, be used in other parts of the application. Hence, why we have the .start exported. So we'd want to accommodate an option to mute console messages on any given usage.

[mleanos]

@codydaig What if we set the default here, from the env config? While keeping the seedOptions object.

[lirantal]

That's a lot better, yes. It means we are not inventing another configuration option but instead re-using and existing one.

[lirantal]

@mleanos will this logging option support also logging the data (the seed user/pass that was created) into the app log file?

[mleanos]

@lirantal We could have an option for that as well. Or have logging options that may include a list of where to log the results.. ['console', 'app logs'.. etc. We don't currently have the app log file functionality associated with the seeding, but we could introduce it.

The key point for me is to be able to control the logging, or any other options that we may introduce, on any given seed operation.. Meaning, that when someone calls seed.start from multiple areas of the application, they can specify options for each of the usages.

[mleanos]

Also, I think having default seed options coming from the env configs would a good idea. WDYT?

[mleanos]

Made some modifications, based on suggestions from @codydaig & @lirantal

The seed options are now defaulting from env configs. Also, changed the seedDB env config setting to an object, with an options field.

Also, fixed an issue with how the env config was reading the seed setting from the processes env variable; see commit message for more details.

[bastianwegge]

Coming from a ruby/C# Scene the whole seeding thing seems to get a lot bigger than it has to be. @lirantal & @codydaig what do you think about creating mocks that can be used in the tests for example for a user? In that case you'd test the mocks using your model-tests, would have only two places (model&mock) to reconfigure when changing something and you'd have the seeding right away. That would obviate the need to tests the seeding. Talked about that with @mleanos earlier, but we were disconnected while talking.

[lirantal]

How about using lodash's .has method? it's a shorthand way of doing it and we can use it consistently across the project?

[ilanbiala]

Please use {} for if statements.

[lirantal]

@mleanos how about we move the seeded user details outside of the seed file and into the env variables as well as config options? (@bastianwegge is that what you meant by mock users?)

[bastianwegge]

This is what I meant:

Imagine a MockFile like this user.model.mock.js using faker.js.

'use strict';
var faker = require('faker');

function user(credentials) {
    if (credentials === undefined) {
        credentials = {
            username: 'username',
            password: 'password'
        };
    }

    return {
        displayName: faker.name.findName(),
        email: faker.internet.email(),
        username: credentials.username,
        password: credentials.password,
        provider: 'local',
        roles: ['user']
    };
}

module.exports = {
    User: user
};

You would require this in your tests that would create something like this:

describe('User Model Unit Tests:', function() {

  afterEach(function(done) {
    User.remove().exec(done);
  });

  beforeEach(function() {
    user = userMock.User();
  });

  describe('Method Save', function() {
    it('should begin with no users', function(done) {
      User.find({}, function(err, users) {
        users.should.have.length(0);
        done();
      });
    });

    it('should be able to save without problems', function(done) {
      var _user = new User(user);

      _user.save(function(err) {
        should.not.exist(err);
        done();
      });
    });
  })
});

This way you would have tested your mongoose-Schema and your mock. Then you could just go ahead and create something like a seeding-service like the following seeds.js without the necessity of testing it because you're using tested services:

function addTestUser(done) {
    var _user = new User(userMock.User());
    _user.save(done);
}

Any questions?

[mleanos]

@lirantal I will address your comments today. I already have a branch that moves the User details out of the seed configuration and puts it into the env configs. I'll add it to this PR.

@bastianwegge What you're suggesting is outside the scope of what this PR is intended for. I think you make some valid points, and I'd like to discuss it further; and get everyone else involved. This would be a big change in most of the server side tests, so it's probably best for a separate PR. Thanks for the idea; I think we can incorporate something like your example.

[mleanos]

@lirantal @codydaig I addressed your comments, and added some tests to the core server config test suite.

1. Moved the seeded user details to the env configs (development, production, and test).
2. Added the seeded user details to the seedDB config options; defaults to use the env config.
3. Added additional tests to the core server configuration test suite
4. Added the usage of lodash's .has method to check the existence of the custom seed options.

@ilanbiala I actually prefer the way the code reads using the inline if statements, in this case. Otherwise, we would have something like this

if (options) {
    seedOptions.logResults = options.logResults || seedOptions.logResults;
    seedOptions.seedUser = options.seedUser || seedOptions.seedUser;
    seedOptions.seedAdmin = options.seedAdmin || options.seedAdmin;
  }

I think what I have now, is much cleaner, and we only touch the seedOptions when the options are provided. I can change this based on your suggestion, if it goes against our coding styles. I'm just not aware that we have a desired style in cases like this.

Something that occurred to me while writing the tests, was that we can only test against the test env seed settings; even though we're manually changing the process.env.NODE_ENV. This probably isn't a problem, but I wanted to point it out.

One last thing, is the possibility of the seedDB being turned on for the test environment. Should we allow this? Or should we set the seedDB.seed setting to false in the test env config? This isn't a big deal to me, but it brought up the issue of whether or not we can expect a user of this project to run the application using the test environment. Just something that seems ambiguous to me. I can see this go both ways.

[mleanos]

BTW, I'm surprised the coverage only went up .02%. I had such high hopes for an increase here :( Was aspiring for something like @lirantal +8% the other day. haha

I'm just glad the build succeeded :)

[mleanos]

@lirantal Was there a specific reason this is here? I don't see a usage. I forgot to mention it before
https://github.com/meanjs/mean/pull/957/files#diff-7dfe67f86c4c0773b98c0422a0cb606aL122

[lirantal]

Yeah I see it's not being used, can you fix it?

[lirantal]

@mleanos why do we have both userFromSeedConfig and user1? are you testing the config options are actually set? I don't think that's required...

[mleanos]

@lirantal Yes. I can remove that unused var.

The userFromSeedConfig is used to test the seeding using the default seed user details set in the in env config. user1 is used to test the passing of the seed user details into the .start method using the options param.

If we don't test against the default settings, then we can only test the seed using the passed in user details, using the options param.

What you're saying is that we can't guarantee that the env config will contain the seed user details? It would be nice to test the seeding using the default settings from the env config though.

How about we test for the existence of the user details in the env config, and if present then we can proceed to test using those settings? Otherwise, we just allow the test to pass since we don't have the user details to test against.

[lirantal]

Got it. I'm ok with the tests as is.

[mleanos]

@ilanbiala How do you feel about this? https://github.com/meanjs/mean/pull/957/files#diff-7dfe67f86c4c0773b98c0422a0cb606aR105

See my comment above about it #957 (comment)

[mleanos]

@codydaig @ilanbiala @lirantal How do the seedDB env config settings look to you?

I'm wondering if I need to add something like process.env.MONGO_SEED_USER_USERNAME to both the admin and user account details. Would probably only need this for username & email.. WDYT?

Also, would it be appropriate to add an exported function to the seed configuration, to retrieve the options? I'm thinking that it would be better to rely on the seed configuration to interact with the seedDB options from other parts of the application. For instance, the server config test suite; rather than retrieving the options from the application config like here https://github.com/meanjs/mean/pull/957/files#diff-27fe56fc024d6e9d46e8c361ad7879e4R45

The idea I'm going with here is to add some additional sanitization to the options. Perhaps, to gracefully handle when the seedDB options aren't present in the env config. I'm trying to get the seedDB feature to be more extensible & reusable; but also not too opinionated. Any thoughts?

[ilanbiala]

@mleanos no, that isn't okay in my opinion, and honestly having one strict style is better. Also, not having brackets leads to confusion and other issues that you may be aware of.

[mleanos]

@ilanbiala Are you specifically saying that you prefer something like this?

if (_.has(options, 'logResults')) { seedOptions.logResults = options.logResults };
if (_.has(options, 'seedUser')) { seedOptions.seedUser = options.seedUser };
if (_.has(options, 'seedAdmin')) { seedOptions.seedAdmin = options.seedAdmin };

Or do you mean you'd rather have it broken up into multiple lines?

if (_.has(options, 'logResults')) { 
  seedOptions.logResults = options.logResults 
};

if (_.has(options, 'seedUser')) { 
  seedOptions.seedUser = options.seedUser 
};

if (_.has(options, 'seedAdmin')) { 
  seedOptions.seedAdmin = options.seedAdmin 
};

If the latter, I'd be concerned with readability. Although, we could put this logic in a separate function like initSeedOptions(options); that sets the global seedDB options object. Any ideas on that?

[ilanbiala]

The latter, and put it in whatever way makes sense. But the bracket requirement should be strict.

[ilanbiala]

Resolve with something to affirm, or maybe the removed user.

[mleanos]

@ilanbiala I addressed your line comments, with the exception of a couple.

The resolve() comments were on non-diffs. I don't think we need to resolve anything in these cases. We're just concerned with whether or not it was successful, or not. The data that we could return from these Promises wouldn't have any relevance outside the scope of the Promise; So I think it's appropriate to resolve with no data here.

As for the issue of the non-async tests using the done callback method, I think it's fine to use it here. It doesn't affect the functionality in any way, but I like the way it reads; it's more clear. Either way, I'm fine with removing them from all the tests in this file.

[ilanbiala]

@mleanos there are quite a few resolves that are just in a .then, which I'm not really understanding why they are there. Can you explain that?

If tests aren't async it's unnecessary, and it also indicates whether the tested function is synchronous or asynchronous, so let's make sure we aren't abusing it.

[mleanos]

@ilanbiala Those resolves are to satisfy the current promise. You'll notice that each usage is at the end of the chain; we need to resolve each Promise, at each level.

I'll go ahead and make those tests synchronous.

[mleanos]

@ilanbiala Updated the tests. If we need to address the resolves, in regards to your concerns, it will require a bigger refactor of the SeedDB feature that we should do in a separate PR.

[mleanos]

@ilanbiala @lirantal Does this look ready to you both?

[ilanbiala]

@mleanos if we can't address the resolves in this PR, then let's open one right after because I don't think that is very intuitive. Otherwise, LGTM.

[mleanos]

I would rather finalize this PR, and then have a discussion on how we want to proceed with our strategy of using Promises & Closures.

@lirantal Is that fine with you? I'll be on Gitter for a couple more hours, if you'd like to discuss.

[lirantal]

Sure.