Skip to content

Conversation

@Lee-W
Copy link
Member

@Lee-W Lee-W commented Aug 24, 2023

As discussed in this stackoverflow thread, part of the lib does not support azure-identity well and need this helper class.

This PR is dependency of #33433, #33469, #33467

Add test case to signed_session which is the main function of this wrapper class


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@uranusjr
Copy link
Member

Is it possible to test this?

@Lee-W
Copy link
Member Author

Lee-W commented Aug 24, 2023

Is it possible to test this?

I've run it with #33433 and succeeded. As for unit testing, I'm not that sure. Even if I add some, they might be mocking without checking anything. Is there any suggestions?

@uranusjr
Copy link
Member

Not really, my impression is this wouldn’t be easy to test anyway, hence the question 😛

@potiuk
Copy link
Member

potiuk commented Aug 24, 2023

Is it possible to test this?
Not really, my impression is this wouldn’t be easy to test anyway, hence the question 😛

Yes. We often do a mock-only tests and It's also important. Mock is really a representation of "How the author imagined the code would interact with the external world". Since you tested it, and you have good understanding how it works. Coding that in the form of mock is capturing that knowledge.

Imagine somene else making a change to that code 2 months from now. Or even YOURSELF doing it 2 months from now (I always try to picture future-self coming back to the code after I completely forgot what it does). If there is no mock and someone changes the behaviour accidentally, they won't notice and it might stop working accidentally. But when it fails, you will look at your test, and you will be able to quickly recall the intent you had without heavy debugging.

I really treat those tests with mocks often as description of what I thought the class in question shoudl be doing - so that I even don't have to remember it.

@Lee-W
Copy link
Member Author

Lee-W commented Aug 24, 2023

Is it possible to test this?
Not really, my impression is this wouldn’t be easy to test anyway, hence the question 😛

Yes. We often do a mock-only tests and It's also important. Mock is really a representation of "How the author imagined the code would interact with the external world". Since you tested it, and you have good understanding how it works. Coding that in the form of mock is capturing that knowledge.

Imagine somene else making a change to that code 2 months from now. Or even YOURSELF doing it 2 months from now (I always try to picture future-self coming back to the code after I completely forgot what it does). If there is no mock and someone changes the behaviour accidentally, they won't notice and it might stop working accidentally. But when it fails, you will look at your test, and you will be able to quickly recall the intent you had without heavy debugging.

I really treat those tests with mocks often as description of what I thought the class in question shoudl be doing - so that I even don't have to remember it.

Got it. Thanks @potiuk for such a detailed response. I'll try to add some mock tests tomorrow

@Lee-W Lee-W requested a review from phanikumv August 25, 2023 07:37
…dapter.signed_session

signed_session is the main function of this wrapper class
@Lee-W Lee-W force-pushed the add-AzureIdentityCredentialAdapter-for-adding-DefaultAzureCredential-backward-compatibility branch from b11a15f to ecb4586 Compare August 25, 2023 16:11
@Lee-W
Copy link
Member Author

Lee-W commented Aug 25, 2023

Hi @potiuk @uranusjr , I tried to add a test in this PR. The original class is accidentally merged into the main branch in another PR. Could you please check whether such test looks good. Thanks!

@Lee-W Lee-W changed the title feat(provider/microsoft): add DefaultAzureCredential compatibility to azure-python-sdk through AzureIdentityCredentialAdapter wrapper test(providers/microsoft): add test case for AzureIdentityCredentialAdapter.signed_session Aug 25, 2023
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

LGTM

@potiuk potiuk merged commit 7a493ea into apache:main Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants