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

Add tests of with_metadata option #657

Merged
merged 3 commits into from
Sep 27, 2024
Merged

Conversation

sgillies
Copy link
Collaborator

@sgillies sgillies commented Sep 25, 2024

The low-level autogenerated API changes have already been merged. Wiring up the high-level API is straight forward. I've run the functions against my own groups and arrays and see what I expect. Note well: some of our arrays have very large metadata and listing them using with_metadata=True will appear to hang your computer. It's really just transferring a lot of bytes across the network.

I've added some new tests. They don't check the results, but do check that we make a request to the API endpoints with the right query params.

@sgillies sgillies added the enhancement New feature or request label Sep 25, 2024
@sgillies sgillies self-assigned this Sep 25, 2024


@pytest.fixture(scope="function")
def with_metadata_checker(monkeypatch):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This fixture monkeypatches the urllib3 pool manager to intercept outgoing requests. It's a lightweight alternative to setting up a proxy or some other more complicated software.

"""Intercept requests to arrays and groups list endpoints and checks the URLs."""

def fake_and_check_request(self, method, url, *args, **kwargs):
assert "with_metadata=True" in url
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's the actual test assertion, inside the fixture. If it fails, pytest does the right thing and reports an error that's quite understandable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ktsitsi @spencerseale @gspowley what do you think of this approach for testing our SDK - API interface? Is it one that you'd be comfortable using and maintaining with me?

Copy link
Collaborator

@spencerseale spencerseale Sep 25, 2024

Choose a reason for hiding this comment

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

So basically we mock the actual request and just ensure that whatever selection we're testing is passed into the request?

Copy link
Collaborator Author

@sgillies sgillies Sep 25, 2024

Choose a reason for hiding this comment

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

@spencerseale correct. Actual integration tests are good, too, but these new tests run really fast and could help eliminate a lot of lurking generic bugs living between the high level Python API and the low level auto-generated API.

HTTP is not the best communication protocol ever, but it is sure easy to peek into. I guess that's why we're still using it :)

Copy link
Member

Choose a reason for hiding this comment

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

I love the mocking! Great way to test and interate in the development and allow more comprehensive testing in the future without needing a full stack.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @sgillies, I will be sure to follow this strategy.

"""Intercept requests to arrays and groups list endpoints and checks the URLs."""

def fake_and_check_request(self, method, url, *args, **kwargs):
assert "with_metadata=True" in url
Copy link
Collaborator Author

@sgillies sgillies Sep 25, 2024

Choose a reason for hiding this comment

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

I honestly don't know which side, client or server, has the responsibility for getting the values of these boolean options right. The server accepts with_metadata=True but will it always do so?

Copy link
Member

Choose a reason for hiding this comment

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

I usually opt for the server to be more flexible but we should also standardize it in the openapi definition

@sgillies sgillies marked this pull request as ready for review September 25, 2024 21:34
@spencerseale
Copy link
Collaborator

I have a project that could use this new feature, will happily use it if it can go into next cloud-py release

@sgillies sgillies merged commit 9bef6bb into main Sep 27, 2024
18 checks passed
@sgillies sgillies deleted the sg/sc-53874/with-metadata-option branch September 27, 2024 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants