Try to make the intention of Recurrence tests clearer

[claremacrae]

trafficstars

I wasn't sure whether to put this in a separate issue, or mention it in #608 ... It became long, so I've put it in a separate issue.

I keep coming back to the tests that I moved in #608 (and also before I moved them), in order to try and understand their intention... The code is really easy to understand - what I'm thinking about is why each set of values in a Case was added...

As an example, what is the difference between these 3 tests:

        {
            interval: 'every 3 months on the 3rd Thursday',
            due: '2021-04-15',
            nextDue: '2021-07-15',
        },
        {
            interval: 'every 3 months on the 3rd Thursday',
            due: '2021-08-19',
            nextDue: '2021-11-18',
        },
        {
            interval: 'every 3 months on the 3rd Thursday',
            due: '2021-09-16',
            nextDue: '2021-12-16',
        },

Perhaps they are testing some subtly different boundary cases? Perhaps some due dates are not on Thursdays? Which ones?

How about these:

        {
            interval: 'every month',
            due: '2021-10-15',
            nextDue: '2021-11-15',
        },
        {
            interval: 'every month',
            due: '2021-10-18',
            nextDue: '2021-11-18',
        },

And in the following? I'm guessing here that there are some boundary cases involved, but if one of these failed, I would hesitate to say why the current answers are the right answers. I'd probably have to get out a calendar and work out the day-of-the-week for each of dates...

        {
            interval: 'every month on the 2nd Wednesday when done',
            due: '2021-09-08',
            today: '2021-09-15',
            nextDue: '2021-10-13',
        },
        {
            interval: 'every month on the 2nd Wednesday when done',
            due: '2021-09-08',
            today: '2021-10-12',
            nextDue: '2021-10-13',
        },
        {
            interval: 'every month on the 2nd Wednesday when done',
            due: '2021-09-08',
            today: '2021-10-13',
            nextDue: '2021-11-10',
        },

As I said in my recent talk on testing, I really like that these tests are data-driven and so can be quite concise and it's easy to add new test cases.

But also, from experience tests are a lot easier to maintain if it is easy to understand what behaviour they are testing.

Suggestion

With the tests for the happens feature, I tried to address this in two ways:

1. Dividing comments that group the related sets of data together
2. A description field on each HapensCase, to (try to) document the situation being tested - and which Jest prints out when the case values are displayed in the console output... If there is a test failure, this makes it really easy to identify what the intended behaviour or scenario was.
3. Consider formatting the test output in a way that adds more info, programmatically, such as to indicate the days of the week for each of the date values. (I know that's cryptic. Sorry. It's a concept from Approval Tests that is best explained by demoing in code)

[schemar]

Thanks, @claremacrae. I wholeheartedly agree and cannot explain to you why it was done this way. I added the initial tests in bbb4df8ac8a973ce628148abefe799f22c1bc10b and then quite a few more were added in #502. I simply don't remember my initial reasoning, which fully proves your point!

[claremacrae]

Thanks, @claremacrae. I wholeheartedly agree and cannot explain to you why it was done this way. I added the initial tests in bbb4df8 and then quite a few more were added in #502. I simply don't remember my initial reasoning, which fully proves your point!

Hahah - I totally appreciate your frankness there!

I've started adding some descriptions for the existing tests, and in doing this, added some more tests and learned some scary things I did not know about how the recurrence works...

So sorry, another recurrence-related-issue incoming shortly... 🤣

[claremacrae]

Just so I don't forget about it, here is an early attempt at exploring the intention of some of the recurrence tests...

https://github.com/claremacrae/obsidian-tasks/commit/69ccde055bfb265cf519a9d925c2a669bd0f03f4

[claremacrae]

and then quite a few more were added in #502.

Ah - thanks for pointing that out. Looking at the change in #502, it seems to modify a number of the existing tests, but I can't tell if it's an artefact of the way diffs are presented in the GitHub web UI, or if it really did modify some existing tests, and if-so, why...

More archaeology coming up... 😄

[schemar]

I don't think it edited them. They are shown as "additions" above, but those were the original tests.

[mauleb]

Howdy @claremacrae, saw the mentions. I can confirm that my goal was to retain the original test suite that was there previously while also extending it to cover the already existing but untested recurrence for scheduled/start dates as well as the changes I introduced via what ultimately became "<> when done". I was also attempting to be pretty thorough, as the original implementation was a pretty major breaking change to the functionality as a whole. That became less of a concern throughout the lifetime of the PR though. But I wouldn't be terribly surprised if tests got "left behind" which have a significant amount of overlap, both new and existing.

Regarding test descriptions, I tend to do something like the following for other projects:

test.each([
  ['when pass when x is true', true],
  // etc
])('comparison resolves correctly (%s)', (_, x) => {
  // etc
});

which is pretty similar to the approach you are already considering. I don't recall why I didn't do something similar over here for these changes.

[claremacrae]

Thank you @mauleb!

Your sample code reminded me of some old advice to describe tests with phrases beginning ‘should…’ - such as ‘should advance multiple increments until finding a valid date’.