fix unit test for "should recognize dates of the month in the past but next month" #1131

dpschen · 2021-12-02T14:52:50Z

dpschen commented

2021-12-02 14:52:50 +00:00

This fixes the test.
But I'm not sure what the test ist supposed to test?

This fixes the test. But I'm not sure what the test ist supposed to test?

dpschen added 1 commit 2021-12-02 14:52:51 +00:00

continuous-integration/drone/pr Build is passing Details

75b1448bd7

fix unit test for "should recognize dates of the month in the past but next month"

dpschen reviewed 2021-12-02 14:53:32 +00:00

src/helpers/time/parseDate.ts

						
				@ -310,3 +311,3 @@

					}

					if (date < new Date()) {

					if (date < now) {

dpschen commented

2021-12-02 14:53:32 +00:00

Using one date for whole function, since time can pass in between

👍 1

konrad marked this conversation as resolved

dpschen reviewed 2021-12-02 14:55:13 +00:00

src/modules/parseTaskText.test.js

						
				@ -210,2 +210,3 @@

							expect(result.date.getDate()).toBe(date.getDate())

							expect(result.date.getMonth()).toBe(result.date.getDate() === 31 ? date.getMonth() + 2 : date.getMonth() + 1)

							const nextMonthWithDate = result.date.getDate() === 31

dpschen commented

2021-12-02 14:55:13 +00:00

The modulo ensures that we always have a month value between 0 and 11.
Not sure if that's the best method or if there are some quirks that would make it better to use date-fns addMonths method instead.

The modulo ensures that we always have a month value between 0 and 11. Not sure if that's the best method or if there are some quirks that would make it better to use [date-fns addMonths method](https://date-fns.org/v2.27.0/docs/addMonths) instead.

konrad commented

2021-12-02 15:38:17 +00:00

As long as the test passes I'd say it should be fine. We'll see :)

dpschen reviewed 2021-12-02 15:06:30 +00:00

src/modules/parseTaskText.test.js

				`@ -208,7 +208,11 @@ describe('Parse Task Text', () => {`

dpschen commented

2021-12-02 15:06:30 +00:00

Regarding the line:

date.setDate(date.getDate() - 1)

(not sure why I'm only able to comment a few lines above my change, but not there anymore)

For me it's not clear what that has to do with the stated goal of the test 'should recognize dates of the month in the past but next month'.

The resulting numer that is used via date.getDate() seems to be the index of the day of "yesterday" (since it's started from 0).

Why not just start from today?
I the test ran yesterday it should (in the best case) also run today?

Regarding the line: ```js date.setDate(date.getDate() - 1) ``` (not sure why I'm only able to comment a few lines above my change, but not there anymore) For me it's not clear what that has to do with the stated goal of the test 'should recognize dates of the month in the past but next month'. The resulting numer that is used via `date.getDate()` seems to be the index of the day of "yesterday" (since it's started from 0). Why not just start from today? I the test ran yesterday it should (in the best case) also run today?

konrad commented

2021-12-02 15:37:32 +00:00

The test checks that dates (only the day) that would be in the past (before today) automatically get moved to the next month. Eg if we have the 2nd today and I create a task "do stuff on the 1st" I want the task to be created with a due date of the first but of next month, not this month.

The need for it came from my first naive Implementation where I would create a new date object (which uses today by default) and then only set the day to the one the parser found.

The wording could probably be improved on this one.

The test checks that dates (only the day) that would be in the past (before today) automatically get moved to the next month. Eg if we have the 2nd today and I create a task "do stuff on the 1st" I want the task to be created with a due date of the first but of next month, not this month. The need for it came from my first naive Implementation where I would create a new date object (which uses today by default) and then only set the day to the one the parser found. The wording could probably be improved on this one.

👍 1

frederick commented

2021-12-02 15:09:14 +00:00

Hi dpschen!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://1131-featurefix-parsedate-unit-test-for-december--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi dpschen! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://1131-featurefix-parsedate-unit-test-for-december--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.

dpschen requested review from konrad 2021-12-02 15:16:03 +00:00

konrad was assigned by dpschen

2021-12-02 15:16:16 +00:00

dpschen reviewed 2021-12-02 15:21:32 +00:00

src/helpers/time/parseDate.ts

						
				@ -299,2 +299,3 @@

					const date = new Date()

					const now = new Date()

					const date = new Date(now)

dpschen commented

2021-12-02 15:21:32 +00:00

Maybe better to use new Date(+now)? Not sure – I didn't do a lot of date related stuff.

Maybe better to use `new Date(+now)`? Not sure – I didn't do a lot of date related stuff.

konrad commented

2021-12-02 15:30:46 +00:00

I think both are fine.

👍 1

konrad marked this conversation as resolved

konrad approved these changes 2021-12-02 15:38:24 +00:00

konrad merged commit 20f0496fa5 into main

2021-12-02 15:39:47 +00:00

konrad deleted branch feature/fix-parsedate-unit-test-for-december

2021-12-02 15:39:47 +00:00

konrad referenced this issue from a commit

2021-12-02 15:39:48 +00:00

fix: unit test for "should recognize dates of the month in the past but next month" (#1131)

This repo is archived. You cannot comment on pull requests.

No reviewers

No Label

No Milestone

No project

No Assignees

3 Participants

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#1131