Add test for `Number.prototype.chr` goody #395

Gobleizer · 2024-08-26T23:48:17Z

Resolves #113.

elimanzo

I'm not too sure if I'm able to view this, since it's considered a Binary file?

miorel · 2024-08-27T06:45:01Z

I can see the file at https://github.com/code-chronicles-code/leetcode-curriculum/blob/2ad7586de4ae68d10661131c29882dea5d5cf920/workspaces/adventure-pack/goodies/typescript/Number.prototype.chr/index.test.ts but yeah, for some reason GitHub thinks it's a binary file. @Gobleizer I'm not sure which of the non-ASCII characters is the culprit... Emojis are usually fine.

Gobleizer · 2024-08-27T21:53:17Z

I believe it is the Null Character (https://unicode-explorer.com/c/0000) which is the result when you attempt to get a character from number 0.
I can see it won't appear in the file preview. But it was necessary to make the test pass, because a null character is not the same as an empty string.

miorel · 2024-08-28T00:48:36Z

Perhaps we should use escapes for the null character? So "\0", similar to "\n" for newlines?

Gobleizer · 2024-08-29T00:10:20Z

Awesome point. Fixed.

elimanzo

hey @Gobleizer Thanks for the PR and fixing the file stuff!

I have a couple of nits.

workspaces/adventure-pack/goodies/typescript/Number.prototype.chr/index.test.ts

Gobleizer · 2024-09-01T00:37:37Z

What do you all think if I added inner describe blocks?
I like having it.each, but I dislike not having clear titles as to what a group of tests is doing right at the top of that group in code.
The test print out is actually better now since I can print unique strings for each test, which is fun.

Gobleizer · 2024-09-01T01:58:32Z

Yeah I went ahead and nested the describe blocks.

elimanzo

Hey @Gobleizer. Nice job on working on this PR! I have just one comment, but I think overall is good.

elimanzo · 2024-09-03T03:21:40Z

workspaces/adventure-pack/goodies/typescript/Number.prototype.chr/index.test.ts

+  describe("convert ASCII codepoint to ASCII string", () => {
+    it.each([
+      { codepoint: 65, expected: "A" },
+      { codepoint: 10, expected: "\n" },
+      { codepoint: 61, expected: "=" },
+      { codepoint: 126, expected: "~" },
+      { codepoint: 32, expected: " " },
+      { codepoint: 48, expected: "0" },
+    ])(
+      "convert ASCII codepoint $codepoint to ASCII string $expected",
+      ({ codepoint, expected }) => {
+        expect(codepoint.chr()).toBe(expected);
+      },
+    );
+  });
+
+  describe("convert codepoint to emoji", () => {
+    it.each([
+      { codepoint: 129302, expected: "🤖" },
+      { codepoint: 129412, expected: "🦄" },
+    ])(
+      "convert codepoint $codepoint to emoji $expected",
+      ({ codepoint, expected }) => {
+        expect(codepoint.chr()).toBe(expected);
+      },
+    );
+  });
+
+  describe("convert hex codepoint to string", () => {
+    it.each([
+      { codepoint: 0x404, expected: "Є" },
+      { codepoint: 0x24, expected: "$" },
+      { codepoint: 0x1f303, expected: "🌃" },
+      { codepoint: 0x1f92a, expected: "🤪" },
+    ])(
+      "convert hex codepoint $codepoint to string $expected",
+      ({ codepoint, expected }) => {
+        expect(codepoint.chr()).toBe(expected);
+      },
+    );
+  });


I'm not sure if using the extra describe blocks is needed. I'd figured that using describe blocks is meant to group things up, but since there's only one case for each describe block, then i don't think it's necessary having it. I'd 2x check with @miorel on what he recommends here, if these extra blocks provides more clarity.

Resolved the other comments, but interested to here @miorel thoughts on this. I could go either way.

So I pulled the PR and ran it both with and without the inner describes. I don't have a strong preference for whether we keep them or not. I think the main use case for the descriptions is when some tests fails, we want to be clear on which test it was.

The grouping with the inner describes is nice when we're running a specific test file, though. If you'd like to keep them my main ask would be to make the strings less redundant with the it descriptions, and also pass a "noun phrase" to the describe.

workspaces/adventure-pack/goodies/typescript/Number.prototype.chr/index.test.ts

miorel

Thanks for working on this! I think it's pretty much ready to go.

miorel · 2024-09-08T00:49:38Z

workspaces/adventure-pack/goodies/typescript/Number.prototype.chr/index.test.ts

+    it.each([
+      { edgecase: -1 },
+      { edgecase: 11120651 },
+      { edgecase: Infinity },
+      { edgecase: -Infinity },
+      { edgecase: 1.5 },
+      { edgecase: 1e-2 },
+      { edgecase: 0xffffff },
+    ])(
+      "handles number $edgecase outside unicode range by throwing range error",
+      ({ edgecase }) => {
+        expect(() => edgecase.chr()).toThrow(RangeError);
+      },
+    );
+
+    it("handles incompatible types that resolve to NaN by throwing range error", () => {
+      expect(() => NaN.chr()).toThrow(RangeError);
+    });


I think NaN can be merged with the other invalid values.

Suggested change

it.each([

{ edgecase: -1 },

{ edgecase: 11120651 },

{ edgecase: Infinity },

{ edgecase: -Infinity },

{ edgecase: 1.5 },

{ edgecase: 1e-2 },

{ edgecase: 0xffffff },

])(

"handles number $edgecase outside unicode range by throwing range error",

({ edgecase }) => {

expect(() => edgecase.chr()).toThrow(RangeError);

},

);

it("handles incompatible types that resolve to NaN by throwing range error", () => {

expect(() => NaN.chr()).toThrow(RangeError);

});

it.each([

-1,

11120651,

Infinity,

-Infinity,

1.5,

1e-2,

0xffffff,

NaN,

])(

"throws RangeError for non-Unicode value %p",

(value) => {

expect(() => value.chr()).toThrow(RangeError);

},

);

miorel · 2024-09-08T00:53:48Z

workspaces/adventure-pack/goodies/typescript/Number.prototype.chr/index.test.ts

+    );
+  });
+
+  describe("converts hex codepoint to string", () => {


Hex only exists for the developer's benefit:

console.log(0x1f303); // prints 127747 console.log(0x1f303 === 127747); // prints true

So I'm not sure we gain from having a hex-specific section, it's just another way of writing valid integers.

Imo we can move the 0x24 into the ASCII codepoints test cases (and we'll just have one of the ASCII codepoints written as hex, while others aren't). Likewise we can move the emoji ones into the emoji test, and have some of those codepoints written as hex and some as decimal.

The Є test can either be removed entirely or have its own test (non-ASCII non-emoji?).

miorel · 2024-09-21T01:19:11Z

Hey @Gobleizer I took this PR as-is since I'm about to work on some changes that might otherwise cause you some merge conflicts. Feel free to do a follow-up with any additional changes from the discussion.

Thanks again for working on this!

Gobleizer added 2 commits August 26, 2024 14:47

test: initial test set up

bb70e28

test: provides tests for typescript Number chr method

2ad7586

Gobleizer requested a review from miorel as a code owner August 26, 2024 23:48

miorel requested review from elimanzo and simona1 August 27, 2024 00:03

elimanzo reviewed Aug 27, 2024

View reviewed changes

fix: replaces incompatible characters

a453ab1

elimanzo requested changes Aug 30, 2024

View reviewed changes

workspaces/adventure-pack/goodies/typescript/Number.prototype.chr/index.test.ts Outdated Show resolved Hide resolved

workspaces/adventure-pack/goodies/typescript/Number.prototype.chr/index.test.ts Outdated Show resolved Hide resolved

miorel reviewed Aug 30, 2024

View reviewed changes

miorel changed the title ~~test/number prototype chr~~ Add test for Number.prototype.chr goody Aug 31, 2024

Gobleizer added 2 commits August 31, 2024 16:05

fix: removes repetitive tests

86a35f3

fix: refactor tests for readability

354da9b

fix: break up tests for readability

3ffe581

Merge branch 'main' into test/number-prototype-chr

0a6de0c

miorel requested a review from elimanzo September 2, 2024 22:58

elimanzo reviewed Sep 3, 2024

View reviewed changes

miorel reviewed Sep 7, 2024

View reviewed changes

workspaces/adventure-pack/goodies/typescript/Number.prototype.chr/index.test.ts Outdated Show resolved Hide resolved

miorel reviewed Sep 7, 2024

View reviewed changes

workspaces/adventure-pack/goodies/typescript/Number.prototype.chr/index.test.ts Outdated Show resolved Hide resolved

Merge branch 'main' into test/number-prototype-chr

b51df98

miorel requested a review from elimanzo September 7, 2024 23:51

fix: removes unnecessary test

cbe71fb

miorel approved these changes Sep 8, 2024

View reviewed changes

miorel merged commit c6d943b into code-chronicles-code:main Sep 21, 2024
6 checks passed

Add test for Number.prototype.chr goody #395

Add test for Number.prototype.chr goody #395

Uh oh!

Conversation

Gobleizer commented Aug 26, 2024 • edited by miorel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elimanzo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miorel commented Aug 27, 2024

Uh oh!

Gobleizer commented Aug 27, 2024

Uh oh!

miorel commented Aug 28, 2024

Uh oh!

Gobleizer commented Aug 29, 2024

Uh oh!

elimanzo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Gobleizer commented Sep 1, 2024

Uh oh!

Gobleizer commented Sep 1, 2024

Uh oh!

elimanzo left a comment

Choose a reason for hiding this comment

Uh oh!

elimanzo Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

Gobleizer Sep 8, 2024

Choose a reason for hiding this comment

Uh oh!

miorel Sep 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

miorel left a comment

Choose a reason for hiding this comment

Uh oh!

miorel Sep 8, 2024

Choose a reason for hiding this comment

Uh oh!

miorel Sep 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miorel commented Sep 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add test for `Number.prototype.chr` goody #395

Add test for `Number.prototype.chr` goody #395

Gobleizer commented Aug 26, 2024 •

edited by miorel

Loading

elimanzo left a comment •

edited

Loading