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.

Sign up for GitHub

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

chore: Refactor cronjob controller #3029

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
+91 −53
Open

chore: Refactor cronjob controller #3029

Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
f2d8bed
refactor cronjob controller
hmalik88 Jan 21, 2025
75020fb
undo comment deletion
hmalik88 Jan 21, 2025
bd7c7c2
Merge branch 'main' into hm/refactor-cronjob-controller
hmalik88 Jan 21, 2025
33dfc92
fix coverage
hmalik88 Jan 21, 2025
546f791
fix coverage again
hmalik88 Jan 21, 2025
5c7ae9a
Merge branch 'main' into hm/refactor-cronjob-controller
hmalik88 Jan 23, 2025
9452deb
apply code review
hmalik88 Jan 23, 2025
2d380a2
update coverage
hmalik88 Jan 23, 2025
131a290
revert change to destroy method
hmalik88 Jan 23, 2025
f7f4cb5
update coverage
hmalik88 Jan 23, 2025
eafee0f
Merge branch 'main' into hm/refactor-cronjob-controller
hmalik88 Jan 29, 2025
99c20e6
Merge branch 'main' into hm/refactor-cronjob-controller
hmalik88 Feb 26, 2025
88b055c
Merge branch 'main' into hm/refactor-cronjob-controller
hmalik88 Feb 28, 2025
2f4ad03
add missing comment
hmalik88 Feb 28, 2025
02d1fee
Merge branch 'main' into hm/refactor-cronjob-controller
hmalik88 Mar 4, 2025
53e27ac
remove validateTime param
hmalik88 Mar 4, 2025
764ddf5
update coverage
hmalik88 Mar 4, 2025
5ac3bbd
skip code coverage on error logs
hmalik88 Mar 11, 2025
d7a3845
Merge branch 'main' into hm/refactor-cronjob-controller
hmalik88 Mar 27, 2025
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
Prev Previous commit
Next Next commit
Loading
Loading status checks…
skip code coverage on error logs
@hmalik88
hmalik88 committed Mar 11, 2025

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Learn about vigilant mode
commit 5ac3bbde5e8f86cdb1e5150773ae1a4d4e60267d
4 changes: 3 additions & 1 deletion packages/snaps-controllers/src/cronjob/CronjobController.ts
View file
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
@@ -160,6 +160,7 @@ export class CronjobController extends BaseController<
this.#initializeActionHandlers();

this.dailyCheckIn().catch((error) => {
/* istanbul ignore next */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

logError(error);
});

@@ -233,6 +234,7 @@ export class CronjobController extends BaseController<
request: request as Record<string, unknown>,
})
.catch((error) => {
/* istanbul ignore next */
logError(error);
});
}
@@ -529,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.
/* istanbul ignore next */
logError(error);
});
});