Config Merge: Values After Booleans Ignored

[joshdmiller]

I don't have time to look into this right now, so I'm filing this to not forget it. If someone wants to take a crack, a high-quality PR would be awesome!

---

During config merging, any boolean values in nested objects cause all subsequent values to be ignored.

Example 1

warlock.json:

{
  "blah": {
    "one": true,
    "two": false,
    "three": "three"
  }
}

Will yield:

$ warlock --conf blah

blah = { one: true, two: false }

Example 2

Some spell's package.json:

{
  "warlock": {
    "blah": {
      "a": "a",
      "one": true,
      "two": false
    }
  }
}

Combined with warlock.json:

{
  "blah": {
    "three": "three"
  }
}

Will yield:

$ warlock --conf blah

blah = { "a": "a", one: true, two: false }

Not sure what's going on here...

[PaulL1]

I have the time to look at this I think. I can just work to fix this point issue if you wish, but I'm more inclined to build some unit tests whilst I'm at it.

If doing that, I would follow the pattern I see in the gulp repository, which uses mocha. https://github.com/gulpjs/gulp/tree/master/test

Is there anything about that that would have you saying don't do that?

[joshdmiller]

@PaulL1 That would be awesome. You got here first, so go for the Gulp approach; it's essentially what I had in mind anyway.

I know we need unit tests going forward; it's very high on my "to do" list now that we have a semi-stable API. If you want to take a crack at unit testing anything, feel free to go for it and receive my eternal gratitude. :-)

[PaulL1]

Couple more questions.

1. I can see warlock is mostly written in coffee. It's not my thing, but doesn't look that hard. Mocha seems to be able to take coffeescript, but the gulp tests are javascript. Do you have a preference?
2. To unit test the config stuff, I need to run init, which is buried in bootstrap. I'm thinking to refactor bootstrap to break it down into smaller pieces, allowing the test library to reuse bits of it (at the moment it's an all or nothing, and I don't want to duplicate a lot of that code in the test suite). Again, is that raising any red flags for you? I'll probably start off by writing test cases for bootstrap, just to prove I can actually write test cases.
3. I'm heading down the path of the test cases running directly in the warlock directory - so I'd imagine cloning ngbp/warlock, then npm install, then running mocha there (with fixtures etc). That also implies a .gitignore to exclude the node_modules folder. I recall earlier on ngbp you had a different workflow, but it seems that if you get into travis / CI again that you'll probably want this running directly in the warlock folder. Fair assumption?

[joshdmiller]

1. Warlock is written in CoffeeScript for a damn good reason: because! Actually, I don't remember why. I was probably just lazy and opted for the more succinct syntax. As long as our unit tests are consistently written, I don't care if they're in CoffeeScript or JavaScript. As, again, you got here first, and as the JavaScript version will run slightly faster and be easier to set up for npm, let's just do them in JavaScript. In the future, warlock will probably build to JavaScript before publishing to npm.
2. warlock.bootstrap is supposed to be just something that calls the other functions in a specified order, returning a promise. Through a little trial and error of some features, a little logic got embedded in there. What I'd like to see is all that logic re-factored out, unit testing each on its own, and then unit tests that run on the e2e bootstrapping process. I say "re-factored" because I still want warlock.bootstrap() to be the only API call needed to get the thing set up. But perhaps I misunderstood your question...
3. I'll definitely get CI running on this as soon as we get unit tests running. Check out the way Gulp runs the unit tests: npm test will execute the linter and kick off the unit tests. Since this is an NPM module, we need to ensure unit tests pass before we npm publish, which this will do automatically. It's an added bonus that it makes our Travis config very straightforward. In addition to CI, we'll probably want a pre-commit hook that runs the unit tests to prevent accidental bad pushes that dirty up the commit history - I've certainly done this before. But don't worry about that now unless you've a black belt in git-fu; we'll circle back around to it.

[PaulL1]

OK, sounds great.

In the meantime I spent a few minutes actually looking at coffeescript, and it's fine once I get my head around the syntax. I've written the sample in coffee, I figure it's easier to stick to one language at a time.

I've refactored bootstrap a little, just enough to make some testable and reusable functions. I suspect we could move those functions into their "owning" module - so for example "readConfig" might go into config.coffee. I haven't done that as yet, but would be easy to do so.

Since you're online I'd be interested in a bit of feedback if you have the time. I've pushed to: https://github.com/PaulL1/warlock/tree/unit_tests. The changes are:

• refactor on bootstrap.coffee
• a new .gitignore
• the test directory

