Closed knicola closed 2 years ago
@niftylettuce @shadowgate15 thoughts?
Okay so got a few things:
FakeTimers
, make sure to add .serial
to the test call or it will mess up other tests and become flaky.t.plan(n)
at the beginning of the test. (That would be anywhere you are adding it to the setTimeout
function.) This just makes sure that no matter what happens if the expected assertions dont get called tests will fail.FakeTimers
you need to overwrite the clock
setTimeout()
not global
. FakeTimers
is already removing global so it won't work. See here..This is looking really good so far!!!!
The CI run for v10 and v12, might be failing because of const next = (() => { ... })();
. I'll test my theory later today with those 2 versions and if that's not the case I'll try adding a delay.
But the run for node v14 seems to be failing because of poor coverage. This might be a good time to talk about the copied later.setTimeout()
and later.setInterval()
code. Write/copy over tests from @breejs/later
to bree
, exclude from coverage or should we talk about migrating timezone changes back to later
?
yup, so date.toLocaleString()
behaves differently in earlier versions of node. might have to find a different way to get the target zone's offset to support v10 and v12. any objections on using a 3rd party lib instead? it'll be the first later
dep, if it's decided to move tz changes over.
Merging #126 (cf43928) into master (ad91421) will not change coverage. The diff coverage is
100.00%
.:exclamation: Current head cf43928 differs from pull request most recent head a629519. Consider uploading reports for the commit a629519 to get more accurate results
@@ Coverage Diff @@
## master #126 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 5 +1
Lines 397 455 +58
=========================================
+ Hits 397 455 +58
Impacted Files | Coverage Δ | |
---|---|---|
src/index.js | 100.00% <100.00%> (ø) |
|
src/job-builder.js | 100.00% <100.00%> (ø) |
|
src/job-validator.js | 100.00% <100.00%> (ø) |
|
src/later-tz-patch.js | 100.00% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update ad91421...a629519. Read the comment docs.
sorry for the delay @knicola I would say we should go ahead and move to @breejs/later
. I think it is a better long term solution.
@shadowgate15 should we close this?
For anyone checking on this, it doesn't look like an equivalent PR has been opened against breejs/later yet.
my bad. I haven't been able to successfully migrate the tests over to breejs/later
, then life happened. I'll take another shot at it this week
@shadowgate15 tz patch needs to be replaced with @breejs/later 4.1.0
yep currently working on that
cool, thanks!
Issue https://github.com/breejs/bree/issues/72
Changelog
Added:
cron
,interval
andtimeout
schedules.timezone
option to instance options. Default value islocal
timezone
option to job options.Notes
This PR does not support timezone conversion for
job.date
yet because bree usessafe-timers
for that instead of@breejs/later
, where most of the tz work was done. It's fairly easy to supportjob.date
as well, but I'm wondering whether it's necessary sinceDate
covers that.