-
Notifications
You must be signed in to change notification settings - Fork 625
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
feat: support async handling and add CronJob status tracking #894
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for your PR. It is overall very good but needs a few adjustments in order to avoid introducing breaking changes in the current behavior.
As explained in my comments, this change should be opt-in by adding an option on the CronJob
declaration and in the stop
method.
I remain at your disposal for any questions or further discussions on the topic!
# Improve async handling and add status tracking This commit introduces the following changes: 1. Change the `fireOnTick()` method to return a `Promise` to handle asynchronous callbacks more effectively. 2. Add a new `isRunning` property to track the running state of the `CronJob` instance. 3. Update the test suite to use the new asynchronous behavior and track the running state of the job. These changes ensure that the `CronJob` class can properly handle asynchronous callbacks, and provide a way to track the running state of the job, which can be useful for various use cases.
…outs # Short Description The changes in this commit remove repeated `callback` variables from the cron tests and adjust the test timeouts to use the `tickAsync` method instead of `tick`. ## Detailed Changes 1. Removed repeated `callback` variables from several test cases. 2. Adjusted the test timeouts to use the `tickAsync` method instead of `tick` to ensure the tests properly wait for the asynchronous operations to complete. 3. Removed the `done` callback from the test cases and instead used the `async` function syntax to handle the asynchronous nature of the tests. ## Motivation These changes improve the readability and maintainability of the cron tests by removing unnecessary variables and ensuring the tests properly handle the asynchronous nature of the CronJob functionality.
This commit introduces a new test case to ensure that the `CronJob` class waits for the running callback to complete before stopping the job. The changes include: - Adding a new test case `should wait for running callback to complete before stopping` that simulates a long-running callback and verifies that the job is not stopped until the callback has finished executing.
…est timeouts" This reverts commit 6b35879.
d9d620d
to
95241c7
Compare
…acks being skipped Co-authored-by: Chooooo <contact@choo.ooo>
🎉 This PR is included in version 3.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
||
await clock.tickAsync(3500); | ||
|
||
expect(executionOrder).toEqual([1, 2]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if new jobs that would start before the previous job is finished are skipped entirely, the job that starts at the first second and is still running at 2 seconds (because it needs to wait 1.5 seconds before finishing) should mean the second job that normally would have started at 2 seconds is skipped, right? so the array should actually just equal [1]
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, the array there should just equal [1]
.
Here's a version of that test that also tracks/asserts the number of callbacks started:
it('should track multiple running jobs correctly', async () => {
const clock = sinon.useFakeTimers();
const executionOrder: number[] = [];
let started = 0;
const job = new CronJob(
'* * * * * *',
async () => {
started++;
await new Promise<void>(resolve => {
setTimeout(() => {
callback();
resolve();
}, 1500);
});
const jobNumber = executionOrder.length + 1;
executionOrder.push(jobNumber);
},
null,
true,
null,
null,
false,
null,
false,
true // waitForCompletion: true
);
await clock.tickAsync(3500);
expect(started).toBe(2);
expect(executionOrder).toEqual([1]);
job.stop();
});
Is the issue on this line?
Line 210 in f4e9e02
if (!this.waitForCompletion && this._isCallbackRunning) return; |
If the !
is removed all the tests pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right. I'll double check the tests and push a fix
<!--- Provide a general summary of your changes in the Title above (following the Conventional Commits standard) --> <!-- More infos: https://www.conventionalcommits.org --> <!-- Commit types: https://github.com/insurgent-lab/conventional-changelog-preset#commit-types--> ## Description <!--- Describe your changes in detail --> fixes #923 ## Related Issue <!--- This project only accepts pull requests related to open issues --> <!--- If suggesting a new feature or change, please discuss it in an issue first --> <!--- If fixing a bug, there should be an issue describing it with steps to reproduce --> <!--- Please link to the issue here: --> #923 ## Motivation and Context <!--- Why is this change required? What problem does it solve? --> it makes the new feature actually work ## How Has This Been Tested? <!--- Please describe in detail how you tested your changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> modified test case added by #894 ## Screenshots (if appropriate): ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [x] My code follows the code style of this project. - [ ] My change requires a change to the documentation. - [ ] I have updated the documentation accordingly. - [x] I have added tests to cover my changes. - [x] All new and existing tests passed. - [ ] If my change introduces a breaking change, I have added a `!` after the type/scope in the title (see the Conventional Commits standard).
## [3.3.0](kelektiv/node-cron@v3.2.1...v3.3.0) (2024-12-10) ### ✨ Features * support async handling and add CronJob status tracking ([kelektiv#894](kelektiv#894)) ([b58fb6b](kelektiv@b58fb6b)), closes [kelektiv#713](kelektiv#713) [kelektiv#556](kelektiv#556) ### ⚙️ Continuous Integrations * **action:** update github/codeql-action action to v3.27.2 ([kelektiv#912](kelektiv#912)) ([d11ba30](kelektiv@d11ba30)) * **action:** update github/codeql-action action to v3.27.5 ([kelektiv#917](kelektiv#917)) ([2a4035e](kelektiv@2a4035e)) * **action:** update step-security/harden-runner action to v2.10.2 ([kelektiv#920](kelektiv#920)) ([26a8f9f](kelektiv@26a8f9f)) * add pre-commit hook to lint and prettify ([kelektiv#911](kelektiv#911)) ([e1140d1](kelektiv@e1140d1)), closes [kelektiv#907](kelektiv#907) ### ♻️ Chores * **deps:** lock file maintenance ([94465ae](kelektiv@94465ae)) * **deps:** lock file maintenance ([23d67a4](kelektiv@23d67a4)) * **deps:** lock file maintenance ([135fdf7](kelektiv@135fdf7)) * **deps:** lock file maintenance ([edcff3b](kelektiv@edcff3b)) * **deps:** pin dependency lint-staged to 15.2.10 ([kelektiv#916](kelektiv#916)) ([5cf24da](kelektiv@5cf24da)) * **deps:** update dependency [@commitlint](https://github.com/commitlint)/cli to v19.6.0 ([9d9ab94](kelektiv@9d9ab94)) * **deps:** update dependency [@types](https://github.com/types)/node to v20.17.7 ([9181b6a](kelektiv@9181b6a)) * **deps:** update dependency [@types](https://github.com/types)/node to v20.17.8 ([5899fc2](kelektiv@5899fc2)) * **deps:** update dependency [@types](https://github.com/types)/node to v20.17.9 ([ca5065a](kelektiv@ca5065a)) * **deps:** update dependency husky to v9.1.7 ([a960a29](kelektiv@a960a29)) * **deps:** update dependency typescript to v5.7.2 ([3447ff5](kelektiv@3447ff5))
## [3.3.1](kelektiv/node-cron@v3.3.0...v3.3.1) (2024-12-12) ### 🐛 Bug Fixes * correct waitForCompletion behavior ([kelektiv#924](kelektiv#924)) ([f6270f8](kelektiv@f6270f8)), closes [kelektiv#923](kelektiv#923) [kelektiv#923](kelektiv#923) [kelektiv#894](kelektiv#894)
Description
This PR improves async handling in the CronJob class and adds status tracking functionality.
fireOnTick()
method to return a Promise for better async callback handlingisCallbackRunning
flag to track the running state of CronJob instanceswaitForCompletion
functionality to thejob.stop()
methodonComplete
callbackDuring test case writing, I encountered a type error with sinon.
To resolve this, added
sinon.restore()
to theafterEach
block.Reference: https://stackoverflow.com/questions/73232999/sinon-cant-install-fake-timers-twice-on-the-same-global-object
Related Issue
Closes #713
Closes #556
Motivation and Context
These changes allow the CronJob class to handle asynchronous callbacks more effectively and provide a way to track the running state of jobs.
How Has This Been Tested?
tickAsync
methodTypes of changes
Checklist:
!
after the type/scope in the title (see the Conventional Commits standard).