Skip to content

[Watcher] Improved error messages for CronEvalTool #32800

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Aug 23, 2018
Merged

[Watcher] Improved error messages for CronEvalTool #32800

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
11fab60
[Watcher] Improved error messages for CronEvalTool
lipsill Aug 12, 2018
a784510
addressing reviewers comments
lipsill Aug 15, 2018
40d86d8
replace assertTrue by assertThat
lipsill Aug 15, 2018
161c0d4
remove leftover import
lipsill Aug 15, 2018
51dcb90
some minor cleanup
lipsill Aug 16, 2018
5bbadcf
Merge remote-tracking branch 'origin/master' into CronEvalTool_errorMsg
lipsill Aug 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions ...her/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
package org.elasticsearch.xpack.watcher.trigger.schedule.tool;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

Expand Down Expand Up @@ -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<String> 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:");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this special handling here (introducing state that requires following by filling up the list). Just adding the additional if-statement plus break should be sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the change as originally the following results were printed by the cli tool:
0 0 0 1-6 * ? resulted in

Valid!
Now is [Wed, 15 Aug 2018 11:28:34]
Here are the next 10 times this cron expression will trigger:
1.	Sat, 1 Sep 2018 02:00:00
2.	Sun, 2 Sep 2018 02:00:00
3.	Mon, 3 Sep 2018 02:00:00
4.	Tue, 4 Sep 2018 02:00:00
5.	Wed, 5 Sep 2018 02:00:00
6.	Thu, 6 Sep 2018 02:00:00
7.	Mon, 1 Oct 2018 02:00:00
8.	Tue, 2 Oct 2018 02:00:00
9.	Wed, 3 Oct 2018 02:00:00
10.	Thu, 4 Oct 2018 02:00:00

And a one time cron, such as 0 3 23 8 9 ? 2019 resulted in

Valid!
Now is [Wed, 15 Aug 2018 11:29:37]
Here are the next 10 times this cron expression will trigger:
1.	Mon, 9 Sep 2019 01:03:00

I found it is a little bit confusing that in this case the message was Here are the next 10 time and only a single one was printed, hence I tried to clarify the message.

I will revert the change.

times.forEach(terminal::println);
}
}
28 changes: 28 additions & 0 deletions ...rc/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalToolTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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 {
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for readability, just put final int year = ZonedDateTime.now(ZoneOffset.UTC).getYear() + 1; at the top of the test and use that one maybe?

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:"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you replace assertTrue/assertFalse with

assertThat(output, containsString("FoObar"));
assertThat(output, not(containsString("ERROR")));

This way the assertion will return something helpful instead of just being true or false in order to ease debugging.

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?)"));
}
}
}