Skip to content
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] Fix broken azure_auth test #544

Merged
merged 8 commits into from
Nov 11, 2024
Merged

Conversation

jsong468
Copy link
Contributor

@jsong468 jsong468 commented Nov 11, 2024

This PR addresses the error that was thrown when trying to run the test_get_token_provider_from_default_azure_credential test. This error arose as a result of changes in azure-identity version 1.18 which introduced a get_token_info method which returns an AccessTokenInfo object. The get_token_info method is an alternative method to the previously existing get_token "that improves support for more complex authentication scenarios" and is the default for authentication aside from when the credential object does not have the get_token_info attribute. Thus, the unit tests now mock hasattr for 'get_token_info' arg.

This PR adds unit tests to use both the get_token and get_token_info methods when calling get_token_provider_from_default_azure_credential to connect to an AOAI endpoint.

Also, instances of Mock are changed to MagicMock.

Copy link
Contributor

@romanlutz romanlutz left a 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. @rdheekonda do you have thoughts on increasing the lower bound on azure-identity?

Copy link
Contributor

@nina-msft nina-msft left a comment

Choose a reason for hiding this comment

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

This lambda side-effect solution feels like magic to me 🪄

Clean fix, great work!

@rdheekonda
Copy link
Contributor

Looks good to me. @rdheekonda do you have thoughts on increasing the lower bound on azure-identity?

Thanks for the fix, @jsong468. Could you confirm that this works with latest version azure-identity 1.19.0?

@jsong468 jsong468 merged commit 6988eac into Azure:main Nov 11, 2024
6 checks passed
@jsong468 jsong468 deleted the 3345-FixAzureAuthTest branch November 11, 2024 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants