cron-parser
cron-parser copied to clipboard
harrisiirak
Incorrect test case, '10 2 12 8 6-7'
This test case: https://github.com/harrisiirak/cron-parser/blob/f36281a84612b84395b1b275c7e74a2e5e9b83f1/test/expression.js#L938 If I understand the expression correctly, should be “At 02:10 on day-of-month 12 and on every day-of-week from Saturday through Sunday in August.”
The test case appears to be looking for (in my time zone, at least), the following dates:
- Sat Aug 03 2013 02:10:00 GMT-0700
- Sat Aug 10 2013 02:10:00 GMT-0700
- Mon Aug 12 2013 02:10:00 GMT-0700
- Sat Aug 17 2013 02:10:00 GMT-0700
That is, it's ensuring that Monday the 12th gets in there. However, I think the correct dates are actually:
- Sat Aug 03 2013 02:10:00 GMT-0700
- Sun Aug 04 2013 02:10:00 GMT-0700
- Sat Aug 10 2013 02:10:00 GMT-0700
- Sun Aug 11 2013 02:10:00 GMT-0700
- Mon Aug 12 2013 02:10:00 GMT-0700
- Sat Aug 17 2013 02:10:00 GMT-0700
- Sun Aug 18 2013 02:10:00 GMT-0700
As it stands, the Sundays (the '7' of the ('6-7') aren't being included.
I believe this to be because the correct normalization: https://github.com/harrisiirak/cron-parser/blob/f36281a84612b84395b1b275c7e74a2e5e9b83f1/lib/expression.js#L246 Is performed only on the scalar path, but the '6-7' range is handled on the vector path, and the normalization isn't performed there.
@bazineta thanks for reporting this! It indeed looks like a bug.
When provided as a sequence (10 2 12 8 6,7) it provides the correct result.
And also, when the range doesn't contain Sunday (10 2 12 8 5-6), it also seems to behave as expected and provides the correct result.
Yes, additional test cases for work-alike options (sequences) pass; it seems to be just the range parser that's affected. If I add the same normalization code to the vector path, the corrected test passes, as do all the other tests, so it seems to not cause any regression.
I note that making that change essentially makes, other than iteration, the vector and scalar paths very similar, as, while there's a numeric conversion here: https://github.com/harrisiirak/cron-parser/blob/f36281a84612b84395b1b275c7e74a2e5e9b83f1/lib/expression.js#L235 I'm not sure that's actually necessary there, as by then anything is, I think, already a number, so there may be an opportunity to simplify this particular function quite a bit.
Created a quick fix/workaround for this bug: https://github.com/harrisiirak/cron-parser/commit/b35678c1ee7ef852dacc27c63212f1aa5193a7b5
I'll digest it a bit, maybe there is a bit better/cleaner solution for that.