Closed marco-souza closed 7 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
8e7c930
) 86.65% compared to head (bff9373
) 87.53%.:exclamation: Current head bff9373 differs from pull request most recent head 18bffda. Consider uploading reports for the commit 18bffda to get more accurate results
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@Sorikairox I plan to extract part of this logic into a ScheduleRegistry
to leave the module file cleaner, but the main business logic is ready for review, please take a look when you have the time! 🙏🏼
@marco-souza Overall it looks good !
Not sure an additional "Registry" class is necessary for now, the code is pretty simple and will probably not evolve that much 😁
There are improvements on test indeed 😭 Maybe Deno has something to fake timers. Or we can test with 1 seconds timeouts, intervals and cron. It shouldn't matter if it's a minute or a few seconds, internal behaviours is the same 🤔
@Sorikairox Thanks for the review
Regarding the Registry
class, I considered creating it to move all this registration logic from this module, but I agree this registry
might not be needed, as we only use it here.
About the test cases, testing timeout and intervals will be simple, but the issue with cronjob is that Deno.cron
is based on POSIX crontab, which doesn't support seconds by default - for reference
IMO, if we test timeout and intervals, the registration logic will be tested, which is the same used from @Cron
. Do you think it would be ok if I remove this test case and only validate @Interval
& @Timeout
?
@marco-souza Thank for the explanation and reference. I learned something !
What about a special Deno task to run cron test ? This one can run in the CI (as it's free for open source projects) and our "normal" test task keep its speed ?
@Sorikairox Nice, I think that works fine!
But I was investigating about deno fake timer and I found this: https://deno.land/std@0.216.0/testing/time.ts?s=FakeTime
Let me try it first but if it doesn't work, I'll continue with the CI task approach.
@Sorikairox I was not able to use FakeTime
for Deno.cron
, as it uses an OS-level implementation, while the FakeTime
only mocks the JS runtime tick.
Furtunatelly, I was able to mock Deno.cron
, with a spy
, and validated that Deno.cron
is being called with the proper info 🥳
Yeah, the CronExpression
comes partially from the Nest.js project itself, but the IntervalExpression
enum is new.
How should I put the license in this case?
@marco-souza
A comment on top of the file // Copyright (c) 2017-2024 Kamil Mysliwiec MIT
Description
Task scheduling allows you to schedule arbitrary code (methods/functions) to execute at a fixed date/time, at recurring intervals, or once after a specified interval. In the Linux world, this is often handled by packages like cron at the OS level. For Deno apps, we have a native module (yet in
unstable
) calledDeno.cron
, to provide cron-like functionalityReferences:
Issue Ticket Number
Fixes #78
Type of change
Checklist:
deno lint
ANDdeno fmt
ANDdeno task test
and got no errors.