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.

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

Jump to bottom

Add cron expression assertion support in assertj-core #3212

Open
neilwangweili opened this issue Oct 8, 2023 · 9 comments · May be fixed by #3220
Open

Add cron expression assertion support in assertj-core #3212

neilwangweili opened this issue Oct 8, 2023 · 9 comments · May be fixed by #3220
Assignees
@neilwangweili
Labels
type: new feature A new feature

Comments

@neilwangweili
Copy link

neilwangweili commented Oct 8, 2023 •
edited

Feature Summary

Origin Of The Problem

Assertion of cron expressions in a project is also very important, and a programmer writing a wrong cron expression in a project can even cause a disaster. According to the idea of TDD, before writing a correct cron expression, you should use tests to express the expectation that the cron expression wants to achieve. But I haven't found a particularly convenient way to do this, and this is my current solution, which makes use of org.springframework.scheduling.support.CronExpression:

My Interim Solution

@Component
public class Timer {

    protected static final String PER_5_MIN = "0 0/5 * * * *";

    @Scheduled(cron = PER_5_MIN, zone = "GMT+08")
    public void execute() {
        // do something...
    }
}
class GitLabIssuesSynchroniserTest {

    @Test
    void should_synchroniser_executed_per_5_min() throws NoSuchFieldException, IllegalAccessException {
        assertTrue(CronExpression.isValidExpression(GitLabIssuesSynchroniser.PER_5_MIN));
        CronExpression parsed = CronExpression.parse(GitLabIssuesSynchroniser.PER_5_MIN);
        Object[] parsedResults = getParsedResults(parsed);
        assertEquals(parsedResults[0].toString(), "DayOfWeek {1, 2, 3, 4, 5, 6, 7}");
        assertEquals(parsedResults[1].toString(), "MonthOfYear {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12}");
        assertEquals(parsedResults[2].toString(), "DayOfMonth {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31}");
        assertEquals(parsedResults[3].toString(), "HourOfDay {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23}");
        assertEquals(parsedResults[4].toString(), "MinuteOfHour {0, 5, 10, 15, 20, 25, 30, 35, 40, 45, 50, 55}");
        assertEquals(parsedResults[5].toString(), "SecondOfMinute {0}");
        assertEquals(parsedResults[6].toString(), "NanoOfSecond {0}");
    }

    private static Object[] getParsedResults(CronExpression parsed) throws NoSuchFieldException, IllegalAccessException {
        Field fields = CronExpression.class.getDeclaredField("fields");
        ReflectionUtils.makeAccessible(fields);
        return (Object[]) fields.get(parsed);
    }

}

My Thoughts

I think this could also be wrapped by the assertj framework, it looks like it should be under the assertj-core package. In this way, timed tasks in software code can be used with greater confidence and testing to verify their functionality is simple.

Example

Perhaps the test code for a programmer using the assertj-core framework to solve cron expression validation is as follows:

    String cron = "0 0/5 * * * *";
    // This CronExpression is in assertj core package.
    CronExpression expression = asCronExpression(cron);
    assertThat(expression).isValid();
    assertThat(expression).isContainsAllDayOfWeek();
    assertThat(expression).isContainsAllMonthOfYear();
    assertThat(expression).isContainsAllDayOfMonth();
    assertThat(expression).isContainsAllHourOfDay();
    assertThat(expression).isContainsHourOfDay(0, 2, 3, 4, 5, 6, 10);
    assertThat(expression).isContainsMinuteOfHour(0, 5, 10, 15, 20, 25, 30, 35, 40, 45, 50, 55);

    // The next line will fail because it does not contain the first minute.
    assertThat(expression).isContainsMinuteOfHour(1);
    assertThat(expression).isContainsSecondOfMinute(0);

    // isMatch...() would require exactly the same
    assertThat(expression).isMatchMinuteOfHour(0, 5, 10, 15, 20, 25, 30, 35, 40, 45, 50, 55);

This is a prototype of what I envision the api to be, and it may need more refined thought.

A Few Doubts

I think it's a great feature to support, and it fills in the points of vulnerability in the testing framework. I am interested in this and would like to contribute to this project on this issue. Please let me know if this feature goes against the original intent of the project.
@joel-costigliola
Copy link
Member

joel-costigliola commented Oct 8, 2023 •
edited

@neilwangweili thanks for the idea, I agree having cron assertions would be a good addition.

AssertJ has a no-dependency policy thus we would need to define CronExpression or copy it from elsewhere.
To make it easy to use I think there should be an entry point taking a String, since assertThat(String) is already defined we would need to expose assertThatCronExpression(String) (and also assertThat(CronExpression).

assertThat(expression).isValid() is nice but I don't find assertThat(expression).isContainsAllDayOfWeek() to be that elegant.
This still need to be debated but here are some alternatives:

  1. containsAllDayOfWeek()
  2. matchesAllDayOfWeek()
  3. includesAllDayOfWeek()

@neilwangweili
Copy link
Author

@neilwangweili thanks for the idea, I agree having cron assertions would be a good addition.

AssertJ has a no-dependency policy thus we would need to define CronExpression and reuse from elsewhere. To make it easy to use I think there should be an entry point taking a String, since assertThat(String) is already defined we would need to expose assertThatCronExpression(String) (and also assertThat(CronExpression).

assertThat(expression).isValid() is nice but I don't find assertThat(expression).isContainsAllDayOfWeek() to be that elegant. This still need to be debated but here are some alternatives:

  1. containsAllDayOfWeek()
  2. matchesAllDayOfWeek()
  3. includesAllDayOfWeek()

@joel-costigliola WOW! Thank you for the affirmation. Can I write a pr to try it out?

@neilwangweili
Copy link
Author

Hi, there? @joel-costigliola

@scordio
Copy link
Member

scordio commented Oct 10, 2023

Go for it, @neilwangweili!

@scordio scordio added the type: new feature A new feature label Oct 10, 2023
@neilwangweili
Copy link
Author

AssertJ has a no-dependency policy thus we would need to define CronExpression and reuse from elsewhere.

You mean we write our own set of cron expression parsers?
Or only delegate into domain area like this?

<dependency>cron.parser</dependency>
<artifactId>xxx</artifactId>
<version>xxx</version>
public class CronExpression {
    private final cron.parser.CronExpression expression;

    // constructor


    public void isValid() {
        expression.isValid();
    }
}

@scordio
Copy link
Member

scordio commented Oct 12, 2023

No new dependency should be added so we have to introduce our own implementation.

@neilwangweili
Copy link
Author

That's cool

@joel-costigliola
Copy link
Member

We take inspiration from an open source implementation of cron expression but we would need to be careful with the licenses

@neilwangweili neilwangweili linked a pull request Oct 13, 2023 that will close this issue
Open
@neilwangweili
Copy link
Author

Some simple valuable api's have been done. #3220

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

@neilwangweili neilwangweili

Labels
type: new feature A new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: Add cron expression assertion, Related to #3212 neilwangweili/assertj
3 participants
@joel-costigliola @scordio @neilwangweili