webroo / dummy-json

Generates random dummy JSON data in Node.js
MIT License
380 stars 61 forks source link

Fix Timezone error on 'time' getDate helper #12

Closed lnfnunes closed 7 years ago

lnfnunes commented 8 years ago

Hi, I'm from Brazil and the test assserts for 'time' helper was failling due to timezone difference (GMT -3).

Before fix:

77 passing (211ms) 2 failing 1) helpers time should return different time when used repeatedly: AssertionError: '09:28, 09:19, 06:45' deepEqual '12:28, 12:19, 09:45'

  • expected - actual
  -09:28, 09:19, 06:45
  +12:28, 12:19, 09:45

2) helpers time should return a time formatted using the format string: AssertionError: '09:28:17' deepEqual '12:28:17'

  • expected - actual
  -09:28:17
  +12:28:17

After fix:

79 passing (186ms)

webroo commented 8 years ago

Thanks for bringing this up and proposing a fix. I'll take a look at the changes in detail as soon as I can.

webroo commented 8 years ago

Thanks, your fix works and solves the problem. I would like to merge it but unfortunately it will permanently change the generated values for anyone using a seed. This means I would have to do a major version bump to the semver as it might break people's existing codebases. I would like to wait for a while before doing that, so I'm going to put your fix on-hold for a bit.

FYI the breaking change only occurs in edge cases where the random dates fall on a day boundary. For example, if I try to generate a date between exactly the same two values I end up with two different years:

// Before fix
dummyjson.parse('{{date "1970-01-01" "1970-01-01" "YYYY-MM-DD"}}')
// '1969-12-31'

// After fix
dummyjson.parse('{{date "1970-01-01" "1970-01-01" "YYYY-MM-DD"}}')
// '1970-01-01'

Unfortunately my tests never covered this situation, but I will add them.

Thanks again, I'll make sure this lands in the next major release.

lnfnunes commented 8 years ago

Hum... sorry but I'm not sure if I understood! Are you saying the "Before fix" should be truthy ? Why the date 1 day before the minimun argument? The "After fix" version looks the correct one to me!

webroo commented 8 years ago

The "After fix" version is correct! Your fix works perfectly :)

However, I was demonstrating that it might change the generated output for some people. This means I must update the version of dummy-json to 2.0.0, as per semver. (If people are using dummy-json in their own projects and writing tests against the output then we might break their code.)

I will merge this fix when I am ready to release 2.0.0, but I would like to wait and add some more features before then.

lnfnunes commented 8 years ago

Ok, tks! I'll wait for this version then. This package is amazing and very usefull. Great job ;)

webroo commented 7 years ago

This has now been fixed in version 2.0.0