From 11fab60d95a866bc19b4718252725f53d32cec80 Mon Sep 17 00:00:00 2001 From: lipsill Date: Sun, 12 Aug 2018 12:34:24 +0200 Subject: [PATCH 1/5] [Watcher] Improved error messages for CronEvalTool CronEvalTool prints an error only for cron expressions that result in no coming time events. If a cron expression results in less than the specified `-count` (default 10) time events, the CronEvalTool prints all the coming times and displays no error message. Fixes #32735 --- .../trigger/schedule/tool/CronEvalTool.java | 18 ++++++++---- .../schedule/tool/CronEvalToolTests.java | 28 +++++++++++++++++++ 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java index 33b1217895dca..c996f3758072b 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java @@ -5,6 +5,7 @@ */ package org.elasticsearch.xpack.watcher.trigger.schedule.tool; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -57,18 +58,25 @@ void execute(Terminal terminal, String expression, int count) throws Exception { DateTime date = DateTime.now(DateTimeZone.UTC); terminal.println("Now is [" + formatter.print(date) + "]"); - terminal.println("Here are the next " + count + " times this cron expression will trigger:"); Cron cron = new Cron(expression); long time = date.getMillis(); - for (int i = 0; i < count; i++) { + int i = 0; + List times = new ArrayList<>(); + + for (; i < count; i++) { long prevTime = time; time = cron.getNextValidTimeAfter(time); if (time < 0) { - throw new UserException(ExitCodes.OK, (i + 1) + ".\t Could not compute future times since [" - + formatter.print(prevTime) + "] " + "(perhaps the cron expression only points to times in the past?)"); + if (i == 0) { + throw new UserException(ExitCodes.OK, "Could not compute future times since [" + + formatter.print(prevTime) + "] " + "(perhaps the cron expression only points to times in the past?)"); + } + break; } - terminal.println((i+1) + ".\t" + formatter.print(time)); + times.add((i + 1) + ".\t" + formatter.print(time)); } + terminal.println((i == count ? "Here are the next " : "There are ") + i + " times this cron expression will trigger:"); + times.forEach(terminal::println); } } diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalToolTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalToolTests.java index 2238842494817..d336317cd10bf 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalToolTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalToolTests.java @@ -8,6 +8,10 @@ import org.elasticsearch.cli.Command; import org.elasticsearch.cli.CommandTestCase; +import java.util.Calendar; +import java.util.Locale; +import java.util.TimeZone; + public class CronEvalToolTests extends CommandTestCase { @Override protected Command newCommand() { @@ -20,4 +24,28 @@ public void testParse() throws Exception { String output = execute(countOption, Integer.toString(count), "0 0 0 1-6 * ?"); assertTrue(output, output.contains("Here are the next " + count + " times this cron expression will trigger")); } + + public void testGetNextValidTimes() throws Exception { + { + String output = execute( + "0 3 23 8 9 ? " + (Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT).get(Calendar.YEAR) + 1)); + assertTrue(output, output.contains("There are 1 times this cron expression will trigger:")); + assertFalse(output.contains("ERROR")); + assertFalse(output.contains("2.\t")); + } + { + String output = execute( + "0 3 23 */4 9 ? " + (Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT).get(Calendar.YEAR) + 1)); + assertTrue(output, output.contains("There are 8 times this cron expression will trigger:")); + assertFalse(output, output.contains("Here are the next 10 times this cron expression will trigger:")); + assertFalse(output, output.contains("ERROR")); + } + { + Exception expectThrows = expectThrows(Exception.class, () -> execute( + "0 3 23 */4 9 ? " + (Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT).get(Calendar.YEAR) - 1))); + String message = expectThrows.getMessage(); + assertTrue(message, message.contains("Could not compute future times since")); + assertTrue(message, message.contains("(perhaps the cron expression only points to times in the past?)")); + } + } } From a7845101dd40a648b0bac8d1d0255cd269c263b7 Mon Sep 17 00:00:00 2001 From: lipsill Date: Wed, 15 Aug 2018 13:41:03 +0200 Subject: [PATCH 2/5] addressing reviewers comments --- .../trigger/schedule/tool/CronEvalTool.java | 9 ++---- .../schedule/tool/CronEvalToolTests.java | 30 ++++++++++--------- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java index c996f3758072b..241f429d33df6 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java @@ -58,13 +58,12 @@ void execute(Terminal terminal, String expression, int count) throws Exception { DateTime date = DateTime.now(DateTimeZone.UTC); terminal.println("Now is [" + formatter.print(date) + "]"); + terminal.println("Here are the next " + count + " times this cron expression will trigger:"); Cron cron = new Cron(expression); long time = date.getMillis(); - int i = 0; - List times = new ArrayList<>(); - for (; i < count; i++) { + for (int i = 0; i < count; i++) { long prevTime = time; time = cron.getNextValidTimeAfter(time); if (time < 0) { @@ -74,9 +73,7 @@ void execute(Terminal terminal, String expression, int count) throws Exception { } break; } - times.add((i + 1) + ".\t" + formatter.print(time)); + terminal.println((i + 1) + ".\t" + formatter.print(time)); } - terminal.println((i == count ? "Here are the next " : "There are ") + i + " times this cron expression will trigger:"); - times.forEach(terminal::println); } } diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalToolTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalToolTests.java index d336317cd10bf..b73f816bc6a65 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalToolTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalToolTests.java @@ -12,6 +12,9 @@ import java.util.Locale; import java.util.TimeZone; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; + public class CronEvalToolTests extends CommandTestCase { @Override protected Command newCommand() { @@ -27,25 +30,24 @@ public void testParse() throws Exception { public void testGetNextValidTimes() throws Exception { { - String output = execute( - "0 3 23 8 9 ? " + (Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT).get(Calendar.YEAR) + 1)); - assertTrue(output, output.contains("There are 1 times this cron expression will trigger:")); - assertFalse(output.contains("ERROR")); - assertFalse(output.contains("2.\t")); + final int nextYear = Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT).get(Calendar.YEAR) + 1; + String output = execute("0 3 23 8 9 ? " + nextYear); + assertThat(output, containsString("Here are the next 10 times this cron expression will trigger:")); + assertThat(output, not(containsString("ERROR"))); + assertThat(output, not(containsString("2.\t"))); } { - String output = execute( - "0 3 23 */4 9 ? " + (Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT).get(Calendar.YEAR) + 1)); - assertTrue(output, output.contains("There are 8 times this cron expression will trigger:")); - assertFalse(output, output.contains("Here are the next 10 times this cron expression will trigger:")); - assertFalse(output, output.contains("ERROR")); + final int nextYear = Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT).get(Calendar.YEAR) + 1; + String output = execute("0 3 23 */4 9 ? " + nextYear); + assertThat(output, containsString("Here are the next 10 times this cron expression will trigger:")); + assertThat(output, not(containsString("ERROR"))); } { - Exception expectThrows = expectThrows(Exception.class, () -> execute( - "0 3 23 */4 9 ? " + (Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT).get(Calendar.YEAR) - 1))); + final int previousYear = Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT).get(Calendar.YEAR) - 1; + Exception expectThrows = expectThrows(Exception.class, () -> execute("0 3 23 */4 9 ? " + previousYear)); String message = expectThrows.getMessage(); - assertTrue(message, message.contains("Could not compute future times since")); - assertTrue(message, message.contains("(perhaps the cron expression only points to times in the past?)")); + assertThat(message, containsString("Could not compute future times since")); + assertThat(message, containsString("(perhaps the cron expression only points to times in the past?)")); } } } From 40d86d864907bbbb1bf95e3e88cc186b517db0f2 Mon Sep 17 00:00:00 2001 From: lipsill Date: Wed, 15 Aug 2018 13:44:14 +0200 Subject: [PATCH 3/5] replace assertTrue by assertThat --- .../xpack/watcher/trigger/schedule/tool/CronEvalToolTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalToolTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalToolTests.java index b73f816bc6a65..7c95520351dda 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalToolTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalToolTests.java @@ -25,7 +25,7 @@ public void testParse() throws Exception { String countOption = randomBoolean() ? "-c" : "--count"; int count = randomIntBetween(1, 100); String output = execute(countOption, Integer.toString(count), "0 0 0 1-6 * ?"); - assertTrue(output, output.contains("Here are the next " + count + " times this cron expression will trigger")); + assertThat(output, containsString("Here are the next " + count + " times this cron expression will trigger")); } public void testGetNextValidTimes() throws Exception { From 161c0d4e161f51b6ec33285d52f3f72659615301 Mon Sep 17 00:00:00 2001 From: lipsill Date: Wed, 15 Aug 2018 13:53:16 +0200 Subject: [PATCH 4/5] remove leftover import --- .../watcher/trigger/schedule/tool/CronEvalTool.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java index 241f429d33df6..5d76ce7afd549 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java @@ -5,22 +5,22 @@ */ package org.elasticsearch.xpack.watcher.trigger.schedule.tool; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; - import joptsimple.OptionSet; import joptsimple.OptionSpec; + import org.elasticsearch.cli.ExitCodes; import org.elasticsearch.cli.LoggingAwareCommand; -import org.elasticsearch.cli.UserException; import org.elasticsearch.cli.Terminal; +import org.elasticsearch.cli.UserException; import org.elasticsearch.xpack.core.scheduler.Cron; import org.joda.time.DateTime; import org.joda.time.DateTimeZone; import org.joda.time.format.DateTimeFormat; import org.joda.time.format.DateTimeFormatter; +import java.util.Arrays; +import java.util.List; + public class CronEvalTool extends LoggingAwareCommand { public static void main(String[] args) throws Exception { From 51dcb90e85c5bd935a786c7bbb9b930b74777eaa Mon Sep 17 00:00:00 2001 From: lipsill Date: Thu, 16 Aug 2018 12:49:48 +0200 Subject: [PATCH 5/5] some minor cleanup --- .../watcher/trigger/schedule/tool/CronEvalTool.java | 9 ++++----- .../trigger/schedule/tool/CronEvalToolTests.java | 10 ++++------ 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java index 5d76ce7afd549..d22d402aa157e 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java @@ -5,22 +5,21 @@ */ package org.elasticsearch.xpack.watcher.trigger.schedule.tool; +import java.util.Arrays; +import java.util.List; + import joptsimple.OptionSet; import joptsimple.OptionSpec; - import org.elasticsearch.cli.ExitCodes; import org.elasticsearch.cli.LoggingAwareCommand; -import org.elasticsearch.cli.Terminal; import org.elasticsearch.cli.UserException; +import org.elasticsearch.cli.Terminal; import org.elasticsearch.xpack.core.scheduler.Cron; import org.joda.time.DateTime; import org.joda.time.DateTimeZone; import org.joda.time.format.DateTimeFormat; import org.joda.time.format.DateTimeFormatter; -import java.util.Arrays; -import java.util.List; - public class CronEvalTool extends LoggingAwareCommand { public static void main(String[] args) throws Exception { diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalToolTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalToolTests.java index 7c95520351dda..f1e864d547c83 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalToolTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalToolTests.java @@ -29,22 +29,20 @@ public void testParse() throws Exception { } public void testGetNextValidTimes() throws Exception { + final int year = Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT).get(Calendar.YEAR) + 1; { - final int nextYear = Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT).get(Calendar.YEAR) + 1; - String output = execute("0 3 23 8 9 ? " + nextYear); + String output = execute("0 3 23 8 9 ? " + year); assertThat(output, containsString("Here are the next 10 times this cron expression will trigger:")); assertThat(output, not(containsString("ERROR"))); assertThat(output, not(containsString("2.\t"))); } { - final int nextYear = Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT).get(Calendar.YEAR) + 1; - String output = execute("0 3 23 */4 9 ? " + nextYear); + String output = execute("0 3 23 */4 9 ? " + year); assertThat(output, containsString("Here are the next 10 times this cron expression will trigger:")); assertThat(output, not(containsString("ERROR"))); } { - final int previousYear = Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT).get(Calendar.YEAR) - 1; - Exception expectThrows = expectThrows(Exception.class, () -> execute("0 3 23 */4 9 ? " + previousYear)); + Exception expectThrows = expectThrows(Exception.class, () -> execute("0 3 23 */4 9 ? 2017")); String message = expectThrows.getMessage(); assertThat(message, containsString("Could not compute future times since")); assertThat(message, containsString("(perhaps the cron expression only points to times in the past?)"));