Repository: obsidian-tasks-group/obsidian-tasks
Pull request #2473 (2023-12-03 08:46:41)

test: test postponed task and date creation
# Description

- Add tests to `Postponer.createPostponedTask()`
- Add tests to `TasksDate.postpone()`

## Motivation and Context

Expand test coverage for safe change of behaviour in the future.

## How has this been tested?

- New unit tests added

## Types of changes

Changes visible to users:

- [x] **Tests** (prefix: `test` - additions and improvements to unit tests and the smoke tests)

## Checklist

- [x] My code follows the code style of this project and passes `yarn run lint`.
- [x] My change has adequate [Unit Test coverage](https://publish.obsidian.md/tasks-contributing/Testing/About+Testing).

## Terms

- [x] My contribution follow this project's [contributing guide](https://publish.obsidian.md/tasks-contributing)
- [x] I agree to follow this project's [Code of Conduct](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CODE_OF_CONDUCT.md)


ilandikov: I have a feeling that `TasksDate.postpone()` is not needed and should be inlined into `Postponer.createPostponedTask()` since:
- in order to use the `postpone()` method, a `Moment` is converted to `TasksDate`
- and later `postpone()` returns a `Moment` (which means we can use just moment and that's it, Bob is your uncle)
-`postpone()` is used only there
- tests over `TasksDate.postpone()` and `Postponer.createPostponedTask()` are quite repetitive

I'm not sure whether this shall be included in this PR, so I just put the tests 
claremacrae: > I have a feeling that `TasksDate.postpone()` is not needed and should be inlined into `Postponer.createPostponedTask()` since:
> 
> * in order to use the `TasksDate.postpone()` method, a `Moment` is converted to `TasksDate`
> * and later `TasksDate.postpone()` returns a `Moment` (which means we can use just moment and that's it, Bob is your uncle)
> * `TasksDate.postpone()` is used only in `Postponer.createPostponedTask()`
> * tests over `TasksDate.postpone()` and `Postponer.createPostponedTask()` are quite repetitive
> 
> I'm not sure whether this shall be included in this PR, so I just put the tests everywhere =)

Interesting observation.

### This PR

I feel that this is the kind of quandary that comes more often when combining adding of tests and changing the behaviour of the code in one step - and then one PR.

If this PR had been preceded by 'add missing tests for ....', there wouldn't be a concern about whether to remove code in the changing-behaviour PR...

I had a similar quandary when first looking at this PR - because there are some obvious missing tests, and I found myself debating whether to ask for more tests now, that were unrelated to the change in the PR.

The code review would have been easier if two separate PRs, and we would have had better tests, as we could have focussed on the behaviours to test.

### When to add tests

This is probably an example of "make the change easy, then make the easy change".

Having thorough test coverage is part of a change being easy.

(This is why I had us extract helper functions and start writing tests as our first work on the postponement code: #2453 - and personally my next PR would have been to add tests for the other extracted function - `createPostponedTask()` - before making any changes to the behaviour...)

ilandikov: > About my comments about missing tests... I don't mind merging this PR as is, to keep it focussed on the behavioural change.
> 
> And then sorting out test coverage in a separate PR, or when we pair next.

I have tried improving this PR as far as I could. I see some missing points thanks to your comments, however I don't have an idea how to improve them. I would be happy to either approach them with together during our session or solve them myself if you give me some clues =)
claremacrae: I'm not proud of myself - feeling rather guilty to do this on my own instead of pairing - but I'm really desperate to get your help to fix the actual postponing behaviour tomorrow (#2471), so I have gone ahead and added a few tests and TODOs...
claremacrae: Ooops...

I added a couple of quick tests of the behaviour of postponing by more than 1 day, and discovered that this change "feels" like it may have broken the behaviour of postponing by more than 1 day, for overdue tasks:

- 67764e55ba9112580a49c656585989e5c7d62ea7

```js
    it('should postpone yesterday date to today', () => {
        checkDatePostponesTo('2023-11-27', 1, 'day', '2023-11-28');

        // TODO: Review this behaviour.
        //       It is not clear that this is the best result - it is just the current result, since #2473.
        checkDatePostponesTo('2023-11-27', 1, 'week', '2023-11-28');
        checkDatePostponesTo('2023-11-27', 1, 'month', '2023-11-28');
    });
```

I won't now merge until we have had a chance to chat about it.

I have a possible fix/update to show you... Am unsure if it is right, but I think it may be a bit of an improvement.

Patch:
diff --git a/src/Scripting/Postponer.ts b/src/Scripting/Postponer.ts
index cf44adb4..a0c95417 100644
--- a/src/Scripting/Postponer.ts
+++ b/src/Scripting/Postponer.ts
@@ -3,7 +3,11 @@ import { Task } from '../Task';
 import { TasksDate } from './TasksDate';
 
 export function shouldShowPostponeButton(task: Task) {
-    return !task.isDone;
+    const hasAValidHappensDate = task.happensDates.some((date) => {
+        return !!date?.isValid();
+    });
+
+    return !task.isDone && hasAValidHappensDate;
 }
 
 export type HappensDate = keyof Pick<Task, 'startDate' | 'scheduledDate' | 'dueDate'>;


Test patch:
diff --git a/tests/Scripting/Postponer.test.ts b/tests/Scripting/Postponer.test.ts
index ab5a1c11..3735604e 100644
--- a/tests/Scripting/Postponer.test.ts
+++ b/tests/Scripting/Postponer.test.ts
@@ -2,15 +2,17 @@
  * @jest-environment jsdom
  */
 import moment from 'moment';
+import type { Task } from 'Task';
 import {
     type HappensDate,
+    createPostponedTask,
     getDateFieldToPostpone,
     postponementSuccessMessage,
     shouldShowPostponeButton,
 } from '../../src/Scripting/Postponer';
-import { TaskBuilder } from '../TestingTools/TaskBuilder';
-import { StatusConfiguration, StatusType } from '../../src/StatusConfiguration';
 import { Status } from '../../src/Status';
+import { StatusConfiguration, StatusType } from '../../src/StatusConfiguration';
+import { TaskBuilder } from '../TestingTools/TaskBuilder';
 
 window.moment = moment;
 
@@ -67,13 +69,15 @@ describe('postpone - date field choice', () => {
         const taskBuilder = new TaskBuilder().scheduledDate(date).startDate(date);
         checkPostponeField(taskBuilder, 'scheduledDate');
     });
+
+    // TODO Check it refuses to postpone an invalid date (failing test)
 });
 
 describe('postpone - whether to show button', () => {
     it('should account for status type', () => {
         function checkPostponeButtonVisibility(statusType: StatusType, expected: boolean) {
             const status = new Status(new StatusConfiguration('p', 'Test', 'q', true, statusType));
-            const task = new TaskBuilder().status(status).build();
+            const task = new TaskBuilder().dueDate('2023-10-30').status(status).build();
             expect(shouldShowPostponeButton(task)).toEqual(expected);
         }
 
@@ -87,22 +91,22 @@ describe('postpone - whether to show button', () => {
         checkPostponeButtonVisibility(StatusType.DONE, false);
     });
 
-    it('should show button for a task with no dates', () => {
+    it('should not show button for a task with no dates', () => {
         const task = new TaskBuilder().build();
 
-        expect(shouldShowPostponeButton(task)).toEqual(true);
+        expect(shouldShowPostponeButton(task)).toEqual(false);
     });
 
-    it('should show button for a task with a created date only', () => {
+    it('should not show button for a task with a created date only', () => {
         const task = new TaskBuilder().createdDate('2023-11-29').build();
 
-        expect(shouldShowPostponeButton(task)).toEqual(true);
+        expect(shouldShowPostponeButton(task)).toEqual(false);
     });
 
-    it('should show button for a task with a done date only', () => {
+    it('should not show button for a task with a done date only', () => {
         const task = new TaskBuilder().doneDate('2023-11-30').build();
 
-        expect(shouldShowPostponeButton(task)).toEqual(true);
+        expect(shouldShowPostponeButton(task)).toEqual(false);
     });
 
     it('should show button for a task with a start date only', () => {
@@ -111,10 +115,10 @@ describe('postpone - whether to show button', () => {
         expect(shouldShowPostponeButton(task)).toEqual(true);
     });
 
-    it('should show button for a task with an invalid start date', () => {
+    it('should not show button for a task with an invalid start date', () => {
         const task = new TaskBuilder().startDate('2023-13-01').build();
 
-        expect(shouldShowPostponeButton(task)).toEqual(true);
+        expect(shouldShowPostponeButton(task)).toEqual(false);
     });
 
     it('should show button for a task with a scheduled date only', () => {
@@ -123,10 +127,10 @@ describe('postpone - whether to show button', () => {
         expect(shouldShowPostponeButton(task)).toEqual(true);
     });
 
-    it('should show button for a task with an invalid scheduled date', () => {
+    it('should not show button for a task with an invalid scheduled date', () => {
         const task = new TaskBuilder().scheduledDate('2023-12-36').build();
 
-        expect(shouldShowPostponeButton(task)).toEqual(true);
+        expect(shouldShowPostponeButton(task)).toEqual(false);
     });
 
     it('should show button for a task with a due date only', () => {
@@ -135,10 +139,55 @@ describe('postpone - whether to show button', () => {
         expect(shouldShowPostponeButton(task)).toEqual(true);
     });
 
-    it('should show button for a task with an invalid due date', () => {
+    it('should not show button for a task with an invalid due date', () => {
         const task = new TaskBuilder().dueDate('20233-12-03').build();
 
-        expect(shouldShowPostponeButton(task)).toEqual(true);
+        expect(shouldShowPostponeButton(task)).toEqual(false);
+    });
+});
+
+describe('postpone - new task creation', () => {
+    beforeEach(() => {
+        jest.useFakeTimers();
+        jest.setSystemTime(new Date('2023-12-03'));
+    });
+
+    afterEach(() => {
+        jest.useRealTimers();
+    });
+
+    function testPostponedTaskAndDate(task: Task, expectedDateField: HappensDate, expectedPostponedDate: string) {
+        const { postponedDate, newTasks } = createPostponedTask(task, expectedDateField, 'day', 1);
+        expect(postponedDate.format('YYYY-MM-DD')).toEqual(expectedPostponedDate);
+        expect(newTasks[expectedDateField]?.format('YYYY-MM-DD')).toEqual(expectedPostponedDate);
+
+        // If the scheduled date was inferred from the filename, and it is the scheduledDate that was postponed,
+        // we must ensure that the 'inferred' flag has been reset to false.
+        // Otherwise, the new scheduled date will be ignored in some locations, like rendering of dates.
+        if (task.scheduledDateIsInferred && expectedDateField === 'scheduledDate') {
+            expect(newTasks.scheduledDateIsInferred).toEqual(false);
+        }
+    }
+
+    it('should postpone an overdue task to today', () => {
+        const task = new TaskBuilder().dueDate('2023-11-01').build();
+        const expectedPostponedDate = '2023-12-04';
+        testPostponedTaskAndDate(task, 'dueDate', expectedPostponedDate);
+    });
+
+    it('should postpone a task scheduled today to tomorrow', () => {
+        const task = new TaskBuilder().scheduledDate('2023-12-03').build();
+        testPostponedTaskAndDate(task, 'scheduledDate', '2023-12-04');
+    });
+
+    it.failing('should postpone a task scheduled today to tomorrow, when the scheduled date is inferred', () => {
+        const task = new TaskBuilder().scheduledDate('2023-12-03').scheduledDateIsInferred(true).build();
+        testPostponedTaskAndDate(task, 'scheduledDate', '2023-12-04');
+    });
+
+    it('should postpone a task that starts in the future to the next day', () => {
+        const task = new TaskBuilder().startDate('2024-03-05').build();
+        testPostponedTaskAndDate(task, 'startDate', '2024-03-06');
     });
 });
 
diff --git a/tests/Scripting/TasksDate.test.ts b/tests/Scripting/TasksDate.test.ts
index 0758d22f..00398f79 100644
--- a/tests/Scripting/TasksDate.test.ts
+++ b/tests/Scripting/TasksDate.test.ts
@@ -4,6 +4,7 @@
 
 import moment from 'moment';
 
+import type { unitOfTime } from 'moment/moment';
 import { TasksDate } from '../../src/Scripting/TasksDate';
 
 window.moment = moment;
@@ -105,3 +106,161 @@ describe('TasksDate', () => {
         expect(new TasksDate(moment('2023-02-31')).fromNow.groupText).toEqual('%%320230611%% Invalid date');
     });
 });
+
+describe('TasksDate - postpone', () => {
+    beforeEach(() => {
+        jest.useFakeTimers();
+        jest.setSystemTime(new Date('2023-11-28'));
+    });
+
+    afterEach(() => {
+        jest.useRealTimers();
+    });
+
+    function checkDatePostponesTo(
+        initialDate: string,
+        amount: number,
+        unitOfTime: unitOfTime.DurationConstructor,
+        expectedDate: string,
+    ) {
+        const tasksDate = new TasksDate(moment(initialDate));
+        const postponedDate = new TasksDate(tasksDate.postpone(unitOfTime, amount));
+        expect(postponedDate.formatAsDate()).toEqual(expectedDate);
+    }
+
+    it.failing('should not modify the original date when postponing', () => {
+        function checkPostponingDoesNotModifyOriginalDate(
+            initialDate: string,
+            amount: number,
+            unitOfTime: moment.unitOfTime.DurationConstructor,
+        ) {
+            // Arrange
+            const initialTasksDate = new TasksDate(moment(initialDate));
+
+            // Act
+            const postponed = new TasksDate(initialTasksDate.postpone(unitOfTime, amount));
+
+            // Assert
+            // We don't care what the new date is; we just need to know that the date has been modified
+            // or the Assert below would pass spuriously.
+            expect(postponed.formatAsDate()).not.toEqual(initialDate);
+
+            expect(initialTasksDate.formatAsDate()).toEqual(initialDate);
+        }
+
+        checkPostponingDoesNotModifyOriginalDate('2023-11-27', 1, 'day'); // before today
+        checkPostponingDoesNotModifyOriginalDate('2023-11-28', 1, 'day'); // today
+        checkPostponingDoesNotModifyOriginalDate('2023-11-30', 1, 'day'); // a future date
+    });
+
+    it('should postpone an older date (before yesterday) to tomorrow', () => {
+        checkDatePostponesTo('2023-11-20', 1, 'day', '2023-11-29');
+    });
+
+    it('should postpone yesterday date to tomorrow', () => {
+        checkDatePostponesTo('2023-11-27', 1, 'day', '2023-11-29');
+
+        // TODO: Review this behaviour.
+        //       It is not clear that this is the best result - it is just the current result, since #2473.
+        checkDatePostponesTo('2023-11-27', 1, 'week', '2023-12-05');
+        checkDatePostponesTo('2023-11-27', 1, 'month', '2023-12-28');
+    });
+
+    it('should postpone today date to tomorrow', () => {
+        checkDatePostponesTo('2023-11-28', 1, 'day', '2023-11-29');
+    });
+
+    it('should postpone tomorrow date to after tomorrow', () => {
+        checkDatePostponesTo('2023-11-29', 1, 'day', '2023-11-30');
+    });
+
+    it('should postpone a future date (after tomorrow)', () => {
+        checkDatePostponesTo('2024-03-20', 1, 'day', '2024-03-21');
+    });
+
+    it('should postpone by a month, to a shorter month (in a leap year)', () => {
+        checkDatePostponesTo('2024-01-31', 1, 'month', '2024-02-29');
+    });
+
+    // TODO Add more tests for increments other than 1 day
+
+    describe('visualise postpone behaviour', () => {
+        function postponeMultipleDatesBy(amount: number, unitOfTime: unitOfTime.DurationConstructor) {
+            // Set a date that is easy to decrement and increment
+            const today = '2023-11-10';
+            jest.setSystemTime(new Date(today));
+
+            const dates = [
+                '2023-11-01',
+                '2023-11-02',
+                '2023-11-03',
+                '2023-11-04',
+                '2023-11-05',
+                '2023-11-06',
+                '2023-11-07',
+                '2023-11-08',
+                '2023-11-09',
+                '2023-11-10',
+                '2023-11-11',
+                '2023-11-12',
+                '2023-11-13',
+            ];
+            let output = `[initial]     => [postponed on '${today}' by '${amount} ${unitOfTime}']
+`;
+            dates.forEach((date) => {
+                const tasksDate = new TasksDate(moment(date));
+                const format = 'YYYY-MM-DD ddd';
+                // We have to save the date before doing the incrementing,
+                // as this test revealed that currently TasksDate.postpone() mutates the original date,
+                // as well as returning the postponed date!!!
+                const dateFormatted = tasksDate.format(format);
+
+                const postponedDate = new TasksDate(tasksDate.postpone(unitOfTime, amount));
+                output += `${dateFormatted} => ${postponedDate.format(format)}\n`;
+            });
+            return output;
+        }
+
+        it('by 1 day', () => {
+            const postponedDatesDescription = postponeMultipleDatesBy(1, 'day');
+            expect(postponedDatesDescription).toMatchInlineSnapshot(`
+                "[initial]     => [postponed on '2023-11-10' by '1 day']
+                2023-11-01 Wed => 2023-11-11 Sat
+                2023-11-02 Thu => 2023-11-11 Sat
+                2023-11-03 Fri => 2023-11-11 Sat
+                2023-11-04 Sat => 2023-11-11 Sat
+                2023-11-05 Sun => 2023-11-11 Sat
+                2023-11-06 Mon => 2023-11-11 Sat
+                2023-11-07 Tue => 2023-11-11 Sat
+                2023-11-08 Wed => 2023-11-11 Sat
+                2023-11-09 Thu => 2023-11-11 Sat
+                2023-11-10 Fri => 2023-11-11 Sat
+                2023-11-11 Sat => 2023-11-12 Sun
+                2023-11-12 Sun => 2023-11-13 Mon
+                2023-11-13 Mon => 2023-11-14 Tue
+                "
+            `);
+        });
+
+        it('by 1 week', () => {
+            const postponedDatesDescription = postponeMultipleDatesBy(1, 'week');
+            expect(postponedDatesDescription).toMatchInlineSnapshot(`
+                "[initial]     => [postponed on '2023-11-10' by '1 week']
+                2023-11-01 Wed => 2023-11-17 Fri
+                2023-11-02 Thu => 2023-11-17 Fri
+                2023-11-03 Fri => 2023-11-17 Fri
+                2023-11-04 Sat => 2023-11-17 Fri
+                2023-11-05 Sun => 2023-11-17 Fri
+                2023-11-06 Mon => 2023-11-17 Fri
+                2023-11-07 Tue => 2023-11-17 Fri
+                2023-11-08 Wed => 2023-11-17 Fri
+                2023-11-09 Thu => 2023-11-17 Fri
+                2023-11-10 Fri => 2023-11-17 Fri
+                2023-11-11 Sat => 2023-11-18 Sat
+                2023-11-12 Sun => 2023-11-19 Sun
+                2023-11-13 Mon => 2023-11-20 Mon
+                "
+            `);
+        });
+    });
+});