Skip to content
This repository has been archived by the owner on Nov 19, 2023. It is now read-only.

feat: adding support for request validation #304

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maticardenas
Copy link
Contributor

@maticardenas maticardenas commented Oct 11, 2023

issue: #291

Adding requestBody validation support.

Currently, the package offers the possibility of validating only the response, however, it is useful to also at the same time,e to have validated that the passed resquestBody in our functional tests are also properly documented and match the implementation.

Additions/Changes

  • Methods for validating request (validate_request) and extract the corresponding requestBody schema section from the OpenAPI schema (get_request_body_schema_section)
  • Updating test_project to also expose Pets API, as it is the OpenAPI schema we have in tests.
  • Adding tests for schema tester, clients and validators.
  • Updating affected variables and functions.

Notes

  • Adding support only for openapi schemas (no swagger, previous versions), which involves StaticSchemaLoader and UrlStaticSchemaLoader instantiated with an openapi 3.x.x. This because it's the current requirement I have for my project.
  • Decided to make request validation only when the scenarios has a successful response. Why? In the case you provide for example an incomplete payload, it will always fail naturally when comparing when your schema - in this situations we should let the functional case validate that the test we chose doesn't fit our design.

Follow Ups

  • Add support for request bodies for swagger, DrfSpectacularSchemaLoader and DrfYasgSchemaLoader.
  • Add support maybe for checking more aspects of the request? query params, headers, authentication, etc.

@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Merging #304 (f679dee) into master (08d9449) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #304     +/-   ##
========================================
+ Coverage    98.6%   98.7%   +0.1%     
========================================
  Files           9       9             
  Lines         531     578     +47     
  Branches       99     108      +9     
========================================
+ Hits          524     571     +47     
  Misses          4       4             
  Partials        3       3             
Files Coverage Δ
openapi_tester/clients.py 100.0% <100.0%> (ø)
openapi_tester/constants.py 100.0% <100.0%> (ø)
openapi_tester/loaders.py 95.9% <ø> (ø)
openapi_tester/schema_tester.py 99.5% <100.0%> (+0.1%) ⬆️

@maticardenas maticardenas force-pushed the 291-validate-requests-func branch 7 times, most recently from 44550aa to 1dcabd5 Compare October 12, 2023 07:49
Copy link
Member

@sondrelg sondrelg left a comment

Choose a reason for hiding this comment

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

Looks pretty good! Thanks for taking the time 👏

I have a few questions and comments, but happy to accept this in general 👍

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
openapi_tester/clients.py Outdated Show resolved Hide resolved
openapi_tester/clients.py Outdated Show resolved Hide resolved
openapi_tester/clients.py Outdated Show resolved Hide resolved
@@ -338,6 +397,7 @@ def test_openapi_object(
reference: str,
case_tester: Callable[[str], None] | None = None,
ignore_case: list[str] | None = None,
**kwargs: Any,
Copy link
Member

Choose a reason for hiding this comment

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

What's this for? Can we use explicit arguments instead?

Copy link
Contributor Author

@maticardenas maticardenas Oct 14, 2023

Choose a reason for hiding this comment

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

Not sure why I had left it like this, it can certainly be an explicit argument. Changed it, thanks!

Copy link
Contributor Author

@maticardenas maticardenas Oct 14, 2023

Choose a reason for hiding this comment

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

Ok, I think I see why I had probably left it like that. After adding one extra explicit argument pylint complains that they are now too many (7, while max. is 6).
In order to overcome this introduced a new OpenAPITestConfig dataclass and grouped the optional "test config" params within it, guess it could be improved but found it more clear for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, @sondrelg taking a look at this again I think this might be sort of a breaking change? Let me know if you'd like me to take a different approach there :)

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the radio silence here @maticardenas 🙇 I've had a busy few weeks. I'll do my best to get this reviewed by the end of the week!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sondrelg no problem at all! and thanks 🙏 , I appreciate the review :)

openapi_tester/schema_tester.py Show resolved Hide resolved
@maticardenas maticardenas force-pushed the 291-validate-requests-func branch 5 times, most recently from 8c849b9 to a3430a1 Compare October 14, 2023 19:44
@sondrelg
Copy link
Member

I think this looks pretty good @maticardenas and would probably accept the PR as it currently stands. That said, I've been considering archiving this project (re #303) for a while, since I haven't used it since 2019, and I think I will do that now instead of accepting this, unfortunately.

I'm happy to see someone is using it, and I would suggest you use your own fork of the project going forward. If you want to maintain the fork beyond this PR, I would be happy to link to it in the README here 👍 If you have any questions about package management or forks you need answered, just let me know and I'll be happy to help

@maticardenas
Copy link
Contributor Author

I think this looks pretty good @maticardenas and would probably accept the PR as it currently stands. That said, I've been considering archiving this project (re #303) for a while, since I haven't used it since 2019, and I think I will do that now instead of accepting this, unfortunately.

I'm happy to see someone is using it, and I would suggest you use your own fork of the project going forward. If you want to maintain the fork beyond this PR, I would be happy to link to it in the README here 👍 If you have any questions about package management or forks you need answered, just let me know and I'll be happy to help

@sondrelg thanks for the review, and unfortunate to hear that you are planning to archive the repository as I currently use it, but I can understand the reasons.

I have merged this same PR in my fork, and plan to maintain the fork beyond the PR (need to add github actions to it now though :D), so it would be nice the link in the README if possible. Thanks again!

@sondrelg
Copy link
Member

Sounds good @maticardenas 👍 Sorry for not giving this feedback earlier

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants