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

Incorrect next execution time for "month" and "day of week" #59

Closed
mjsaber opened this issue Dec 11, 2015 · 6 comments
Closed

Incorrect next execution time for "month" and "day of week" #59

mjsaber opened this issue Dec 11, 2015 · 6 comments

Comments

@mjsaber
Copy link
Contributor

mjsaber commented Dec 11, 2015

    CronDefinition cronDefinition = CronDefinitionBuilder.instanceDefinitionFor(CronType.UNIX);
    CronParser parser = new CronParser(cronDefinition);
    String crontab = "* * */3 */4 */5";
    Cron cron = parser.parse(crontab);
    ExecutionTime executionTime = ExecutionTime.forCron(cron);
    DateTime scanTime = new DateTime();
    DateTime nextExecutionTime = executionTime.nextExecution(scanTime);
    System.out.println(String.format("Scan time %s, next execution time: %s", scanTime, nextExecutionTime));

16:32:56.633 [main] DEBUG c.cronutils.model.time.ExecutionTime - computing days for [class com.cronutils.model.field.expression.Every]
16:32:56.637 [main] DEBUG c.cronutils.model.time.ExecutionTime - computing days 4
16:32:56.639 [main] DEBUG c.cronutils.model.time.ExecutionTime - computing days for [class com.cronutils.model.field.expression.Every]
16:32:56.639 [main] DEBUG c.cronutils.model.time.ExecutionTime - computing days 4
Scan time 2015-12-10T16:32:56.586-08:00, next execution time: 2015-12-10T16:33:00.000-08:00
(12/10/2015 is Thursday)

This is incorrect. Month */4 should mean Jan, May and Sep because month entry range should be (1-12), now it looks like it calculated from 0. In terms of Day of Week */5, it should be Sun, Fri, but not Thu.

@jmrozanec
Copy link
Owner

@mjsaber thank you for reporting this. Help is appreciated!

@mjsaber
Copy link
Contributor Author

mjsaber commented Dec 11, 2015

I'd like to help. Any idea how should I start? Or what is the best practice to go through the code?

@jmrozanec
Copy link
Owner

@mjsaber I just uploaded two tests with the reported case, one for checking months behavior, another for DoW assertions. I did not made specific assertions on them. Can you check what assertions are required, and debug from there on what should be changed? Months scale is definitively an important code point to check. Let me know if I can help with further context in any piece of code. Thank you!

@mjsaber
Copy link
Contributor Author

mjsaber commented Jan 26, 2016

@jmrozanec I think the implementation of
public int generateNextValue(int reference) throws NoSuchValueException
in EveryFieldValueGenerator is not correct for DoM. Take * * */3 * * as an example: generateCandidatesNotIncludingIntervalExtremes will give 3, 6, 9 ... as the result. However, it should be 1, 4, 6...
I think I can help improve the implementation, how should I purpose a push request? I'm new to github..

Thanks,

@jmrozanec
Copy link
Owner

Hello @mjsaber! First you would need to fork this repo, then implement the changes and commit to your fork. When you are ready, you issue a pull request. I will review it, and merge into this repo. When proposing a fix, please provide some unit tests, to ensure the case is covered.
Let me know if I can provide further help.

@jmrozanec
Copy link
Owner

@mjsaber Fixed and closed! Thanks for contributing a fix!

@jmrozanec jmrozanec added the done label Mar 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants