Repository navigation
London | 26-ITP-SEPT | Carol Nassuna | Sprint 1 | Exhaustively test and fix formatAs12HourClock - #1657
London | 26-ITP-SEPT | Carol Nassuna | Sprint 1 | Exhaustively test and fix formatAs12HourClock#1657Mugs3 wants to merge 2 commits into
Conversation
abdishakoor-dev
left a comment
There was a problem hiding this comment.
You kept both starter tests exactly as they were, and your midnight test is doing its job: it fails, because the function still has a bug there. That's exactly what a test is for.
How to run your tests. You'll need this for every step below:
- In VS Code, look at the file list on the left (the Explorer).
- Right-click the folder called
format-clock-edge-casesand choose Open in Integrated Terminal. A terminal panel opens at the bottom of the screen. - Click in that terminal panel, type
node --test timeConverter.test.jsand press Enter. - Each test is listed with a ✔ (passed) or a ✖ (failed). At the bottom you'll see
passandfailwith a number next to each.
At the moment you'll see pass 8 and fail 1. The ✖ is next to "can correctly convert midnight".
What to change before I can mark this Complete:
-
Fix midnight in
timeConverter.js, lines 5 and 6. My comment on line 5 explains how. -
Fix the typo in
timeConverter.test.jsline 30. My comment there explains it. -
Add three more tests at the bottom of
timeConverter.test.js, after line 39. Copy one of your existing tests and change the time and the expected answer:"00:30": half past midnight"11:59": the last minute before am turns into pm"13:00": one o'clock in the afternoon
Work out what each should give on a 12-hour clock first, and write that as the expected answer.
-
Delete lines 16 to 20 of
timeConverter.js(the fiveconsole.loglines).
When you've made the changes, run node --test timeConverter.test.js in the terminal again. Push only when it says fail 0. Then add the Needs Review label again. Once these four are done, I expect to mark this Complete.
|
|
||
| if (hours > 12) { | ||
| return `${hours - 12}:00 pm`; | ||
| if (time == "00") { |
There was a problem hiding this comment.
This is why the midnight test fails.
Line 5 checks whether time is "00". But for midnight, time is the whole text "00:00", so the check is never true, and the code carries on down to line 12, which returns "00:00 am".
Line 2 already gives you the hour on its own, as a number, in the variable hours. Change line 5 so it checks hours instead of time. Remember hours is a number, so compare it with a number, not with text in quotes.
Then look at line 6. It returns 12:00: followed by the minutes, which gives 12:00:00 am, one part too many. Line 10 is how you did it for noon. Midnight needs the same shape, but with am instead of pm.
Run node --test timeConverter.test.js in the terminal again. The midnight test should now show ✔.
| }); | ||
|
|
||
| test("can correctly convert time at the beginning of noon", function () { | ||
| assert.equal(formatAs12HourClock("12:001"), "12:01 pm"); |
There was a problem hiding this comment.
There's an extra 1 here: "12:001" should be "12:01". Delete the extra 1 so the test checks a real time. (It passes at the moment only by luck, because the function looks at the last two characters.)
| } | ||
|
|
||
| export {formatAs12HourClock}; | ||
| console.log(formatAs12HourClock("23:00")); |
There was a problem hiding this comment.
These five lines were useful while you were working it out, but they print every time the tests run: you can see 11:00 pm, 2:00 pm and so on above your test results. Delete lines 16 to 20, then run node --test timeConverter.test.js again to check nothing else changed.
|
In ITP, whenever your PR is ready to be re-reviewed, in addition to adding the "Needs review" label, you should also address the reviewer's comments. Please use AI to explore, "How to respond to reviewer's inline comments in a PR?" In addition, the Changelist content in the PR description does not quite tell what this PR is about. Please also use AI to research "What to include in the Changelist section of a PR description?" |
abdishakoor-dev
left a comment
There was a problem hiding this comment.
Hi Carol, all four changes from my last review are done: midnight works, the typo is fixed, the console.log lines are gone, and your three new tests pass. I also tried every hour boundary from 00:00 to 23:59 and each one gives the right answer. You also kept the zero in front of single-digit hours after noon (01:00 pm), which matches how your morning times look (08:00 am). Nice and consistent.
One small thing before I mark this Complete: Prettier again on timeConverter.test.js. Your last commit removed the empty line at the end of the file. Run Format Document, push, and add the Needs Review label again. If you turn on format on save, this won't slip through again: https://github.com/CodeYourFuture/Module-JavaScript-Fundamentals/blob/main/practical_guide.md
|
|
||
| test("can correctly convert 1 hour after midday", function () { | ||
| assert.equal(formatAs12HourClock("13:00"), "01:00 pm"); | ||
| }); No newline at end of file |
There was a problem hiding this comment.
Prettier wants an empty line after }); at the end of the file.
Self checklist
Task code
CYF-1197
Changelist
10/10 updated files
Updated the 2 files