Add tests #104
Add tests #104
Conversation
🦋 Changeset detectedLatest commit: bec6cdd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate Limit Exceeded@kamiazya has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 14 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates streamline and enhance the robustness of CSV parsing and string conversion functionalities. Simplifications in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #104 +/- ##
==========================================
+ Coverage 94.50% 99.44% +4.94%
==========================================
Files 35 36 +1
Lines 364 363 -1
Branches 109 107 -2
==========================================
+ Hits 344 361 +17
+ Misses 14 2 -12
+ Partials 6 0 -6 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (7)
- src/Lexer.ts (1 hunks)
- src/assertCommonOptions.spec.ts (1 hunks)
- src/convertBinaryToString.spec.ts (1 hunks)
- src/parse.spec.ts (3 hunks)
- src/parseResponseToStream.spec.ts (1 hunks)
- src/utils/parseMime.spec.ts (1 hunks)
- vite.config.ts (1 hunks)
Additional comments: 4
vite.config.ts (1)
- 49-54: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The removal of
all: falsefrom thecoverageconfiguration invite.config.tsis not explicitly shown in the provided code. Ensure that this change aligns with the intended test coverage behavior, potentially leading to more comprehensive coverage reporting.Verification successful
The revised script successfully executed, searching for the
coverage: {pattern invite.config.tsand filtering out any lines containingall: false. The output indicates that thecoverage: {pattern was found, but no lines were excluded by thegrep -v "all: false"command, suggesting thatall: falseis not present within the coverage configuration block. This aligns with the review comment's concern about the removal ofall: falsefrom the coverage configuration.Based on this information, it can be concluded that the review comment is accurate in noting the absence of
all: falsewithin the coverage configuration invite.config.ts, which was the point of verification. Therefore, the verification process supports the original review comment's concern.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the removal of `all: false` in coverage configuration rg "coverage: {" vite.config.ts | grep -v "all: false"Length of output: 142
Script:
#!/bin/bash # Correctly verify the removal of `all: false` in coverage configuration by escaping the curly brace rg "coverage: \\{" vite.config.ts | grep -v "all: false"Length of output: 73
src/utils/parseMime.spec.ts (1)
- 45-57: The new test case for parsing mime types with multiple parameters is a good addition. Ensure consistency in the use of spaces around parameters across tests for better readability.
src/assertCommonOptions.spec.ts (1)
- 56-84: The new test case for validating delimiters that include CR or LF characters is well-implemented. It effectively uses property-based testing to cover a wide range of input scenarios.
src/Lexer.ts (1)
- 76-76: Directly updating
currentFieldin the default case of the switch statement simplifies the control flow. Ensure this change does not affect the handling of edge cases wherecurrentFieldmight need to be processed differently.
| describe("function convertBinaryToString", () => { | ||
| it("should convert Uint8Array to string", () => { | ||
| const text = "abc"; | ||
| const binary = new TextEncoder().encode(text); | ||
| const result = convertBinaryToString(binary, {}); | ||
| expect(result).toBe(text); | ||
| }); | ||
|
|
||
| it("should convert ArrayBuffer to string", () => { | ||
| const text = "abc"; | ||
| const binary = new TextEncoder().encode(text).buffer; | ||
| const result = convertBinaryToString(binary, {}); | ||
| expect(result).toBe(text); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test case for converting Uint8Array to string does not verify the behavior with different encodings or non-ASCII characters. Consider adding tests for various character sets and binary data scenarios.
+ it("should handle non-ASCII characters in Uint8Array", () => {
+ const text = "こんにちは"; // Hello in Japanese
+ const binary = new TextEncoder().encode(text);
+ const result = convertBinaryToString(binary, {});
+ expect(result).toBe(text);
+ });Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| describe("function convertBinaryToString", () => { | |
| it("should convert Uint8Array to string", () => { | |
| const text = "abc"; | |
| const binary = new TextEncoder().encode(text); | |
| const result = convertBinaryToString(binary, {}); | |
| expect(result).toBe(text); | |
| }); | |
| it("should convert ArrayBuffer to string", () => { | |
| const text = "abc"; | |
| const binary = new TextEncoder().encode(text).buffer; | |
| const result = convertBinaryToString(binary, {}); | |
| expect(result).toBe(text); | |
| }); | |
| }); | |
| describe("function convertBinaryToString", () => { | |
| it("should convert Uint8Array to string", () => { | |
| const text = "abc"; | |
| const binary = new TextEncoder().encode(text); | |
| const result = convertBinaryToString(binary, {}); | |
| expect(result).toBe(text); | |
| }); | |
| it("should handle non-ASCII characters in Uint8Array", () => { | |
| const text = "こんにちは"; // Hello in Japanese | |
| const binary = new TextEncoder().encode(text); | |
| const result = convertBinaryToString(binary, {}); | |
| expect(result).toBe(text); | |
| }); | |
| it("should convert ArrayBuffer to string", () => { | |
| const text = "abc"; | |
| const binary = new TextEncoder().encode(text).buffer; | |
| const result = convertBinaryToString(binary, {}); | |
| expect(result).toBe(text); | |
| }); | |
| }); |
| it("should convert ArrayBuffer to string", () => { | ||
| const text = "abc"; | ||
| const binary = new TextEncoder().encode(text).buffer; | ||
| const result = convertBinaryToString(binary, {}); | ||
| expect(result).toBe(text); | ||
| }); |
There was a problem hiding this comment.
Similar to the Uint8Array test, this test for converting ArrayBuffer to string should also include scenarios with different encodings and non-ASCII characters to ensure comprehensive coverage.
+ it("should handle non-ASCII characters in ArrayBuffer", () => {
+ const text = "Привет"; // Hello in Russian
+ const binary = new TextEncoder().encode(text).buffer;
+ const result = convertBinaryToString(binary, {});
+ expect(result).toBe(text);
+ });Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| it("should convert ArrayBuffer to string", () => { | |
| const text = "abc"; | |
| const binary = new TextEncoder().encode(text).buffer; | |
| const result = convertBinaryToString(binary, {}); | |
| expect(result).toBe(text); | |
| }); | |
| it("should convert ArrayBuffer to string", () => { | |
| const text = "abc"; | |
| const binary = new TextEncoder().encode(text).buffer; | |
| const result = convertBinaryToString(binary, {}); | |
| expect(result).toBe(text); | |
| }); | |
| it("should handle non-ASCII characters in ArrayBuffer", () => { | |
| const text = "Привет"; // Hello in Russian | |
| const binary = new TextEncoder().encode(text).buffer; | |
| const result = convertBinaryToString(binary, {}); | |
| expect(result).toBe(text); | |
| }); |
src/parseResponseToStream.spec.ts
Outdated
| describe("parseResponseToStream", () => { | ||
| it("should parse a response to a stream", () => { | ||
| const csv = "a,b,c\n1,2,3\n4,5,6"; | ||
| const response = new Response(csv, { | ||
| headers: { | ||
| "content-type": "text/csv", | ||
| }, | ||
| }); | ||
| const stream = parseResponseToStream(response); | ||
| expect(stream).toBeInstanceOf(ReadableStream); | ||
| }); | ||
|
|
||
| it("should throw error if content-type header is not text/csv", () => { | ||
| const response = new Response("", { | ||
| headers: { | ||
| "content-type": "application/json", | ||
| }, | ||
| }); | ||
| expect(() => parseResponseToStream(response)).toThrow( | ||
| `Invalid mime type: ${response.headers.get("content-type")}`, | ||
| ); | ||
| }); | ||
|
|
||
| it("should throw error if request body is null", () => { | ||
| const response = new Response(null, { | ||
| headers: { | ||
| "content-type": "text/csv", | ||
| }, | ||
| }); | ||
| expect(() => parseResponseToStream(response)).toThrow( | ||
| "Response body is null", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
The tests for parseResponseToStream cover essential scenarios, including content type validation and null body handling. Consider adding a test case for handling non-CSV content types that are still text-based, to verify that the error message is appropriate.
+ it("should throw error for non-CSV text content-type", () => {
+ const response = new Response("data", {
+ headers: {
+ "content-type": "text/plain",
+ },
+ });
+ expect(() => parseResponseToStream(response)).toThrow(
+ `Invalid mime type: text/plain`,
+ );
+ });Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| describe("parseResponseToStream", () => { | |
| it("should parse a response to a stream", () => { | |
| const csv = "a,b,c\n1,2,3\n4,5,6"; | |
| const response = new Response(csv, { | |
| headers: { | |
| "content-type": "text/csv", | |
| }, | |
| }); | |
| const stream = parseResponseToStream(response); | |
| expect(stream).toBeInstanceOf(ReadableStream); | |
| }); | |
| it("should throw error if content-type header is not text/csv", () => { | |
| const response = new Response("", { | |
| headers: { | |
| "content-type": "application/json", | |
| }, | |
| }); | |
| expect(() => parseResponseToStream(response)).toThrow( | |
| `Invalid mime type: ${response.headers.get("content-type")}`, | |
| ); | |
| }); | |
| it("should throw error if request body is null", () => { | |
| const response = new Response(null, { | |
| headers: { | |
| "content-type": "text/csv", | |
| }, | |
| }); | |
| expect(() => parseResponseToStream(response)).toThrow( | |
| "Response body is null", | |
| ); | |
| }); | |
| describe("parseResponseToStream", () => { | |
| it("should parse a response to a stream", () => { | |
| const csv = "a,b,c\n1,2,3\n4,5,6"; | |
| const response = new Response(csv, { | |
| headers: { | |
| "content-type": "text/csv", | |
| }, | |
| }); | |
| const stream = parseResponseToStream(response); | |
| expect(stream).toBeInstanceOf(ReadableStream); | |
| }); | |
| it("should throw error if content-type header is not text/csv", () => { | |
| const response = new Response("", { | |
| headers: { | |
| "content-type": "application/json", | |
| }, | |
| }); | |
| expect(() => parseResponseToStream(response)).toThrow( | |
| `Invalid mime type: ${response.headers.get("content-type")}`, | |
| ); | |
| }); | |
| it("should throw error for non-CSV text content-type", () => { | |
| const response = new Response("data", { | |
| headers: { | |
| "content-type": "text/plain", | |
| }, | |
| }); | |
| expect(() => parseResponseToStream(response)).toThrow( | |
| `Invalid mime type: text/plain`, | |
| ); | |
| }); | |
| it("should throw error if request body is null", () => { | |
| const response = new Response(null, { | |
| headers: { | |
| "content-type": "text/csv", | |
| }, | |
| }); | |
| expect(() => parseResponseToStream(response)).toThrow( | |
| "Response body is null", | |
| ); | |
| }); | |
| } |
src/parse.spec.ts
Outdated
| g(FC.row, { | ||
| fieldConstraints: { | ||
| minLength: 1, | ||
| kindExcludes: ["string16bits"], | ||
| }, | ||
| columnsConstraints: { | ||
| minLength: header.length, | ||
| maxLength: header.length, | ||
| }, | ||
| }), | ||
| ]; | ||
| const csv = [ | ||
| header.map((v) => escapeField(v)).join(","), | ||
| EOL, | ||
| ...csvData.flatMap((row, i) => [ | ||
| ...row.map((v) => escapeField(v)).join(","), | ||
| ...(EOF || csvData.length - 1 !== i ? [EOL] : []), | ||
| ]), | ||
| ].join(""); | ||
| const data = csvData.map((row) => | ||
| Object.fromEntries(row.map((v, i) => [header[i], v])), | ||
| ); | ||
| return { | ||
| data, | ||
| csv: new SingleValueReadableStream(new TextEncoder().encode(csv)), | ||
| }; | ||
| }), | ||
| async ({ data, csv }) => { | ||
| let i = 0; | ||
| for await (const row of parse(csv)) { | ||
| expect(row).toEqual(data[i++]); | ||
| } | ||
| }, | ||
| ), | ||
| )); | ||
|
|
||
| it("should parse CSV Response", () => | ||
| fc.assert( | ||
| fc.asyncProperty( | ||
| fc.gen().map((g) => { | ||
| const header = g(FC.header, { | ||
| fieldConstraints: { | ||
| kindExcludes: ["string16bits"], | ||
| }, | ||
| }); | ||
| const EOL = g(FC.eol); | ||
| const EOF = g(fc.boolean); | ||
| const csvData = [ | ||
| ...g(FC.csvData, { | ||
| rowsConstraints: { | ||
| minLength: 1, | ||
| }, | ||
| columnsConstraints: { | ||
| minLength: header.length, | ||
| maxLength: header.length, | ||
| }, | ||
| fieldConstraints: { | ||
| kindExcludes: ["string16bits"], | ||
| }, | ||
| }), | ||
| // Last row is not empty for testing. | ||
| g(FC.row, { | ||
| fieldConstraints: { | ||
| minLength: 1, | ||
| kindExcludes: ["string16bits"], | ||
| }, | ||
| columnsConstraints: { | ||
| minLength: header.length, | ||
| maxLength: header.length, | ||
| }, | ||
| }), | ||
| ]; | ||
| const csv = [ | ||
| header.map((v) => escapeField(v)).join(","), | ||
| EOL, | ||
| ...csvData.flatMap((row, i) => [ | ||
| ...row.map((v) => escapeField(v)).join(","), | ||
| ...(EOF || csvData.length - 1 !== i ? [EOL] : []), | ||
| ]), | ||
| ].join(""); | ||
| const data = csvData.map((row) => | ||
| Object.fromEntries(row.map((v, i) => [header[i], v])), | ||
| ); | ||
| return { | ||
| data, | ||
| csv: new Response( | ||
| new SingleValueReadableStream(new TextEncoder().encode(csv)), | ||
| { headers: { "content-type": "text/csv" } }, | ||
| ), | ||
| }; | ||
| }), | ||
| async ({ data, csv }) => { | ||
| let i = 0; | ||
| for await (const row of parse(csv)) { | ||
| expect(row).toEqual(data[i++]); | ||
| } | ||
| }, | ||
| ), | ||
| )); | ||
| }); |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [8-369]
The tests for the parse function now cover a wide range of input sources, including CSV strings, buffers, and streams. This comprehensive testing ensures robust parsing functionality. For further improvement, consider adding tests for malformed CSV inputs to verify error handling and resilience.
+ it("should handle malformed CSV strings gracefully", async () => {
+ const malformedCsv = "a,b,c\n1,\"2";
+ await expect(async () => {
+ for await (const _ of parse(malformedCsv)) {}
+ }).rejects.toThrow("Unexpected end of input in quoted field");
+ });
This pull request adds tests for the existing code to improve code quality and ensure proper functionality.
Summary by CodeRabbit
assertCommonOptions.parsefunction tests to handle various input sources asynchronously.