Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

GH-36 Changes for cron syntax compat (dayOfWeek 0-6 and no-seconds syntax support) #41

Merged
merged 3 commits into from

4 participants

@danhbear

Here are the changes for cron syntax compatibility. The parsing scenarios are really a matter of preference. I'm actually not certain what Unix cron does.

  • If less than 5 digits, should CronTime just throw an error immediately? Previously the code assumed the first n digits matched the beginning of a correct cron time. Now it assumes the first n digits match the end of a correct cron time. This is to support the standard 5-digit.
  • I included CronTime.parseDefaults to keep other digits' parse defaults the same as before. Not sure what desired behavior is here.

Personally as a consumer I'd prefer it throw for <5 to avoid any confusion on the above.

@travisbot

This pull request fails (merged 7102fdb into 36eb441).

@danhbear

@ncb000gt Not sure why this is failing. I ran the tests locally and they passed on 3 of the 4 jobs.

tests/test-cron.js
@@ -34,6 +34,19 @@ module.exports = testCase({
assert.done();
}, 5250);
},
+ 'test standard cron no-seconds syntax doesnt send on seconds (* * * * *)': function(assert) {
+ assert.expect(1);
+ var called = 0;
+ var c = new cron.CronJob('* * * * *', function() {
+ called++;
+ }, null, true);
+ setTimeout(function() {
+ c.stop();
+ // Note that it could be called once if the test lands on a minute boundary
@ncb000gt Owner

You can account for this by checking to see if the test is within 6 seconds of the boundary. If it is, just set a timeout to fire after 5 seconds...many of the tests in this suite are like that due to the time sensitive nature of the tests. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ncb000gt
Owner

The build looks like it failed after it checked out the changes for the tests with v0.8.x. Very strange. I can probably get the tests to run again...just gotta look into it.

@ncb000gt
Owner

Bleh, it looks like i can only force it to refresh master for the time being. If you make the change to the test to support the minute boundary it should kick it off again.

@ncb000gt
Owner

For linkage. This fixes GH-36.

@travisbot

This pull request passes (merged 8205bc7 into 36eb441).

@danhbear

Alrighty, travis test glitch seems fixed. Let me know if 8205bc7 addresses your comment in the CR.

@ncb000gt
Owner

That looks perfect. I had to do this back when I moved to Travis due to the fact that I couldn't time the tests myself. :)

I'll merge this into a branch for a bit more review and testing. Thanks!

@danhbear

Any update on merging this in?

@ncb000gt ncb000gt merged commit 8205bc7 into ncb000gt:master
@ncb000gt
Owner

Herp derp. I completely blanked on this. Too busy. :)

Anyway, it is in master and the tests are all passing. I'll be updating the readme and version number to point out the breaking changes (DoW). Then I'll publish the changes out.

Thanks again.

@danhbear

Awesome, thanks! Yeah would be good to update the version or let people know, not sure the proper node etiquette. Depending on their npm usage of node-cron is seems like they might get in trouble. For example, we have some services that do an npm install/update during each service deploy.

@ncb000gt
Owner

Right. But if people are versioning their production code correctly then the package.json should use explicit versions. I'm unaware of any other way besides in the repository to really inform users that there are breaking changes.

@danhbear

Makes sense. I should probably avoid things like package.json entries like ">=0.1.13" and ">=0.0".

@ncb000gt
Owner

I think you'll find that it will usually be fine, but there will be that one case where something changes in a very unexpected way and poof, no more DB or something like that. :) Only takes one of those to happen before you start getting REAL specific- not that I'm speaking from experience or anything...I mean, who loses data...really...cough

@ncb000gt
Owner

I published this to NPM. Thank you for the code and enjoy! :)

@scien

Here's a great article for package dependencies standards. http://blog.nodejitsu.com/package-dependencies-done-right.

So I use entries like 0.2.x (major.minor.patch), with the (hopefully true) assumption that patches are non-breaking and good to have, but upgrade a major or minor version should be investigated/tested before integration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
8 README.md
@@ -7,7 +7,7 @@ Originally this project was a NodeJS fork of [James Padolsey's][jamespadolsey] [
After [Craig Condon][crcn] made some updates and changes to the code base this has evolved to something that has a bit of both. The cron syntax parsing is mostly James' while using timeout instead of interval is Craig's.
-Additionally, this library goes beyond the basic cron syntax and allows you to supply a Date object. This will be used as the trigger for your callback. Cron syntax is still an acceptable CronTime format.
+Additionally, this library goes beyond the basic cron syntax and allows you to supply a Date object. This will be used as the trigger for your callback. Cron syntax is still an acceptable CronTime format. Although the Cron patterns suported here extend on the standard Unix format to support seconds digits, leaving it off will default to 0 and match the Unix behavior.
Usage (basic cron usage):
==========
@@ -25,13 +25,13 @@ Available Cron patterns:
Ranges. E.g. 1-3,5
Steps. E.g. */2
-[Read up on cron patterns here](http://help.sap.com/saphelp_xmii120/helpdata/en/44/89a17188cc6fb5e10000000a155369/content.htm).
+[Read up on cron patterns here](http://crontab.org).
Another cron example
==========
var cronJob = require('cron').CronJob;
- var job = new cronJob('00 30 11 * * 2-6', function(){
+ var job = new cronJob('00 30 11 * * 1-5', function(){
// Runs every weekday (Monday through Friday)
// at 11:30:00 AM. It does not run on Saturday
// or Sunday.
@@ -60,7 +60,7 @@ For good measure
var cronJob = require('cron').CronJob;
var job = new cronJob({
- cronTime: '00 30 11 * * 2-6',
+ cronTime: '00 30 11 * * 1-5',
onTick: function() {
// Runs every weekday (Monday through Friday)
// at 11:30:00 AM. It does not run on Saturday
View
20 lib/cron.js
@@ -26,10 +26,11 @@ function CronTime(source, zone) {
};
CronTime.map = ['second', 'minute', 'hour', 'dayOfMonth', 'month', 'dayOfWeek'];
-CronTime.constraints = [ [0, 59], [0, 59], [0, 23], [1, 31], [0, 11], [1, 7] ];
+CronTime.constraints = [ [0, 59], [0, 59], [0, 23], [1, 31], [0, 11], [0, 6] ];
+CronTime.parseDefaults = [ '0', '*', '*', '*', '*', '*' ];
CronTime.aliases = {
jan:0, feb:1, mar:2, apr:3, may:4, jun:5, jul:6, aug:7, sep:8, oct:9, nov:10, dec:11,
- sun:1, mon:2, tue:3, wed:4, thu:5, fri:6, sat:7
+ sun:0, mon:1, tue:2, wed:3, thu:4, fri:5, sat:6
};
@@ -107,7 +108,7 @@ CronTime.prototype = {
continue;
}
- if (!(date.getDay()+1 in this.dayOfWeek)) {
+ if (!(date.getDay() in this.dayOfWeek)) {
date.setDate(date.getDate()+1);
date.setHours(0);
date.setMinutes(0);
@@ -179,11 +180,14 @@ CronTime.prototype = {
throw new Error('Unknown alias: ' + alias);
}),
split = source.replace(/^\s\s*|\s\s*$/g, '').split(/\s+/),
- cur, len = 6;
-
- while (len--) {
- cur = split[len] || '*';
- this._parseField(cur, CronTime.map[len], CronTime.constraints[len]);
+ cur, i = 0, len = CronTime.map.length;
+
+ for (; i < CronTime.map.length; i++) {
+ // If the split source string doesn't contain all digits,
+ // assume defaults for first n missing digits.
+ // This adds support for 5-digit standard cron syntax
+ cur = split[i - (len - split.length)] || CronTime.parseDefaults[i];
+ this._parseField(cur, CronTime.map[i], CronTime.constraints[i]);
}
},
View
20 tests/test-cron.js
@@ -34,6 +34,26 @@ module.exports = testCase({
assert.done();
}, 5250);
},
+ 'test standard cron no-seconds syntax doesnt send on seconds (* * * * *)': function(assert) {
+ assert.expect(0);
+ // Delay test from running at minute boundary
+ var prepDate = new Date();
+ if (prepDate.getSeconds() >= 55) {
+ setTimeout(testRun, 5000);
+ } else {
+ testRun();
+ }
+
+ function testRun() {
+ var c = new cron.CronJob('* * * * *', function() {
+ assert.ok(true);
+ }, null, true);
+ setTimeout(function() {
+ c.stop();
+ assert.done();
+ }, 5250);
+ }
+ },
'test every second for 5 seconds with oncomplete (* * * * * *)': function(assert) {
assert.expect(6);
var c = new cron.CronJob('* * * * * *', function(done) {
View
15 tests/test-crontime.js
@@ -37,6 +37,21 @@ module.exports = testCase({
});
assert.done();
},
+ 'test no second digit doesnt throw, i.e. standard cron format (8 8 8 8 5)': function(assert) {
+ assert.expect(1);
+ assert.doesNotThrow(function() {
+ new cron.CronTime('* * * * *');
+ });
+ assert.done();
+ },
+ 'test no second digit defaults to 0, i.e. standard cron format (8 8 8 8 5)': function(assert) {
+ assert.expect(1);
+ var now = new Date();
+ var standard = new cron.CronTime('8 8 8 8 5');
+ var extended = new cron.CronTime('0 8 8 8 8 5');
+ assert.ok(standard._getNextDateFrom(now).getTime() === extended._getNextDateFrom(now).getTime());
+ assert.done();
+ },
'test hyphen (0-10 * * * * *)': function(assert) {
assert.expect(1);
assert.doesNotThrow(function() {
Something went wrong with that request. Please try again.