Added tests for weekly repeated tasks

[EngelPika32]

Description

This PR adds some tests for weekly repeated tasks. Some tests fail as they expect a fixed behavior already (PR with fixes is upcoming).

Tangent: Just noticed during PR creation that a small refactor is included here as well. Wasn't intended to be in this exact PR, but seems fine to me. Can remove/force-push it out as well, if needed.

Related Issues

As I said, I'm adding some tests to help resolve #1902

Todo:

• [ ] Check/Verify tests for Daily
• [ ] Tests for Monthly
• [ ] Tests for Yearly
• [ ] Run prettier / remove prettier-ignore comments
• [ ] Create PR with fixes

(new PR, for the todos, is upcoming)

[johannesjo]

Thank you very much for this @EngelPika32 and for all the other work you have done for this project :) It's really good to have some help!

Do you need some feedback on the WIP? How should we proceed?

[EngelPika32]

Do you need some feedback on the WIP? How should we proceed?

I'm open to feedback if you have some. Better have it now and apply it to the rest directly than waiting, no?

I'm currently working on the tests for monthly (and yearly). Though, as I don't use them, I'm slightly clueless about how to test edge cases properly with the current test setup. Will probably figure smth. out by the end of the week. (at least, I hope so. I'm open for input and test data!)

At the same time, I can already say, weekly is fixed in my local dev branch. Haven't created a PR for the fix yet, as the surrounding code is pretty much WIP – trying various ways to efficiently filter monthly & yearly.

Unfortunately, I can't apply the same fix I did for weekly to monthly & yearly due to a likely performance hit. Hence I'm open to letting someone else figure out an efficient way to fix monthly & yearly. I'd happily review the changes then.

PS: Feel free to merge this PR as "added tests for weekly repeated tasks". I originally wrote 'WIP' cuz I thought I'll have the other tests done by now and wanted to add them straight away. But, I dunno how long they'll take me.

[EngelPika32]

mmh… do you think we should release a fix for weekly repeated tasks without having the others fixed? It'd certainly improve the usability of SP directly (by re-creating pre v7.10 behavior). Could cherry-pick required changes and create a PR tomorrow.

[johannesjo]

mmh… do you think we should release a fix for weekly repeated tasks without having the others fixed? It'd certainly improve the usability of SP directly (by re-creating pre v7.10 behavior). Could cherry-pick required changes and create a PR tomorrow.

Merging this in smaller chunks sound like a good idea to me, since it makes reviewing much easier. I would however cleanup the code so that the outcommented tests are in again, if possible and that there are no logs left.

[johannesjo]

I provided a bit of input on the code. I hope it's helpful! Please let me know, if you have any questions!

[johannesjo]

Unfortunately, I can't apply the same fix I did for weekly to monthly & yearly due to a likely performance hit. Hence I'm open to letting someone else figure out an efficient way to fix monthly & yearly. I'd happily review the changes then.

May I ask how you're trying to solve this? Is the idea, to create the tasks if they haven't been created before even though it's not the exact date they have been planned for?

[EngelPika32]

I would however clean up the code so that the out commented tests are in again, if possible and that there are no logs left.

Out commented tests: See the above comments in the review. no logs left: Wdym? Calls to console.log within tests are fine (IMHO). They help resolve occurring issues on failing tests. (IDE can show logs per test) Actually, I wanted to ask if I can keep/commit console.debug logs in the task-repeat-cfg.reducer.ts? (relevant for upcoming PRs).

May I ask how you're trying to solve this? Is the idea, to create the tasks if they haven't been created before even though it's not the exact date they have been planned for?

Well, that's kinda the whole point of the referenced issue, isn't it? A bit more fitting: "Create tasks, if they should've been created in the past (and weren't created already)." Hence the lastTaskCreation is very relevant. This allows the required checks & prevents the creation of too many tasks as well.

And regarding my progress: I got around my issue, added a test for monthly, and fixed monthly. (Though, still have to test against edge cases like February.)

[johannesjo]

Finally I found at least a little bit of time to think about this. Sorry for the delay :)

Calls to console.log within tests are fine (IMHO). They help resolve occurring issues on failing tests.

While they help for that particular test if you would need to debug it, they add noise that makes it a little bit harder to debug any other possible issues with tests. As with regular code, I think it is a good rule of thumb to get rid of all logs, whenever possible, unless they're needed. I wouldn't want to argue that this is the best approach, but it's what I am trying to stick to in most of my software projects, at least for the development code :)

Given that the tests are currently failing, I wouldn't want to merge this back on its own until those are not failing anymore, since this effectively would block everything else. It probably also makes more sense to review the tests in the context of the code they will accompany (no complaints so far :)).

I also think we need to address the conceptual implications before merging code for this. I will continue the discussion in the original issue. Let's continue from there.

[github-actions[bot]]

This PR has not received any updates in 90 days. Please comment, if this still relevant!