-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Tests for MultipartFormDataHelper and OpenAISyncClientTest #44811
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
base: main
Are you sure you want to change the base?
Conversation
+Tests for OpenAISyncClientTest - ChatCompletionsStreamOptions
Thank you for your contribution @gregocmsft! We will review the pull request and get back to you soon. |
@gregocmsft please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
API change check API changes are not detected in this pull request. |
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.
This is great! Thank you. I think you just need to add the analogous tests I mentioned in the PR comment and you are good to go!
@ParameterizedTest(name = DISPLAY_NAME_WITH_ARGUMENTS) | ||
@MethodSource("com.azure.ai.openai.TestUtils#getTestParameters") | ||
public void testGetCompletionsStreamChatCompletionStreamOptions(HttpClient httpClient, | ||
OpenAIServiceVersion serviceVersion) { | ||
client = getOpenAIClient(httpClient, serviceVersion); | ||
getCompletionsRunner((deploymentId, prompt) -> { | ||
IterableStream<Completions> resultCompletions = client.getCompletionsStream(deploymentId, | ||
new CompletionsOptions(prompt), new ChatCompletionStreamOptions().setIncludeUsage(true)); | ||
|
||
Object[] result = resultCompletions.stream().toArray(); | ||
Completions[] completionsArray = Arrays.copyOf(result, result.length, Completions[].class); | ||
|
||
assertTrue(completionsArray.length > 1); | ||
// First element returns the prompt filter results (no output tokens are present) | ||
assertFalse(CoreUtils.isNullOrEmpty(completionsArray[0].getPromptFilterResults())); | ||
// Choices (output tokens) are present in all the elements in between | ||
for (int i = 1; i < completionsArray.length - 2; i++) { | ||
assertCompletionsStream(completionsArray[i]); | ||
} | ||
|
||
// Last element returns the completion tokens (no output tokens are present) | ||
assertNotNull(completionsArray[completionsArray.length - 1].getUsage()); | ||
}); | ||
} | ||
|
||
@ParameterizedTest(name = DISPLAY_NAME_WITH_ARGUMENTS) | ||
@MethodSource("com.azure.ai.openai.TestUtils#getTestParameters") | ||
public void testGetChatCompletionsStreamChatCompletionStreamOptionsUsageTrue(HttpClient httpClient, | ||
OpenAIServiceVersion serviceVersion) { | ||
client = getOpenAIClient(httpClient, serviceVersion); | ||
getChatCompletionsStreamUsageRunner((deploymentId, chatCompletionsOptions) -> { | ||
IterableStream<ChatCompletions> resultChatCompletions = client.getChatCompletionsStream(deploymentId, | ||
chatCompletionsOptions, new ChatCompletionStreamOptions().setIncludeUsage(true)); | ||
assertChatCompletionStreamUsage(resultChatCompletions.stream().collect(Collectors.toList())); | ||
}); | ||
} | ||
|
||
@ParameterizedTest(name = DISPLAY_NAME_WITH_ARGUMENTS) | ||
@MethodSource("com.azure.ai.openai.TestUtils#getTestParameters") | ||
public void testGetChatCompletionsStreamChatCompletionStreamOptionsUsageFalse(HttpClient httpClient, | ||
OpenAIServiceVersion serviceVersion) { | ||
client = getOpenAIClient(httpClient, serviceVersion); | ||
getChatCompletionsRunner((deploymentId, chatMessages) -> { | ||
IterableStream<ChatCompletions> resultChatCompletions = client.getChatCompletionsStream(deploymentId, | ||
new ChatCompletionsOptions(chatMessages), new ChatCompletionStreamOptions().setIncludeUsage(false)); | ||
assertTrue(resultChatCompletions.stream().toArray().length > 1); | ||
resultChatCompletions.forEach(OpenAIClientTestBase::assertChatCompletionsStream); | ||
}); | ||
} |
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.
Good stuff! I think that all these tests are good. The only thing left to do would be to write the version for the NonAzureOpenAISyncClientTest
(which is just copy and pasting this, adjusting the client
initialization. After that, adding the async version for both Azure and Non-Azure and you are good to go!
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.
Hi! I'm not sure how to generate the versions since I have not used any client initialization for these unit tests. I only worked with the tools of MultipartFormDataHelper but the tests do not interact with the API so no client initialization was needed.
Edit: I see what you're saying but I'm not sure why these tests in particular are here since I did not develop them. I will verify with the team what could happen.
The CI failures I see are mostly related to the test recording being missing. Once you've added the other tests, give me a heads up and I will add the recording to this PR so we can move it forward. I think you might also have style check issues, although if you are able to run |
+Tests for MultipartFormDataHelper
+Tests for OpenAISyncClientTest - ChatCompletionsStreamOptions