chore: add tests for query params #430
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 9a6ea8a in 22 seconds
More details
- Looked at
160lines of code in3files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. langfuse-core/src/utils.ts:116
- Draft comment:
Consider encoding array values in a way that is compatible with your API, such as using multiple key-value pairs for each element or a different delimiter. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_AlpN9hwJWWgTksAo
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| traceId: trace.id, | ||
| version: null, | ||
| }); | ||
| it.only("create span", async () => { |
There was a problem hiding this comment.
Remove it.only to ensure all tests run.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on dfedb37 in 18 seconds
More details
- Looked at
74lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. integration-test/langfuse-integration-node.spec.ts:117
- Draft comment:
The test case 'create span' is duplicated. Consider removing the commented-out version to avoid confusion. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is relevant because it addresses a change in the diff where a test case was uncommented and modified. The suggestion to remove the commented-out version is actionable and clear, as it helps maintain code clarity by removing unnecessary commented-out code.
The comment might be considered unnecessary if the commented-out code is already removed in the diff, making the suggestion redundant.
The comment is still useful as it highlights the importance of removing commented-out code to avoid confusion, even if it was already addressed in the diff.
Keep the comment as it provides a clear and actionable suggestion related to the changes made in the diff.
Workflow ID: wflow_YcULl8vjgJ6ih7LT
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Disclaimer: Experimental PR review
PR Summary
This pull request refactors the query parameter encoding and adds corresponding tests, with a focus on improving array handling in the Langfuse Node.js SDK.
- Refactored
encodeQueryParamsfunction inlangfuse-core/src/utils.tsfor better array support and security - Added new tests in
langfuse-core/test/utils.spec.tsto verify query parameter encoding, including arrays and special characters - Modified 'create span' test in
integration-test/langfuse-integration-node.spec.tsto fetch traces with specific tags (temporary change) - Improved encoding of both keys and values using
encodeURIComponentfor enhanced security and compatibility
3 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings
| value instanceof Date ? encodeURIComponent(value.toISOString()) : encodeURIComponent(value.toString()); | ||
| return `${encodedKey}=${encodedValue}`; |
There was a problem hiding this comment.
logic: This doesn't handle array values correctly. Arrays will be converted to strings, potentially losing data. Consider adding specific array handling.
| describe("encodeQueryParams", () => { | ||
| it("should encode query parameters correctly", () => { | ||
| const params = { | ||
| name: "John Doe", | ||
| age: 30, | ||
| active: true, | ||
| date: new Date("2022-01-01T00:00:00.000Z"), | ||
| empty: null, | ||
| undefinedValue: undefined, | ||
| }; | ||
| const expected = "name=John%20Doe&age=30&active=true&date=2022-01-01T00%3A00%3A00.000Z"; | ||
| expect(encodeQueryParams(params)).toEqual(expected); | ||
| }); |
There was a problem hiding this comment.
logic: Test case doesn't check handling of null and undefined values
Important
Add tests and modify
encodeQueryParamsto handle special characters and arrays, and comment out a test inlangfuse-integration-node.spec.ts.encodeQueryParamsinutils.spec.tsto handle special characters, arrays, and null/undefined values.encodeQueryParamsinutils.tsto encode special characters and arrays correctly.create spantest inlangfuse-integration-node.spec.ts.This description was created by
for dfedb37. It will automatically update as commits are pushed.