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

python_examples: Validate error responses. #30418

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

Vector73
Copy link
Collaborator

This PR is a follow-up of #30115. It adds assert statements to validate the error response for the "400" error code. The current validation using validate_against_openapi_schema doesn't correctly validate "400" responses.

Fixes:

Screenshots and screen captures:

Self-review checklist
  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

@Vector73
Copy link
Collaborator Author

@laurynmm can you review this?

@Vector73 Vector73 changed the title python_examples: Validate error response. python_examples: Validate error responses. Jun 13, 2024
@Vector73 Vector73 force-pushed the validate-error-result branch 2 times, most recently from 5799756 to 541abdb Compare June 13, 2024 13:04
@timabbott
Copy link
Sponsor Member

The current validation using validate_against_openapi_schema doesn't correctly validate "400" responses.

In what way is that validation not correct? It's not obvious to me why the remedy should be to remove that vs., for example, checking both.

@Vector73
Copy link
Collaborator Author

As explained here, validate_against_openapi_schema will always return True for 400 responses. I can keep both checks for responses but this function will not currently validate the 400 ones.

Copy link
Collaborator

@laurynmm laurynmm left a comment

Choose a reason for hiding this comment

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

@Vector73 - Thanks for doing this follow-up! I had one small comment on the function name.

It might be good to add a TODO comment somewhere (maybe above this new function) about wanting to add back the validate_against_openapi_schema when that is updated to actually validate our error responses in the documentation.

Even if we don't delete those validate_against_openapi_schema checks for these error responses, I think adding this new check for the expected error code is useful. The schema check wouldn't confirm that anyway.

zerver/openapi/python_examples.py Outdated Show resolved Hide resolved
@timabbott
Copy link
Sponsor Member

I think it's better to keep the validate_against_openapi_schema statements even if they always succeed with the current OpenAPI spec; that may not be the case for the future as the spec changes, and I can easily see us forgetting to re-add validation.

@laurynmm
Copy link
Collaborator

My one concern is that it then looks like these documented error responses are being validated as part of this test, when we're really just skipping any validation for 4xx responses.

@timabbott
Copy link
Sponsor Member

My view is we should probably open an issue to track removing #30115 (comment), rather than removing infrastructure in other places; that seems like the simpler path to having complete validation down the line.

@laurynmm
Copy link
Collaborator

Okay. Makes sense.

@Vector73 - For this PR, the consensus is to not remove validate_against_openapi_schema for these error tests, but to add the new check on the error code returned.

We most recently talked about documenting error responses on CZO here: https://chat.zulip.org/#narrow/stream/412-api-documentation/topic/documenting.20error.20responses/near/1644535. It's a bit more complicated than the code comment in zerver/openapi/openapi.validate_test_response implies in that we've not yet updated our documentation generators to handle examples fields in the API documentation.

@Vector73
Copy link
Collaborator Author

@laurynmm I have updated this PR.

@laurynmm laurynmm added the integration review Added by maintainers when a PR may be ready for integration. label Jun 20, 2024
Adds assert statements to validate error response for "400" error code. The
current validation using `validate_against_openapi_schema` doesn't work for
"400" responses.

Adds separate functions to validate "200" and "400" responses and removes `validate_response_result`.
@zulipbot zulipbot added size: XL and removed size: S labels Jun 21, 2024
@timabbott timabbott merged commit 5230aad into zulip:main Jun 21, 2024
6 checks passed
@timabbott
Copy link
Sponsor Member

Looks great, merged, thanks @Vector73!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when a PR may be ready for integration. size: XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants