Skip to content

Increase code coverage for Open AI Client Builder#44790

Closed
v-lrdev wants to merge 5 commits intoAzure:mainfrom
markmunozmsft:openai-client-builder-tests
Closed

Increase code coverage for Open AI Client Builder#44790
v-lrdev wants to merge 5 commits intoAzure:mainfrom
markmunozmsft:openai-client-builder-tests

Conversation

@v-lrdev
Copy link
Contributor

@v-lrdev v-lrdev commented Mar 25, 2025

Description

This PR adds unit tests for the OpenAIClientBuilder class to verify the correct behavior of its methods, including:

  • Building clients with different credentials and configurations.
  • Setting and verifying various options and policies.
  • Ensuring type-safe handling of HttpPipelinePolicy lists.

Changes

  • Added unit tests for OpenAIClientBuilder methods.
  • Used reflection to access and verify private fields.
  • Ensured type-safe handling of HttpPipelinePolicy lists.

@github-actions github-actions bot added Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Mar 25, 2025
@github-actions
Copy link
Contributor

Thank you for your contribution @v-lrdev! We will review the pull request and get back to you soon.

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

Copy link
Member

@jpalvarezl jpalvarezl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. There is a couple of comments, but I would defer to @mssfang to provide guidance here in terms of whether we should go as far as using reflections to see that we are setting the right objects in the builder. Thank you!

import com.azure.core.credential.AccessToken;
import com.azure.core.credential.AzureKeyCredential;
import com.azure.core.credential.TokenCredential;
import com.azure.core.http.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you will need to expand the star imports, as the style check script will probably not like it

assertNotNull(client, "The client should not be null");

// Access private fields using reflection
Field endpointField = OpenAIClientBuilder.class.getDeclaredField("endpoint");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to be a bit more careful here, most of the methods and member variables in OpenAIClientBuilder are generated so they are not succeptible to change much, I think. But in the given case that endpoint no longer exists as such, what would be the behaviour of this test? Is this when NoSuchFieldException is thrown?

.collect(Collectors.toList());

assertTrue(pipelinePolicies.contains(customPolicy));
} catch (NoSuchFieldException | IllegalAccessException e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's best if we move these to be thrown in the method signature, like you've done for the other tests.

@v-lrdev v-lrdev closed this Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. OpenAI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants