chore: Refactor cronjob controller #3029
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3029 +/- ##
==========================================
+ Coverage 94.98% 95.02% +0.03%
==========================================
Files 511 511
Lines 11313 11312 -1
Branches 1741 1742 +1
==========================================
+ Hits 10746 10749 +3
+ Misses 567 563 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I haven't reviewed this in detail. but generally when I proposed we refactor the CronjobController, the main goal I had in mind was to unify the logic used to manage background events and cronjobs.
We currently treat these as entirely separate entities, but I don't think they need to be. My general thought was that we should investigate treating them as one entity with the only variation being non-recurring versus recurring cronjobs (or background events - no strong opinion on the name).
I had intended for the referenced ticket to be to investigate ways to achieve this.
There was a problem hiding this comment.
This PR unifies some logic, but I inherently found it hard to make a mental model of both being the same thing because the term non-recurring cronjob in of itself is an oxymoron. Maybe there's a better term to encompass both things. One divergence is that we fetch the cron jobs every time from the manifest, maybe if we can fetch once and unify the state then some of the other logic itself can be refactored. We can keep the original ticket open, I think this is an improvement nonetheless.
| */ | ||
| #cleanupTimer(id: string) { | ||
| const timer = this.#timers.get(id); | ||
| if (timer) { |
There was a problem hiding this comment.
Should this throw an error if the timer doesn't exist?
There was a problem hiding this comment.
Hmm I don't think so, this function is only called when cancelling a background event.
| this.#initializeActionHandlers(); | ||
|
|
||
| this.dailyCheckIn().catch((error) => { | ||
| /* istanbul ignore next */ |
There was a problem hiding this comment.
Added ignore statement here (and below) as this scenario is pretty difficult to replicate in our test environment. I don't think it's worth the effort to try and cover these.
There was a problem hiding this comment.
Should we really add an ignore or leave this uncovered in the coverage report ? I feel like it would be better
There was a problem hiding this comment.
I think leaving it uncovered would make sense if we ever intend on trying to cover this, otherwise it's just a nuisance to look at in coverage reports. cc: @FrederikBolding
| @@ -493,7 +531,7 @@ export class CronjobController extends BaseController< | |||
| this.#dailyTimer = new Timer(DAILY_TIMEOUT); | |||
| this.#dailyTimer.start(() => { | |||
| this.dailyCheckIn().catch((error) => { | |||
| // TODO: Decide how to handle errors. | |||
There was a problem hiding this comment.
Why this TODO was removed ?
There was a problem hiding this comment.
Unintended, reverted!
Refactors the
CronjobControllerlogic to deduplicate logic being used across cronjobs and background events.