merge.all tests and implementation

[Bloomca]

Hi!

As we discussed here, I've made a test file with tests for calling.

It doesn't test any functionality, because actually it depends on implementation.

So, the first object in array is the target, and all others are sources? And all tests for functionality should be the same as in the file test/merge.js? I can add them as well, just want to ensure.

Hope it is what you've meant =)

cc @TehShrike

[TehShrike]

Yup, pretty much what I was thinking! I wonder about that second test though - it might be more appropriate to throw a regular Error in that case instead of TypeError, since the argument was an array, it just didn't have enough elements.

I don't think we need many tests beyond this for functionality, just enough to make sure that the main merge function (which already has a bunch of tests) is being properly called.

So about all I can think of is:

• pass in more than 2 arguments, and make sure that properties from all of them end up in the output - asserting that we're calling the main merge function on every input
• pass in more than 2 arguments, and when the clone option is true, assert that an object property on each of the inputs is different from the object property in the output - asserting that we're passing in the options with every call to the main merge function

Do you want to add those tests and the implementation in this branch/pull request? I'm fine leaving this pull request open, or we can merge your changes into a merge-all branch on this repo and open future pull requests against that branch, either way.

[Bloomca]

Yeah, it makes sense, I'll replace it with normal Error type.

Okay, I'll add these tests for functionality, and I can push them here as well, so you could review them immediately after adding.

Then we can merge, or simple close this PR, and just reopen it in another branch – either way works with me. For now I think I'll add missing tests here.

[Bloomca]

@TehShrike Okay, I've added few tests for functionality, but I feel that I am doing it wrong way. What I feel would be much better approach, is just to spy merge function and check with which arguments and how many times it was invoked. With this approach we will be sure that we invoked correct number of times with correct arguments, which basically justifies our correctness.

What are your thoughts?

[TehShrike]

I'm not a huge fan of spies on internal functions in cases like this, it gets down the path of asserting things about the implementation details instead of the inputs and outputs.

These tests look good to me, though I suspect that last test would pass even if you set clone to false. I think each of the initial objects would need to have properties with different names for the merge algorithm to ever consider not cloning them in the first place.

var firstObject = { link1: { example: 123 } }
var secondObject = { link2: { example: true } }
var thirdObject = { link3: { example: 'string' } }

We should just make sure that the assertions in that test fail when clone is false after the implementation is written.

Thanks for writing these tests! Do you want to write the implementation and push it up in this pull request too?

[Bloomca]

@TehShrike okay, it makes sense, thank you!

So I will change the last test then.

And yeah, I'd like to implement it too! Should I push it here too, and then just squash all commits after approval?

[TehShrike]

Yup, just push it into this branch and that'll be fine! Don't worry about squashing commits unless you really want to, I don't mind having a history of the implementation steps.

[Bloomca]

@TehShrike I've updated tests and add implementation, could you please take a look?

Also we need to update version to 1.3.0 and update changelog, am I right?

[TehShrike]

This looks pretty great to me! I'll merge it in a couple days if nobody else notices anything.

The changelog and readme should be updated, but don't update the version number in the package.json file, I'll do that when I deploy.

[Bloomca]

Okay, wonderful.

Would like to hear feedback!

[TehShrike]

Oh, I just noticed one formatting thing - that new code is indented with two spaces, but the rest of the file is indented with four.

[Bloomca]

Fixed. Yeah, too spoiled with linting.

[TehShrike]

Published as 1.3.0. Thanks! :-D