Introduce connect client tests #507
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This builds off of the tests that asserted adlfs user agents are set for SDK clients. Even though the user agent tests added some coverage to how adlfs connected to SDK coverage, the adlfs test suite lacked coverage for how adlfs configurations were proxied to SDK clients (e.g., which account url to use and what credentials to set). By having these tests, it will make it easier to correctly refactorings related to client creation and easier to add support new SDK client configurations in the future. In addition to add these tests, a bug was discovered where for SAS token authentication on the file-like object, it would not respect location mode, while the file system did. The logic in the file-like object was updated to be consistent with files system logic.
adlfs/tests/test_connect_client.py
Outdated
| call_kwargs["credential"] = credential | ||
| if location_mode is not None: | ||
| call_kwargs["_location_mode"] = location_mode | ||
| if user_agent is not None: |
There was a problem hiding this comment.
If the user agent is never none, does there need to be an if statement?
There was a problem hiding this comment.
That's a good callout. I mainly did it out of symmetry with the other kwargs and we could leverage that if we ever add the ability to update the user agent in adlfs. But we don't today so we can always set it in the expected call.
anjaliratnam-msft
approved these changes
Jul 24, 2025
|
@martindurant this one should be good to review as well. It is mainly adding test cases around client creation and fixing an edge case that was found because of the new tests. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This builds off of the tests in this PR: #501 that asserted adlfs user agents are set for SDK clients. Even though the user agent tests added some coverage to how adlfs connected to SDK coverage, the adlfs test suite lacked coverage for how adlfs configurations were proxied to SDK clients (e.g., which account url to use and what credentials to set). By having these tests, it will make it easier to correctly do refactorings related to client creation (e.g., this PR: #504) and easier to add support new SDK client configurations in the future.
In addition to add these tests, a bug was discovered where for SAS token authentication on the file-like object, it would not respect location mode, while the file system did. The logic in the file-like object was updated to be consistent with files system logic since SAS tokens can be reused for secondary endpoints.