#3085 Remove unexpected behavior when calling moment with an empty object

[ottoliar]

As mentioned in #3085, calling moment({}) or moment([]) resulted in different behavior than calling moment(), which we agreed was unexpected behavior. By setting the input to the same as it would be as if no empty array or object was passed in, the behavior is more consistent.

[maggiepint]

A couple comments on this:

There is code in the utils folder to check if an object is a date. I would use this instead of the manual check you have here.

Please add unit tests that prove that your code works. I would expect to see tests that take both empty array and empty object, and ensures that the result is exactly the same as what it would have been if moment were invoked on it's own.

Finally, I'm not sure why you're excluding UTC here. Seems like we would want the same behavior even if we're creating UTC. Is there something I'm missing?

[maggiepint]

Thinking about this a little harder, this code probably is a bit hard to test. I would think at the very least we could run the code path though (assert that a moment is produced if an empty array or object is passed).

[icambron]

You could assert that the time produced is +/- one MS of moment()

[ottoliar]

Hi guys sorry for the delayed reply. I redid the PR with your suggestions, please take a look and let me know. Thanks!

[maggiepint]

@ottoliar , based on your tests, I think you are misinterpreting what .now() does. Calling that function simply returns the current milliseconds from the JS Date object.

See the documentation here:
http://momentjs.com/docs/#/customization/now/

Note that the definition of that function is as follows:

export var now = function () {
    return Date.now ? Date.now() : +(new Date());
};

To test your code path, you need to be calling moment({}) and moment([]), but I don't see that here.

It is actually the case that the now function could help you. If you actually look at the unit test that you removed (is there a reason that you removed this?), it is doing almost exactly what you want. The now value is set to a hard-coded time, so it is able to assert that moment construction with an empty array is the same as construction with no parameters, without having to introduce all of the 'within one millisecond' stuff you have going on. @icambron and I should frankly have thought of that earlier.

[icambron]

@icambron and I should frankly have thought of that earlier.

My bad

[ottoliar]

Hi guys thanks for the feedback. I tried adding unit tests to test against the hard-coded time to assert that it was same as calling moment with no object or array passed in.

[ichernev]

@ottoliar I'd like to make the tests more rock solid by getting time before and after moment([]) and asserting it's in between. That is guaranteed not to fail on any device/platform.

Also, as @maggiepint mentioned you should remove the clause for non-utc (or explain why you have it). If we remove that (I think we should) then also add tests for moment.utc.

to do the testing right just make a function that takes a lambda that returns the new moment, get the time, run the fn, get the time again and assert.

[ottoliar]

Thanks I'll try writing those tests soon. I was adding the UTC check to make the test
assert.ok(moment.utc([]).toISOString() === '2015-01-01T00:00:00.000Z', 'moment() constructor should fall back to the date defined by moment.now when an empty array is given, but it did not');
pass. Is there a workaround I could do here without making the UTC check?

[maggiepint]

Circling back around to this now. @ottoliar - calling moment().utc() really doesn't test anything useful here. Instead, as @ichernev and I both said, the utc condition should not be there. Then, the following tests should pass:

assert.equal(moment.utc([]).format(), moment.utc().format(), 'utc moment created with array same as utc moment created with empty constructor');
assert.equal(moment.utc({}).format(), moment.utc().format(), 'utc moment created with object same as utc moment created with empty constructor');

Does that help?

[ottoliar]

Hi Maggie in this test:
assert.equal(moment.utc([]).format(), moment.utc().format(), 'utc moment created with array same as utc moment created with empty constructor');

doesn't that conflict with the already existing test:

assert.ok(moment.utc([]).toISOString() === '2015-01-01T00:00:00.000Z',

This is why I removed that particular test in the earlier commit. It seems this test wants to default back to that value when an array is passed in while the first one wants it to be the same as just calling moment.utc()

[ichernev]

Close in favor of #3238.