-
Notifications
You must be signed in to change notification settings - Fork 170
Add all missing TS integ tests & remove integ-tests.old.ts
#1992
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
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.
Caution
Changes requested ❌
Reviewed everything up to ae7bc16 in 1 minute and 54 seconds. Click for details.
- Reviewed
1154
lines of code in12
files - Skipped
0
files when reviewing. - Skipped posting
5
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. integ-tests/typescript/tests/providers/aws.test.ts:205
- Draft comment:
Remove the use of 'it.only' to avoid restricting the test suite to only this test. - 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.
2. integ-tests/typescript/tests/providers/aws.test.ts:348
- Draft comment:
Remove 'it.only' here as well. Debug-only markers can prevent other tests from running. - 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.
3. integ-tests/typescript/tests/tracing.test.ts:68
- Draft comment:
The fixed assertion 'expect(stats.started).toBe(28)' may be brittle if tracing internals change. Consider a more flexible assertion (e.g. checking a minimum count). - 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.
4. integ-tests/typescript/tests/tracing.test.ts:80
- Draft comment:
Review the use of console.log statements in test callbacks; consider removing or reducing them to avoid noisy test output. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
5. integ-tests/typescript/tests/dynamic-clients.test.ts:12
- Draft comment:
Typographical note: The variable name 'capitol' on this line might be a typo. If you intended to refer to the city's capital or a metaphor for something else, consider renaming it to 'capital' to avoid confusion. - 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% Without more context about what ExpectFailure returns or what this test is actually checking for, we can't be certain that 'capitol' is wrong. 'Capitol' is a valid English word (meaning a government building), and this could be intentional. The variable name choice doesn't impact functionality, and this seems like a nitpicky comment. The spelling could actually be intentional - maybe the test is specifically checking for something related to a capitol building in London? Even if it is a typo, this kind of minor variable naming issue in a test file doesn't warrant a comment - it doesn't affect functionality or code quality significantly. Delete this comment. It's speculative, not clearly wrong, and focuses on an unimportant detail in test code.
Workflow ID: wflow_cP1srfHaYjJhSZ8f
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
@@ -38,6 +38,11 @@ describe('OpenAI Provider', () => { | |||
}) | |||
|
|||
describe('Streaming', () => { | |||
it.only('should support OpenAI shorthand streaming', async () => { |
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.
Remove the it.only
in the OpenAI shorthand streaming test to ensure all tests run.
it.only('should support OpenAI shorthand streaming', async () => { | |
it('should support OpenAI shorthand streaming', async () => { |
expect(final.length).toBeGreaterThan(0) | ||
}) | ||
|
||
// it('should fail if azure is not configured streaming', async () => { |
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.
Consider removing the commented-out streaming failure test if it's no longer needed to keep the codebase clean.
Final PR after #1593 & #1639
Important
Add comprehensive integration tests for dynamic clients, types, media, providers, retry policies, and tracing, and remove outdated test file.
dynamic-clients.test.ts
for testing dynamic client registration and usage.dynamic-types.test.ts
for testing dynamic type handling, including classes, enums, and literals.media.test.ts
for testing image and audio input handling.prompt_renderer.test.ts
for testing prompt rendering with aliasing.retry.test.ts
for testing retry policies and fallbacks.tracing.test.ts
for testing synchronous and asynchronous tracing, and event logging.Anthropic
,AWS
,Azure
, andOpenAI
providers, including streaming and error handling.integ-tests.test.ts.old
, consolidating tests into new files.This description was created by
for ae7bc16. You can customize this summary. It will automatically update as commits are pushed.