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.

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

Merged
merged 17 commits into from
Dec 10, 2024

Conversation

Zamoca42
Copy link
Contributor

@Zamoca42 Zamoca42 commented Sep 4, 2024

Description

This PR improves async handling in the CronJob class and adds status tracking functionality.

  • Modified the fireOnTick() method to return a Promise for better async callback handling
  • Added an isCallbackRunning flag to track the running state of CronJob instances
  • Updated the test suite to use the new async behavior and track the job's running state
  • Added waitForCompletion functionality to the job.stop() method
    • waits for running jobs to complete before executing the onComplete callback

During test case writing, I encountered a type error with sinon.
To resolve this, added sinon.restore() to the afterEach block.

Reference: https://stackoverflow.com/questions/73232999/sinon-cant-install-fake-timers-twice-on-the-same-global-object

스크린샷 2024-09-03 오후 7 10 58

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?

  • Updated existing test suite to verify the new async behavior
  • Adjusted test timeouts to use the tickAsync method
  • Added new test cases to check for proper waiting of running callbacks before stopping

Types of changes

  • 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:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • 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).

Copy link
Collaborator

@sheerlox sheerlox left a 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!

src/job.ts Outdated Show resolved Hide resolved
tests/cron.test.ts Outdated Show resolved Hide resolved
src/job.ts Outdated Show resolved Hide resolved
tests/cron.test.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@intcreator intcreator mentioned this pull request Nov 3, 2024
2 tasks
Zamoca42 and others added 11 commits November 4, 2024 13:18
# 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.
README.md Outdated Show resolved Hide resolved
@Zamoca42 Zamoca42 requested a review from sheerlox November 21, 2024 12:04
@intcreator intcreator merged commit b58fb6b into kelektiv:main Dec 10, 2024
18 checks passed
@ncb000gt
Copy link
Member

🎉 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]);
Copy link
Collaborator

@intcreator intcreator Dec 11, 2024

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?

Copy link

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?

if (!this.waitForCompletion && this._isCallbackRunning) return;

If the ! is removed all the tests pass.

Copy link
Collaborator

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

intcreator added a commit that referenced this pull request Dec 12, 2024
<!--- 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).
JosephVoid pushed a commit to JosephVoid/node-cron that referenced this pull request Dec 14, 2024
## [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))
JosephVoid pushed a commit to JosephVoid/node-cron that referenced this pull request Dec 14, 2024
## [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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants