-
-
Notifications
You must be signed in to change notification settings - Fork 258
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
test: organize and add tests #377
Conversation
✅ Deploy Preview for robyn canceled.
|
f4345f1
to
7284d58
Compare
Hey @AntoineRR 👋 Thanks for the PR - the |
Oh I didn't notice the precommit failing, I will fix that thanks |
7284d58
to
49da0a2
Compare
from utils import get | ||
|
||
|
||
@pytest.mark.skip(reason="Fix middleware request headers modification") |
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.
This is broken?
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.
Yes, I started investigating it but I'm actually waiting for PR #375 to be merged because I think its new struct will help as we could make Request
a pyclass and pass its instance to the middlewares (and routes) instead of a hashmap, and that should help fix this. We could also manipulate a true Request
object in the routes and misslewares instead of a raw dict, which would be much nicer!
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.
Amazing work @AntoineRR ! 🔥 This PR must've required a lot of work.
I have a few questions and suggestions. Let me know what you think of them 😄
Thanks for the review ! 😉 I will go through it and let you know when I did the changes |
bd38ea4
to
304a57d
Compare
PR updated @sansyrox 🙂 |
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.
@AntoineRR , all looks good! 😄
I just have one final comment.
304a57d
to
83eeeaf
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.
Perfect. Looks good to me. Great work! 😄
Description
I felt like the tests needed to be better organized, so it would be easier to write new tests and to check the unit test coverage. This PR does the following to achieve those goals:
utils.py
file that helps with requesting endpoints, and checks the status code and global headers of the response./sync
and async one with/async
for example)There is still room for improvements, like actually testing if the startup and shutdown handlers are executed, but I didn't figure out a way to test this properly for now.