fix: issue #74 use local timezone #76
Conversation
|
you would also need to add to job-builder.js and job-utils.js. it also might be good to have this as an option in config with the default being local tz. |
|
@shadowgate15 why? For my newly added tests the one place to fix seems to be enough. Could you provide additional tests that make clear the other places also need the extension? And the broken build is a different test that also breaks the PR #75 currently. |
|
if anything, it isn't needed in index, since it is only used to schedule intervals and timeouts. In job-builder, it is used to create a schedule which is then used to set timeouts and intervals. I believe job-utils, uses it similarly. |
|
I'll defer to you @shadowgate15 and @naz to merge/review |
| bree.start('basic'); | ||
| bree.on('worker created', () => { | ||
| const now = new Date(clock.now); | ||
| t.is(now.getHours(), 18); |
There was a problem hiding this comment.
let's change this to the timezone offset instead, so that we are testing explicitly what we want instead of an after effect. you should be able to use now.getTimezoneOffset()
There was a problem hiding this comment.
I think now.getTimezoneOffset() makes assertion unnecessary complex. Because then the result will depend on the timezone the test is executed. For my local system for example (CET) the result would be -60.
The check on now.getHours() simply checks the wanted effect: the cron pattern says "do it at 18:00 o'clock". So when the job gets executed, the hour of now is expected to be 18. Independent of timezone.
There was a problem hiding this comment.
want to be testing the actual offset not just what the hours say.
There was a problem hiding this comment.
Ok, I added the offset check in the tests.
|
Anything I can do to speedup process? |
|
does it still work correctly when you add a new job after bree has been created? |
moved localTime init to job-builder.js; new test to verify functionality when job is added; extended verification of timezone offset in test
| bree.start('basic'); | ||
| bree.on('worker created', () => { | ||
| const now = new Date(clock.now); | ||
| t.is(now.getHours(), 18); |
There was a problem hiding this comment.
I think now.getTimezoneOffset() makes assertion unnecessary complex. Because then the result will depend on the timezone the test is executed. For my local system for example (CET) the result would be -60.
The check on now.getHours() simply checks the wanted effect: the cron pattern says "do it at 18:00 o'clock". So when the job gets executed, the hour of now is expected to be 18. Independent of timezone.
| bree.start('basic'); | ||
| bree.on('worker created', () => { | ||
| const now = new Date(clock.now); | ||
| t.is(now.getHours(), 18); |
There was a problem hiding this comment.
Ok, I added the offset check in the tests.
I have done it in index because it is the first place where |
|
Thanks for all the work @snrmwg! |
My pleasure! Looking forward to the next release of breejs. @shadowgate15 |
|
v5.0.0 released https://github.com/breejs/bree/releases/tag/v5.0.0 🎉 you all rock. thank you @shadowgate15 et all |
Use local timezone for jobs created with cron pattern or intervals written as human-friendly time representations.
Fixes #74.