To run it, you need to npm install, then you can run:

node_modules/mocha/bin/mocha --compilers coffee:coffee-script/register

Clearly the command line needs tidying up, I think I need to install mocha globally, and there must be a way to include a config file that tells it to interpret coffee, at the moment command line is what I know. :-)

A few areas for you to think on:

1. I'm probably going to rename each test script along the lines of bootstrap_spec.coffee. Just haven't done it yet. It's annoying having two different bootstrap.coffee files open in the same editor
2. I haven't yet worked out what loadPlugins does in enough detail to write tests for it. But I might skip that for now, and focus on config now that I've proven I can actually write unit tests against this
3. Have you used mocha before? Any idea how to test for exceptions? With jasmine in rails I can just expect an exception, but I think the asynch is getting in the way here. I may work out how to mock the fatal method instead, but wondering if you had a better idea
4. Anything else with the general structure/process that you have opinions on. I'm not overly attached to any of it - I've just "leveraged" stuff that's already on the net in various places, I don't have enough experience with mocha or coffee to have a preferred way - so if you see something that looks funny don't assume that's because I have some weird preference - call it out.

[joshdmiller]

Random question - while I have no objection to using .gitignore, why do we need one?

You can use a mocha.opts file to simplify the command-line a little bit. We can simplify even more with a global installation too; it's then just mocha.

I'm probably going to rename each test script along the lines of bootstrap_spec.coffee. Just haven't done it yet. It's annoying having two different bootstrap.coffee files open in the same editor

I agree, but I prefer bootstrap.spec.coffee instead. It's clearer to me.

I haven't yet worked out what loadPlugins does in enough detail to write tests for it. But I might skip that for now, and focus on config now that I've proven I can actually write unit tests against this

That's fine. loadPlugins basically just reads in all the JavaScript or CoffeeScript files from a directory, ensures they export a function, and then calls that function, passing in warlock as an argument. loadNpmPlugins will call said method for every installed NPM module that both has a plugins/ directory and has the keyword warlockplugin in its package.json file; it just passes the path to plugins/ and then loads the warlock key into the config from each module's package.json. The main load method just kicks off the NPM process and then calls loadPlugins for every directory the use puts into the plugins array of warlock.json.

The code contains some async magic from caolan/async and obviously heavy use of promises, which make it a little harder to follow.

Feel free to skip it for now.

Have you used mocha before? Any idea how to test for exceptions? With jasmine in rails I can just expect an exception, but I think the asynch is getting in the way here. I may work out how to mock the fatal method instead, but wondering if you had a better idea

Usually, it's best to use an expectations library of some kind with Mocha. I like should.js, which is written by the Mocha people. Then it's: whatever().should.throw().

In many cases, for the promises, you have to test using a then call. E.g.:

if 'should throw an error', ( done ) ->
  whatever().( ( success ) ->
    success.should.not.be.ok()
  , ( err ) ->
    err.should.be.instanceOf 'WhateverError
  )

fatal doesn't throw an error, though; it simply exits out in shame. That will probably need to be mocked out. But it may require changes to the API; e.g. the code assumes the app is exiting and so often doesn't return values or anything.

Anything else with the general structure/process that you have opinions on. I'm not overly attached to any of it - I've just "leveraged" stuff that's already on the net in various places, I don't have enough experience with mocha or coffee to have a preferred way - so if you see something that looks funny don't assume that's because I have some weird preference - call it out.

It looks really good; I have only one minor thing. I understand why you're exporting the refactored bits (like readConfig), but to avoid user confusion and to keep it out of the public API, we should prefix them with an underscore (e.g. _readConfig).

