-
Notifications
You must be signed in to change notification settings - Fork 106
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
Fix missing Typescript integ tests after split #1639
base: canary
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
👍 Looks good to me! Reviewed everything up to 86aa951 in 2 minutes and 35 seconds
More details
- Looked at
245
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
12
drafted comments based on config settings.
1. integ-tests/typescript/tests/input-output.test.ts:59
- Draft comment:
Good refactoring! Separating output tests into multiple 'it' blocks improves clarity and debugging. Ensure that the repetitive use of the input 'test input' in each block is intentional. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. integ-tests/typescript/tests/input-output.test.ts:112
- Draft comment:
Test for 'single map string to string' looks correct. Verify if deep equality is sufficient for map comparisons. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. integ-tests/typescript/tests/input-output.test.ts:107
- Draft comment:
The 'single optional string' test expects the response to contain 'none'. Confirm that this output is the desired default/placeholder. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. integ-tests/typescript/tests/integ-tests.test.ts.old:41
- Draft comment:
Several AWS-related tests have been removed. Please confirm that these tests are now covered elsewhere or that their removal is intentional. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. integ-tests/typescript/tests/integ-tests.test.ts.old:173
- Draft comment:
Found an 'it.only' in the OpenAI shorthand streaming test. Remember to remove 'it.only' to avoid running only that test in CI. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. integ-tests/typescript/tests/integ-tests.test.ts.old:28
- Draft comment:
With many tests removed from this file, consider if the file should be entirely removed to avoid confusion, especially if tests have been split properly. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. integ-tests/typescript/tests/input-output.test.ts:63
- Draft comment:
Consider extracting the repeated 'test input' constant into a shared variable to reduce duplication. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
8. integ-tests/typescript/tests/input-output.test.ts:75
- Draft comment:
The variable name 'classs' appears to have an extra 's'. Consider renaming it for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While technically correct that there's a typo in the variable name, this is a test file and the variable is only used in a very small scope (3 lines). The meaning is completely clear despite the typo. This seems like an unnecessarily nitpicky comment that doesn't materially improve code quality.
The typo could be seen as unprofessional and might confuse readers at first glance. It's a simple fix that would improve code cleanliness.
The benefit of fixing this minor typo is outweighed by the overhead of making a code change. The variable is used in a very limited scope in a test file and its meaning is clear.
Delete this comment as it's too minor of an issue to warrant a code change, especially in a test file.
9. integ-tests/typescript/tests/integ-tests.test.ts.old:173
- Draft comment:
Remove 'it.only' from the OpenAI shorthand streaming test to ensure all tests run. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. integ-tests/typescript/tests/input-output.test.ts:75
- Draft comment:
Minor typographical error: the variable name 'classs' seems to have an extra 's'. If this is unintentional, please consider renaming it to a more appropriate name (perhaps 'cls' or 'outputClass'). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While the comment is technically correct about the typo, variable naming issues are relatively minor and would be caught during code review anyway. The extra 's' doesn't impact functionality, and the variable is used consistently within its small scope. According to the rules, we should not make comments that are obvious or unimportant.
The typo could potentially cause confusion for future maintainers. Also, consistent naming is part of code quality.
While code quality is important, this is a very minor issue in a test file, and the variable name is used consistently in its small scope. The benefit of fixing this is minimal.
This comment should be removed as it points out a trivial naming issue that doesn't impact functionality and would be caught during normal code review.
11. integ-tests/typescript/tests/integ-tests.test.ts.old:578
- Draft comment:
Potential typographical error: Consider renaming 'atopLevelAsyncTracing' to 'topLevelAsyncTracing' for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. integ-tests/typescript/tests/integ-tests.test.ts.old:494
- Draft comment:
Potential typographical error: The variable 'capitol' in the test might be intended to be 'capital' (referring to the capital city). Please verify and correct if necessary. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_YcEuCMkBeV5BRvoy
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Reorganized and added missing TypeScript integration tests for input/output types and semantic streaming, while removing redundant tests.
input-output.test.ts
for single bool, int, class, class list, enum list, optional list and map, optional string, map string to string, map string to map, literal int, bool, and string.input-output.test.ts
.integ-tests.test.ts.old
.integ-tests.test.ts.old
.This description was created by
for 86aa951. It will automatically update as commits are pushed.