-
Notifications
You must be signed in to change notification settings - Fork 24
feat: adding support for request validation #304
Conversation
0a83327
to
4b30130
Compare
Codecov Report
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
|
44550aa
to
1dcabd5
Compare
There was a problem hiding this 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 👍
openapi_tester/schema_tester.py
Outdated
@@ -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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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 :)
8c849b9
to
a3430a1
Compare
a3430a1
to
f679dee
Compare
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! |
Sounds good @maticardenas 👍 Sorry for not giving this feedback earlier |
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
validate_request
) and extract the correspondingrequestBody
schema section from the OpenAPI schema (get_request_body_schema_section
)test_project
to also exposePets
API, as it is the OpenAPI schema we have in tests.Notes
openapi
schemas (noswagger
, previous versions), which involvesStaticSchemaLoader
andUrlStaticSchemaLoader
instantiated with anopenapi 3.x.x
. This because it's the current requirement I have for my project.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
swagger
,DrfSpectacularSchemaLoader
andDrfYasgSchemaLoader
.query
params,headers
,authentication
, etc.