It also might be easier to follow if we chain more (which is awkward in CoffeeScript, I've discovered). From this to this:

  bootstrap.readConfig( configPath, pkgPath )
  .then( bootstrap.loadPlugins )
  .then( bootstrap.setDefaultTask )

e.g. avoiding passing around promise objects. All of the methods can return promises if they need to and Q will handle it for us, so testing becomes the same for all the methods. e.g.:

it 'config, empty package.json, loaded correctly', ( done ) ->
  bootstrap.readConfig( 'test/fixtures/bootstrap/user.json', 'test/fixtures/bootstrap/empty.json' )
  .then ->
    config.get( 'standard_values.one' ).should.eql false
    config.get( 'standard_values.extra_1' ).should.eql 1
    config.get( 'standard_values.extra_2' ).should.eql 'fred'
    done() # make sure you call this because the test is async

# ...

describe 'setDefaultTask', ->
  it 'establishes default tasks', ( done ) ->
    config.set( 'default', [ 'a_task' ] )    
    bootstrap.setDefaultTask().then ->
      # test goes here
      done()

---

Completely as an aside, I've been thinking lately about a couple of concepts that you brought up. One is dependency injection. As we start adding more and more unit tests, it may make sense to use some kind of DI container like broadway or intravenous. I don't want to jump off that bridge yet, but keep it in mind if you keep running into the need to mock components.

Another thing is the use of promises. I love promises. But considering that the nature of Warlock, it may make sense to move the asynchronous processes from promises to streams. That would be a very large refactor and I'm not ready to jump off that bridge yet either, but it would make it more straightforward to read and to test if all these methods were streams.

# code
bootstrap.readConfig( configPath, pkgPath )
  .pipe( bootstrap.loadPlugins )
  .pipe( bootstrap.setDefaultTask )

# test
it 'config, empty package.json, loaded correctly', ( done ) ->
  bootstrap.readConfig( 'test/fixtures/bootstrap/user.json', 'test/fixtures/bootstrap/empty.json' ).apply () ->
    config.get( 'standard_values.one' ).should.eql false
    config.get( 'standard_values.extra_1' ).should.eql 1
    config.get( 'standard_values.extra_2' ).should.eql 'fred'
    done() # make sure you call this because the test is async

Just thoughts to keep around.

[PaulL1]

Thanks for that, I'll look through today. I've included should.js, so that will work fine - I'll play around a bit with mocking to see if I can get a verification of termination without exiting the test framework or writing ugly messages on the console.

On .gitignore, I think there are three things that combine to make me think we need it:

1. Testing directly against the warlock directory, rather than installing somewhere else and testing there
2. Needing to add dev dependencies, so an npm install is necessary
3. My occasional laziness where I type git add .

Perhaps the last is the behaviour I need to change - unless there is a different workflow that you see here that avoids the problem?

[PaulL1]

I’ve applied changes to reflect your suggestions - all good suggestions.

I have found and fixed the defect - it came down to the fact that forIn terminates execution if the callback function returns false. We had no explicit return value, so if the value we processed happened to be “false” then we were returning false, and it would terminate execution.

I am finishing out the unit tests on config, and I’m not entirely clear on the expected behaviour that we expect when we pass in a merge value to config. I had initially assumed that merge would be an object that we wanted to merge in, but it looks like it’s used as a true/false. So if we set merge to true, then we put the key/val into an object and call merge to push that object into the config. If merge isn’t true, then we just assign the value to the key. I’m not clear what the circumstances are when these two would give different results, and therefore why it matters which we do. Is there a material difference, or is this kruft that used to mean something and now doesn’t?

[joshdmiller]

Awesome! :-)

The merge argument to warlock.config will cause a property to be deep-merged rather than overwritten. E.g. assuming this is what's already in the config:

{
  "prop": {
    "val": {
      "one": [ "one", "two" ],
      "two": "two"
    }
  }
}

If we run warlock.config( 'prop.val.one', 'hello' ), it will result in this:

{
  "prop": {
    "val": {
      "one": "hello",
      "two": "two"
    }
  }
}

But if run warlock.config( 'prop.val.one', 'hello', true ), it will be this:

{
  "prop": {
    "val": {
      "one": [ "one", "two", "hello" ],
      "two": "two"
    }
  }
}

Internally, Warlock uses the sub-methods of warlock.config; this method is mostly just exposed to end-users and spell authors. I can foresee circumstances where a person might want to completely overwrite the defaults in a spell - or not. That's why it's in there, but right now it's not used.

[PaulL1]

On the deep merge, I want to check my understanding here, because it seems to be doing a deep merge irrespective of what I set the merge flag to - I want to check it's a bug before I fix it.

My unit tests are:

config file

{
  "blah": {
    "one": true,
    "two": false,
    "three": "three",
    "an_array": [ "item_1", "item_3"],
    "an_object": {
      "value_1": 1,
      "value_2": "two"
    }
  }
}

Test cases

    it 'override an array value in the loaded config, it\'s still there when asked for again', ( done )->
      bootstrap._readConfig 'test/fixtures/config/user.json', 'test/fixtures/config/package.json'
      .then ->
        try
          config( 'blah.an_array', 'item_2', false ).should.eql 'item_2'
          config( 'blah.an_array' ).should.eql 'item_2'

          done()
        catch err
          done( err )

    it 'use merge to add a value to an array in the loaded config, values have been persisted into the config', ( done )->
      bootstrap._readConfig 'test/fixtures/config/user.json', 'test/fixtures/config/package.json'
      .then ->
        try
          config( 'blah.an_array', 'item_2', true ).should.eql [ 'item_2', 'item_1', 'item_3' ]
          config( 'blah.an_array' ).should.eql [ 'item_2', 'item_1', 'item_3' ]

          done()
        catch err
          done( err )

