Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: issue #74 use local timezone #76

Merged
merged 6 commits into from
Feb 5, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ const {
const buildJob = require('./job-builder');
const validateJob = require('./job-validator');

later.date.localTime();

// bthreads requires us to do this for web workers (see bthreads docs for insight)
threads.Buffer = Buffer;

Expand Down
39 changes: 37 additions & 2 deletions test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,41 @@ test('fails if cron pattern is invalid', (t) => {
);
});

test.serial('job created with cron string is using local timezone', (t) => {
t.plan(1);
const bree = new Bree({
root,
jobs: [{ name: 'basic', cron: '0 18 * * *' }]
});

const clock = FakeTimers.install({ now: Date.now() });
bree.start('basic');
bree.on('worker created', () => {
const now = new Date(clock.now);
t.is(now.getHours(), 18);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

want to be testing the actual offset not just what the hours say.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I added the offset check in the tests.

});
clock.next();
clock.uninstall();
});

test.serial('job created with human interval is using local timezone', (t) => {
t.plan(2);
const bree = new Bree({
root,
jobs: [{ name: 'basic', interval: 'at 13:26' }]
});

const clock = FakeTimers.install({ now: Date.now() });
bree.start('basic');
bree.on('worker created', () => {
const now = new Date(clock.now);
t.is(now.getHours(), 13);
t.is(now.getMinutes(), 26);
});
clock.next();
clock.uninstall();
});

test('fails if closeWorkersAfterMs is <= 0 or infinite', (t) => {
t.throws(
() =>
Expand Down Expand Up @@ -444,7 +479,7 @@ test.serial('run > job terminates after set time', async (t) => {
await new Promise((resolve, reject) => {
bree.workers.infinite.on('error', reject);
bree.workers.infinite.on('exit', (code) => {
t.true(code === 1);
t.is(code, 1);
resolve();
});
});
Expand All @@ -467,7 +502,7 @@ test.serial('run > job terminates before set time', async (t) => {
await new Promise((resolve, reject) => {
bree.workers.basic.on('error', reject);
bree.workers.basic.on('exit', (code) => {
t.true(code === 0);
t.is(code, 0);
resolve();
});
});
Expand Down