London | 26-ITP-May | Chinwe Chukwuma | Sprint 2 | Module-Data-Groups - #1353
London | 26-ITP-May | Chinwe Chukwuma | Sprint 2 | Module-Data-Groups#1353ChinweP wants to merge 15 commits into
Conversation
ChinweP
added
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
removed
the
Needs Review
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
ChinweP
changed the title
This comment has been minimized.
This comment has been minimized.
ChinweP
added
the
Needs Review
|
There is another Sprint-2 folder inside the Sprint-2 folder. I am not sure which set of files are the ones to be reviewed. Could you update this PR branch accordingly? |
cjyuan
added
Reviewed
ChinweP
added
the
Needs Review
|
@cjyuan, apologies for the inconvenience. I've removed the unnecessary Sprint-2. |
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
You missed updating a few files in the implement sub-folder.
| // Given an object with properties | ||
| // When passed to contains with an existing property name | ||
| // Then it should return true | ||
| test("returns true when object contains the property", () => { | ||
| const obj = { a: 1, b: 2 }; | ||
| expect(contains(obj, "a")).toBe(true); | ||
| }); | ||
|
|
||
| // Given an object with properties | ||
| // When passed to contains with a non-existent property name | ||
| // Then it should return false | ||
| test("returns false when object does not contain the property", () => { | ||
| const obj = { a: 1, b: 2 }; | ||
| expect(contains(obj, "c")).toBe(false); | ||
| }); |
There was a problem hiding this comment.
How are these two tests different from the tests on lines 19-28?
| // When passed to contains | ||
| // Then it should return false or throw an error | ||
| test("returns false for invalid parameters like an array", () => { | ||
| expect(contains([], "a")).toBe(false); |
There was a problem hiding this comment.
Arrays are objects, with their indices acting as keys. A proper test should use a non-empty array along with a valid key to ensure the function returns false specifically because the input is an array, not because the key is missing.
There was a problem hiding this comment.
I have removed the duplicate tests and changed the array test to use a non-empty array with a valid key.
cjyuan
removed
the
Needs Review
ChinweP
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Changes look good.
Could you also update the remaining files in the implement subfolder?
| expect(contains(arr, "0")).toBe(false); | ||
| expect(contains(null, "a")).toBe(false); | ||
| expect(contains(123, "a")).toBe(false); | ||
| expect(contains("hello", "a")).toBe(false); |
There was a problem hiding this comment.
Note: Object.hasOwn("ABC", "0") is true (similar to array, indices are also considered properties of a string).
cjyuan
removed
the
Needs Review
|
I've now updated the invalid parameter test. Thanks for pointing this out. |
ChinweP
added
📅 Sprint 2
2 participants
Learners, PR Template
Self checklist
Changelist
Sprint 2 completed