The results I'm getting are:

  1) warlock config:  config base export:  override an array value in the loaded config, it's still there when asked for again:
     AssertionError: expected [ 'item_2', 'item_1', 'item_3' ] to equal 'item_2'

I have a similar result on a similar test case for objects:

    it 'override an object value in the loaded config, it\'s still there when asked for again', ( done )->
      bootstrap._readConfig 'test/fixtures/config/user.json', 'test/fixtures/config/package.json'
      .then ->
        try
          config( 'blah.an_object', { 'value_3': 'three' }, false ).should.eql { value_3: 'three' }
          config( 'blah.an_object' ).should.eql { value_3: 'three' }

          done()
        catch err
          done( err )

    it 'use merge to add a value to an object in the loaded config, values have been persisted into the config', ( done )->
      bootstrap._readConfig 'test/fixtures/config/user.json', 'test/fixtures/config/package.json'
      .then ->
        try
          config( 'blah.an_object', { 'value_3': 'three' }, true ).should.eql { value_1: 1, value_2: 'two', value_3: 'three' }
          config( 'blah.an_object' ).should.eql { value_1: 1, value_2: 'two', value_3: 'three' }

          done()
        catch err
          done( err )

 2) warlock config:  config base export:  use merge to add a value to an object in the loaded config, values have been persisted into the config:

      AssertionError: expected { value_3: 'three', value_1: 1, value_2: 'two' } to equal { value_3: 'three' }
      + expected - actual

       {
      -  "value_1": 1,
      -  "value_2": "two",
         "value_3": "three"
       }

[joshdmiller]

It's 2am here, so take what I have to say now with a grain of salt, but I think the problem you're encountering is the difference between the user conf and the runtime conf. That which is controlled directly by the user (warlock.json and warlock --conf key --set val) are set in the former, while everything set by plugins, spells, etc., is set in the latter. When a config value is retrieved from storage using warlock.config(), the two are merged with the user conf taking precedence. The reason for the split is so that the --write command-line flag can save changes to the user's warlock.json file without also saving globbing patterns from spells, for example.

So what I think is happening in your test case is that the value you set in warlock.json is going in the user conf and then in your test you set a new value in the runtime conf, which overwrites the value because of the flag. When you request it again with warlock.config(), it's merging the two together, resulting in the combined array.

This should pass:

config( 'test.new_key', [ 1, 2, 3 ] )
config( 'test.new_key' ).should.eql [ 1, 2, 3 ]
config( 'test.new_key', "overwrite", false ).should.eql "overwrite"

But if test.new_key is in the user conf and the runtime conf, it returns a merged value instead of the one we just set. This is not a bug, but probably should be changed just the same. This is only an issue after the application bootstraps, though, so it shouldn't be something the user actually encounters. But we could put in a check in warlock.config() and when merge is true and the key is also in the user conf, we react specially to it. Can you think of a use case where this would even be needed?

FYI, the same function as warlock.config() for the user conf only is exposed as warlock.config.user(), the difference being that the runtime values are not merged in. Therefore your test could be rewritten like this, I think:

it 'override an array value in the loaded config, it\'s still there when asked for again', ( done )->
  bootstrap._readConfig 'test/fixtures/config/user.json', 'test/fixtures/config/package.json'
    .then ->
      try
        config.user( 'blah.an_array', 'item_2', false ).should.eql 'item_2'
        config.user( 'blah.an_array' ).should.eql 'item_2'
        done()
      catch err
        done( err )

BTW - why the try block? Wouldn't a thrown error cause the test to fail anyway? Or is this a Josh-is-too-sleepy-to-see-the-light kind of scenario?

[PaulL1]

Excellent, makes sense, I can work through that.

Mocha + could.js has a defect/issue with asynch tests. Basically it waits for the done() call, but if it fails then done() never gets called, so you get a timeout error instead of the actual failure. Which makes it hard to work out why your test failed. The try/catch is one of the recommended workarounds, there’s a fix on the way bit it relies on a change in the could.js library - we could move to chai which already has that change, but the try catch was easy enough.

mochajs/mocha#278