Conversation
russom-g
added
the
Needs Review
abdishakoor-dev
left a comment
abdishakoor-dev
left a comment
There was a problem hiding this comment.
Thanks Russom. Your edge cases are thorough: midnight, noon, the minute either side of each, and the padding for 13:00. Small commits with clear names too.
One thing before I can mark this Complete. See my comment on timeConverter.test.js line 46.
Add the Needs Review label again once you've pushed.
| }); | ||
|
|
||
| test("correctly convert time after 12:00", function () { | ||
| assert.equal(formatAs12HourClock("23:00"), "11:00 PM"); |
There was a problem hiding this comment.
The starter tests expected "11:00 pm" here and "08:00 am" on line 18. You changed them to PM and AM.
The given tests show the format the function must return. So change the code to match them. Do not change the tests to match the code.
Your own list on lines 56 to 65 uses am and pm too. Please use lowercase in the code and in all your tests.
There was a problem hiding this comment.
PM and AM changed to lower case. Description for 23:00 ->11:00 pm corrected.
Starter test put back where they were. The reason I swapped the the two starter test cases was to make the whole test order to appear in chronological order.
Thanks for the feedback.
There was a problem hiding this comment.
Fixed. Both starter tests expect "11:00 pm" and "08:00 am" again, and the code matches.
abdishakoor-dev
added
Reviewed
russom-g
added
Needs Review
abdishakoor-dev
left a comment
abdishakoor-dev
left a comment
There was a problem hiding this comment.
Thanks Russom. The code and all your tests use lowercase am and pm now, and all 12 tests pass.
Marking this Complete. Well done.
abdishakoor-dev
added
Complete
2 participants
Learners, PR Template
Self checklist
Task code
CYF-1197
Changelist
Edge test cases written and tested. On the main function bugs fixed. Additional conditions written for checking multiple cases