Conversation
khanhdo05
left a comment
khanhdo05
left a comment
There was a problem hiding this comment.
Please see each comments to learn and understand better testing.
Actions required
- Fix indentation
- Remove unnecessary tests like those that tested
minuteNumberToHour,minuteNumberToMinute,minuteNumberToHourAndMinute - Move tests for
dateToMinuteNumberandtimeStringToMinuteNumbertobackend/src/test/userPreferenceUtils.spec.ts, make sure to write correspondingdescribeblocks.
| }; | ||
| } | ||
|
|
||
|
|
| const result = UserPreferenceUtils.dateToMinuteNumber(new Date('2025-08-20T05:05:00')); | ||
| expect(result).toBe(305); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
These blocks of 109 to 120 are indented weird, are they part of describe ("test date to minutes") ?, if so please indent them correctly.
| * Tests converting a string to minutes : timeStringToMinuteNumber | ||
| */ | ||
| describe ("string to minute", () => { | ||
| // Tests for 0 edge case |
There was a problem hiding this comment.
weird identation, please fix
There was a problem hiding this comment.
Overall, this test file is unnecessary.
Naming notes
First of all, good naming habit for tests is that you test the whole file and you have that exact file you are testing's name then .spec. Example here is that you are testing things from UserPreferenceUtils, it should be UserPreferenceUtils.spec.ts for the test file name. If you look into the test/, you will see I already make this file and have 1 test in there where I test the function transform.
Why did I say this test file you created is semi-unnecessary
Function timeStringToMinuteNumber() uses all the above functions that you also tested - does it make sense to keep re-testing things?
A good rule about whether we should test things is that:
- Don't test something you didn't code, for example:
public static minuteNumberToHour(m: number): number {
return Math.floor(m / 60);
}
public static minuteNumberToMinute(m: number): number {
return m % 60;
}
These two functions are just math, it is not a logic you introduced, no need to test whether Javascript math module works or not.
What should have been tested instead
- Not have this file
- Write more test in
backend/src/test/userPreferenceUtils.spec.ts - If there's edge cases you can think of in these tests you write below, incorporate it so that your test of
transformortimeStringToMinuteNumbercovers it. - Generally, for readability, I am ok with not writing more
transformtest but moretimeStringToMinuteNumberbut we do also need moretransformtests.
2 participants
No description provided.