-
Notifications
You must be signed in to change notification settings - Fork 2
fix: infinite loops, resource leaks, and memory management issues #1048
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
665131b
c7fdb33
b1ff22e
9430229
93459b1
dbf6698
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,13 @@ | ||
| [install] | ||
| exact = true | ||
| saveTextLockfile = true | ||
| saveTextLockfile = true | ||
|
|
||
| [test] | ||
| coverage.exclude = [ | ||
| "**/dist/**", | ||
| "**/node_modules/**", | ||
| "**/*.d.ts", | ||
| "**/test/**", | ||
| "**/*.test.ts", | ||
| "**/*.test.js" | ||
| ] |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,154 @@ | ||||||||||
| /* eslint-disable @typescript-eslint/no-explicit-any */ | ||||||||||
| import { describe, expect, mock, test } from "bun:test"; | ||||||||||
| import { Readable } from "node:stream"; | ||||||||||
|
|
||||||||||
| // Since the login command is complex with dependencies, we'll test the STDIN reading logic separately | ||||||||||
| // by extracting it into a testable function | ||||||||||
| describe("login STDIN reading", () => { | ||||||||||
| test("reads token from STDIN properly", async () => { | ||||||||||
| const testToken = "test-token-123"; | ||||||||||
|
|
||||||||||
| // Create a mock stdin stream | ||||||||||
| const mockStdin = new Readable({ | ||||||||||
| read() { | ||||||||||
| this.push(testToken); | ||||||||||
| this.push(null); // End the stream | ||||||||||
| }, | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| // Mock the STDIN reading logic | ||||||||||
| const readStdinToken = async (): Promise<string> => { | ||||||||||
| const chunks: Buffer[] = []; | ||||||||||
| let timeout: NodeJS.Timeout | undefined; | ||||||||||
|
|
||||||||||
| const timeoutPromise = new Promise<never>((_, reject) => { | ||||||||||
| timeout = setTimeout(() => { | ||||||||||
| reject(new Error("Timeout reading from STDIN after 30 seconds")); | ||||||||||
| }, 30_000); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| try { | ||||||||||
| const readPromise = (async () => { | ||||||||||
| for await (const chunk of mockStdin) { | ||||||||||
| chunks.push(Buffer.from(chunk)); | ||||||||||
| } | ||||||||||
| return Buffer.concat(chunks).toString().trim(); | ||||||||||
| })(); | ||||||||||
|
|
||||||||||
| const result = await Promise.race([readPromise, timeoutPromise]); | ||||||||||
| return result; | ||||||||||
| } finally { | ||||||||||
| if (timeout) { | ||||||||||
| clearTimeout(timeout); | ||||||||||
| } | ||||||||||
| } | ||||||||||
| }; | ||||||||||
|
|
||||||||||
| const result = await readStdinToken(); | ||||||||||
| expect(result).toBe(testToken); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| test("handles timeout correctly", async () => { | ||||||||||
| // Create a mock stdin that never ends | ||||||||||
| const mockStdin = new Readable({ | ||||||||||
| read() { | ||||||||||
| // Don't push null, keep stream open | ||||||||||
| }, | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| const readStdinTokenWithShortTimeout = async (): Promise<string> => { | ||||||||||
| const chunks: Buffer[] = []; | ||||||||||
| let timeout: NodeJS.Timeout | undefined; | ||||||||||
|
|
||||||||||
| const timeoutPromise = new Promise<never>((_, reject) => { | ||||||||||
| timeout = setTimeout(() => { | ||||||||||
| reject(new Error("Timeout reading from STDIN after 30 seconds")); | ||||||||||
| }, 10); // Very short timeout for test | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| try { | ||||||||||
| const readPromise = (async () => { | ||||||||||
| for await (const chunk of mockStdin) { | ||||||||||
| chunks.push(Buffer.from(chunk)); | ||||||||||
| } | ||||||||||
| return Buffer.concat(chunks).toString().trim(); | ||||||||||
| })(); | ||||||||||
|
|
||||||||||
| const result = await Promise.race([readPromise, timeoutPromise]); | ||||||||||
| return result; | ||||||||||
|
Comment on lines
+77
to
+78
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (code-quality): Inline variable that is immediately returned (
Suggested change
ExplanationSomething that we often see in people's code is assigning to a result variableand then immediately returning it. Returning the result directly shortens the code and removes an unnecessary Where intermediate variables can be useful is if they then get used as a |
||||||||||
| } finally { | ||||||||||
| if (timeout) { | ||||||||||
| clearTimeout(timeout); | ||||||||||
| } | ||||||||||
| } | ||||||||||
| }; | ||||||||||
|
|
||||||||||
| await expect(readStdinTokenWithShortTimeout()).rejects.toThrow("Timeout reading from STDIN after 30 seconds"); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| test("properly cleans up timeout on success", async () => { | ||||||||||
| const testToken = "cleanup-test-token"; | ||||||||||
| let timeoutCleared = false; | ||||||||||
|
|
||||||||||
| // Mock setTimeout and clearTimeout to verify cleanup | ||||||||||
| const originalSetTimeout = global.setTimeout; | ||||||||||
| const originalClearTimeout = global.clearTimeout; | ||||||||||
|
|
||||||||||
| // biome-ignore lint/suspicious/noExplicitAny: Test mocking requires any | ||||||||||
| global.setTimeout = mock((fn: any, delay: number) => { | ||||||||||
| return originalSetTimeout(fn, delay); | ||||||||||
| // biome-ignore lint/suspicious/noExplicitAny: Test mocking requires any | ||||||||||
| }) as any; | ||||||||||
|
|
||||||||||
| // biome-ignore lint/suspicious/noExplicitAny: Test mocking requires any | ||||||||||
| global.clearTimeout = mock((id: any) => { | ||||||||||
| timeoutCleared = true; | ||||||||||
| return originalClearTimeout(id); | ||||||||||
| // biome-ignore lint/suspicious/noExplicitAny: Test mocking requires any | ||||||||||
| }) as any; | ||||||||||
|
|
||||||||||
| const mockStdin = new Readable({ | ||||||||||
| read() { | ||||||||||
| this.push(testToken); | ||||||||||
| this.push(null); | ||||||||||
| }, | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| const readStdinTokenWithCleanup = async (): Promise<string> => { | ||||||||||
| const chunks: Buffer[] = []; | ||||||||||
| let timeout: NodeJS.Timeout | undefined; | ||||||||||
|
|
||||||||||
| const timeoutPromise = new Promise<never>((_, reject) => { | ||||||||||
| timeout = setTimeout(() => { | ||||||||||
| reject(new Error("Timeout reading from STDIN after 30 seconds")); | ||||||||||
| }, 30_000); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| try { | ||||||||||
| const readPromise = (async () => { | ||||||||||
| for await (const chunk of mockStdin) { | ||||||||||
| chunks.push(Buffer.from(chunk)); | ||||||||||
| } | ||||||||||
| return Buffer.concat(chunks).toString().trim(); | ||||||||||
| })(); | ||||||||||
|
|
||||||||||
| const result = await Promise.race([readPromise, timeoutPromise]); | ||||||||||
| return result; | ||||||||||
|
Comment on lines
+135
to
+136
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (code-quality): Inline variable that is immediately returned (
Suggested change
ExplanationSomething that we often see in people's code is assigning to a result variableand then immediately returning it. Returning the result directly shortens the code and removes an unnecessary Where intermediate variables can be useful is if they then get used as a |
||||||||||
| } finally { | ||||||||||
| if (timeout) { | ||||||||||
| clearTimeout(timeout); | ||||||||||
| } | ||||||||||
| } | ||||||||||
| }; | ||||||||||
|
|
||||||||||
| const result = await readStdinTokenWithCleanup(); | ||||||||||
|
|
||||||||||
| expect(result).toBe(testToken); | ||||||||||
| expect(timeoutCleared).toBe(true); | ||||||||||
| expect(global.clearTimeout).toHaveBeenCalled(); | ||||||||||
|
|
||||||||||
| // Restore original functions | ||||||||||
| global.setTimeout = originalSetTimeout; | ||||||||||
| global.clearTimeout = originalClearTimeout; | ||||||||||
| }); | ||||||||||
| }); | ||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -53,16 +53,33 @@ export function loginCommand(): Command { | |||||||||
| if (cmd.args.length > 0) { | ||||||||||
| cancel("A token should be provided using STDIN, not as an argument"); | ||||||||||
| } | ||||||||||
| personalAccessToken = await Promise.race([ | ||||||||||
| (async () => { | ||||||||||
| const chunks: Buffer[] = []; | ||||||||||
| for await (const chunk of process.stdin) { | ||||||||||
| chunks.push(Buffer.from(chunk)); | ||||||||||
| personalAccessToken = await (async () => { | ||||||||||
| const chunks: Buffer[] = []; | ||||||||||
| let timeout: NodeJS.Timeout | undefined; | ||||||||||
|
|
||||||||||
| // Set up a timeout to prevent hanging if STDIN never closes | ||||||||||
| const timeoutPromise = new Promise<never>((_, reject) => { | ||||||||||
| timeout = setTimeout(() => { | ||||||||||
| reject(new Error("Timeout reading from STDIN after 30 seconds")); | ||||||||||
| }, 30_000); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| try { | ||||||||||
| const readPromise = (async () => { | ||||||||||
| for await (const chunk of process.stdin) { | ||||||||||
| chunks.push(Buffer.from(chunk)); | ||||||||||
| } | ||||||||||
| return Buffer.concat(chunks).toString().trim(); | ||||||||||
| })(); | ||||||||||
|
|
||||||||||
| const result = await Promise.race([readPromise, timeoutPromise]); | ||||||||||
| return result; | ||||||||||
|
Comment on lines
+75
to
+76
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (code-quality): Inline variable that is immediately returned (
Suggested change
ExplanationSomething that we often see in people's code is assigning to a result variableand then immediately returning it. Returning the result directly shortens the code and removes an unnecessary Where intermediate variables can be useful is if they then get used as a |
||||||||||
| } finally { | ||||||||||
| if (timeout) { | ||||||||||
| clearTimeout(timeout); | ||||||||||
| } | ||||||||||
| return Buffer.concat(chunks).toString().trim(); | ||||||||||
| })(), | ||||||||||
| new Promise<string>((resolve) => setTimeout(() => resolve(""), 1_000)), | ||||||||||
| ]); | ||||||||||
| } | ||||||||||
| })(); | ||||||||||
| try { | ||||||||||
| validate(PersonalAccessTokenSchema, personalAccessToken); | ||||||||||
| } catch { | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,97 @@ | ||||||||||||||||||||||||||||||||||||||||||||||
| /* eslint-disable @typescript-eslint/no-explicit-any */ | ||||||||||||||||||||||||||||||||||||||||||||||
| import { describe, expect, mock, test } from "bun:test"; | ||||||||||||||||||||||||||||||||||||||||||||||
| import { waitForCompletion } from "./wait-for-completion.js"; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| // Mock the settlemint client and services | ||||||||||||||||||||||||||||||||||||||||||||||
| const mockRead = mock(() => Promise.resolve({ status: "COMPLETED" })); | ||||||||||||||||||||||||||||||||||||||||||||||
| const mockRestart = mock(() => Promise.resolve()); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| const mockService = { | ||||||||||||||||||||||||||||||||||||||||||||||
| read: mockRead, | ||||||||||||||||||||||||||||||||||||||||||||||
| restart: mockRestart, | ||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+6
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add mocks for testing polling and timeout scenarios. The current mocks always return "COMPLETED" status immediately, which doesn't test the actual polling behavior or timeout logic that was fixed according to the PR objectives. Add additional mocks to test polling scenarios: +// Mock for testing polling behavior
+const mockPendingThenCompleted = mock()
+ .mockReturnValueOnce(Promise.resolve({ status: "PENDING" }))
+ .mockReturnValueOnce(Promise.resolve({ status: "COMPLETED" }));
+
+// Mock for testing timeout scenarios
+const mockAlwaysPending = mock(() => Promise.resolve({ status: "PENDING" }));📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| const mockSettlemintClient = { | ||||||||||||||||||||||||||||||||||||||||||||||
| blockchainNetwork: mockService, | ||||||||||||||||||||||||||||||||||||||||||||||
| blockchainNode: mockService, | ||||||||||||||||||||||||||||||||||||||||||||||
| customDeployment: mockService, | ||||||||||||||||||||||||||||||||||||||||||||||
| insights: mockService, | ||||||||||||||||||||||||||||||||||||||||||||||
| integrationTool: mockService, | ||||||||||||||||||||||||||||||||||||||||||||||
| loadBalancer: mockService, | ||||||||||||||||||||||||||||||||||||||||||||||
| middleware: mockService, | ||||||||||||||||||||||||||||||||||||||||||||||
| privateKey: mockService, | ||||||||||||||||||||||||||||||||||||||||||||||
| storage: mockService, | ||||||||||||||||||||||||||||||||||||||||||||||
| // biome-ignore lint/suspicious/noExplicitAny: Test mocking requires any | ||||||||||||||||||||||||||||||||||||||||||||||
| } as any; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| describe("waitForCompletion", () => { | ||||||||||||||||||||||||||||||||||||||||||||||
| test("resolves immediately for workspace resource type", async () => { | ||||||||||||||||||||||||||||||||||||||||||||||
| const result = await waitForCompletion({ | ||||||||||||||||||||||||||||||||||||||||||||||
| settlemint: mockSettlemintClient, | ||||||||||||||||||||||||||||||||||||||||||||||
| type: "workspace", | ||||||||||||||||||||||||||||||||||||||||||||||
| uniqueName: "test-workspace", | ||||||||||||||||||||||||||||||||||||||||||||||
| action: "deploy", | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| expect(result).toBe(true); | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| test("resolves immediately for application resource type", async () => { | ||||||||||||||||||||||||||||||||||||||||||||||
| const result = await waitForCompletion({ | ||||||||||||||||||||||||||||||||||||||||||||||
| settlemint: mockSettlemintClient, | ||||||||||||||||||||||||||||||||||||||||||||||
| type: "application", | ||||||||||||||||||||||||||||||||||||||||||||||
| uniqueName: "test-app", | ||||||||||||||||||||||||||||||||||||||||||||||
| action: "deploy", | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| expect(result).toBe(true); | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| test("calls service read method", async () => { | ||||||||||||||||||||||||||||||||||||||||||||||
| // Mock service that completes immediately | ||||||||||||||||||||||||||||||||||||||||||||||
| const mockCompleteRead = mock(() => Promise.resolve({ status: "COMPLETED" })); | ||||||||||||||||||||||||||||||||||||||||||||||
| const completeMockService = { read: mockCompleteRead, restart: mockRestart }; | ||||||||||||||||||||||||||||||||||||||||||||||
| // biome-ignore lint/suspicious/noExplicitAny: Test mocking requires any | ||||||||||||||||||||||||||||||||||||||||||||||
| const completeMockClient = { blockchainNetwork: completeMockService } as any; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| const result = await waitForCompletion({ | ||||||||||||||||||||||||||||||||||||||||||||||
| settlemint: completeMockClient, | ||||||||||||||||||||||||||||||||||||||||||||||
| type: "blockchain network", | ||||||||||||||||||||||||||||||||||||||||||||||
| uniqueName: "test-network", | ||||||||||||||||||||||||||||||||||||||||||||||
| action: "deploy", | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| expect(result).toBe(true); | ||||||||||||||||||||||||||||||||||||||||||||||
| expect(mockCompleteRead).toHaveBeenCalledWith("test-network"); | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| test("handles failed status correctly", async () => { | ||||||||||||||||||||||||||||||||||||||||||||||
| const mockFailedRead = mock(() => Promise.resolve({ status: "FAILED" })); | ||||||||||||||||||||||||||||||||||||||||||||||
| const failedMockService = { read: mockFailedRead, restart: mockRestart }; | ||||||||||||||||||||||||||||||||||||||||||||||
| // biome-ignore lint/suspicious/noExplicitAny: Test mocking requires any | ||||||||||||||||||||||||||||||||||||||||||||||
| const failedMockClient = { blockchainNetwork: failedMockService } as any; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| const result = await waitForCompletion({ | ||||||||||||||||||||||||||||||||||||||||||||||
| settlemint: failedMockClient, | ||||||||||||||||||||||||||||||||||||||||||||||
| type: "blockchain network", | ||||||||||||||||||||||||||||||||||||||||||||||
| uniqueName: "test-network", | ||||||||||||||||||||||||||||||||||||||||||||||
| action: "deploy", | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| expect(result).toBe(false); | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| test("throws error for unsupported service type", async () => { | ||||||||||||||||||||||||||||||||||||||||||||||
| // biome-ignore lint/suspicious/noExplicitAny: Test mocking requires any | ||||||||||||||||||||||||||||||||||||||||||||||
| const mockClientWithoutService = {} as any; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| await expect(() => | ||||||||||||||||||||||||||||||||||||||||||||||
| waitForCompletion({ | ||||||||||||||||||||||||||||||||||||||||||||||
| settlemint: mockClientWithoutService, | ||||||||||||||||||||||||||||||||||||||||||||||
| type: "blockchain network" as const, | ||||||||||||||||||||||||||||||||||||||||||||||
| uniqueName: "test-network", | ||||||||||||||||||||||||||||||||||||||||||||||
| action: "deploy", | ||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||
| ).toThrow("Service blockchainNetwork does not support status checking"); | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| /* eslint-disable @typescript-eslint/no-explicit-any */ | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { parseNumber } from "./parse-number.js"; | ||
|
|
||
| describe("parseNumber", () => { | ||
| test("parses valid integer strings", () => { | ||
| expect(parseNumber("123")).toBe(123); | ||
| expect(parseNumber("0")).toBe(0); | ||
| expect(parseNumber("-42")).toBe(-42); | ||
| }); | ||
|
|
||
| test("parses valid decimal strings", () => { | ||
| expect(parseNumber("3.14")).toBe(3.14); | ||
| expect(parseNumber("-2.5")).toBe(-2.5); | ||
| expect(parseNumber("0.0")).toBe(0); | ||
| }); | ||
|
|
||
| test("handles whitespace trimming", () => { | ||
| expect(parseNumber(" 123 ")).toBe(123); | ||
| expect(parseNumber("\t42\n")).toBe(42); | ||
| expect(parseNumber(" -3.14 ")).toBe(-3.14); | ||
| }); | ||
|
|
||
| test("throws error for non-string input", () => { | ||
| // biome-ignore lint/suspicious/noExplicitAny: Testing type validation | ||
| expect(() => parseNumber(123 as any)).toThrow("Value must be a string"); | ||
| // biome-ignore lint/suspicious/noExplicitAny: Testing type validation | ||
| expect(() => parseNumber(null as any)).toThrow("Value must be a string"); | ||
| // biome-ignore lint/suspicious/noExplicitAny: Testing type validation | ||
| expect(() => parseNumber(undefined as any)).toThrow("Value must be a string"); | ||
| }); | ||
|
|
||
| test("throws error for empty strings", () => { | ||
| expect(() => parseNumber("")).toThrow("Value cannot be empty"); | ||
| expect(() => parseNumber(" ")).toThrow("Value cannot be empty"); | ||
| expect(() => parseNumber("\t\n")).toThrow("Value cannot be empty"); | ||
| }); | ||
|
|
||
| test("throws error for invalid number strings", () => { | ||
| expect(() => parseNumber("abc")).toThrow('"abc" is not a valid number'); | ||
| expect(() => parseNumber("12.34.56")).toThrow('"12.34.56" is not a valid number'); | ||
| expect(() => parseNumber("not-a-number")).toThrow('"not-a-number" is not a valid number'); | ||
| }); | ||
|
|
||
| test("throws error for infinity values", () => { | ||
| expect(() => parseNumber("Infinity")).toThrow('"Infinity" is not a finite number'); | ||
| expect(() => parseNumber("-Infinity")).toThrow('"-Infinity" is not a finite number'); | ||
| }); | ||
|
|
||
| test("handles scientific notation", () => { | ||
| expect(parseNumber("1e5")).toBe(100000); | ||
| expect(parseNumber("2.5e-2")).toBe(0.025); | ||
| expect(parseNumber("-3e4")).toBe(-30000); | ||
| }); | ||
|
|
||
| test("handles edge cases", () => { | ||
| expect(parseNumber("0")).toBe(0); | ||
| expect(parseNumber("-0")).toBe(-0); | ||
| expect(parseNumber("0.000001")).toBe(0.000001); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Inline variable that is immediately returned (
inline-immediately-returned-variable)Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.