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

[Bug] interval setting does seem to ignore startAt #247

Closed
CommanderStorm opened this issue May 4, 2024 · 7 comments · Fixed by #248
Closed

[Bug] interval setting does seem to ignore startAt #247

CommanderStorm opened this issue May 4, 2024 · 7 comments · Fixed by #248
Labels

Comments

@CommanderStorm
Copy link

Describe the bug
Hi, first thanks a ton for the library. We have been (ab)using it a lot.

I think we have come across what I think is a bug in the implementation of #63:
If startAt is in the past, interval does not calculate starting from startAt but is skippped instead (We think).

To Reproduce
Steps to reproduce the behavior:

  1. enter startAt and an absurdly large interval (to make what I think is a bug more obvious ^^)
  2. Run code
    https://jsfiddle.net/ak85Lwrv/4/
    image

Expected behavior

I would expect that the first execution of the cronjob is not "now" and then the interval comes into play, but rather that this respects startAt.

System:

  • OS: Manjraro/Linux
  • Runtime Chrome/JSfiddle
  • Version 124.0.6367.91

Additional context
This issue is the outcome of the investigation of @buzzinJohnnyBoi in louislam/uptime-kuma#4738

@Hexagon
Copy link
Owner

Hexagon commented May 4, 2024

Oh! Will have a look at this as soon as possible. Thanks for a great bug report!

@buzzinJohnnyBoi
Copy link

buzzinJohnnyBoi commented May 11, 2024

Hello, @Hexagon, I gave finding the bug a shot, and I think I have fixed it.
In the /src/croner.js file (_next function), I just added a check !prev && options.startAt && options.interval, then calculate when prev (previous run) should have been. I have tested this with multiple different values, and it gets the correct value each time.
All tests and linters pass, and I have run npm run build and the tests for dist pass as well.
I also added a test in /src/suites/options.cjs with the startAt in the past and the excepted date in the future.
Is it OK if I create a PR for this issue?
My Commit

@Hexagon
Copy link
Owner

Hexagon commented May 11, 2024

@buzzinJohnnyBoi Awesome, send the PR 👍

@buzzinJohnnyBoi
Copy link

I sent the PR, wasn't sure if you wanted me to include the changes after running npm run build, so I looked at some of your previous PRs and it looks like you do that sometimes, so I included them :)

@Hexagon
Copy link
Owner

Hexagon commented May 13, 2024

@buzzinJohnnyBoi's fix available to test through npm i croner@8.0.3-dev.0

@CommanderStorm
Copy link
Author

Closing as #248 was merged ^^
Thanks everyone ❤️

@Hexagon
Copy link
Owner

Hexagon commented May 18, 2024

An additional fix by @buzzinJohnnyBoi released through prerelease 8.0.3-dev.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants