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

test: organize and add tests #377

Merged
merged 3 commits into from
Feb 4, 2023
Merged

Conversation

AntoineRR
Copy link
Collaborator

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:

  • Add a utils.py file that helps with requesting endpoints, and checks the status code and global headers of the response.
  • Test everything with sync and async endpoints
  • Add tests for adding a global header, startup handler and shutdown handler, and a test for modifying request headers with middlewares (which is skipped for now as this is currently bugged in Robyn)
  • Rename endpoints for a better understanding (sync endpoints start with /sync and async one with /async for example)
  • Fix some docstrings

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.

@netlify
Copy link

netlify bot commented Jan 28, 2023

Deploy Preview for robyn canceled.

Name Link
🔨 Latest commit 83eeeaf
🔍 Latest deploy log https://app.netlify.com/sites/robyn/deploys/63de727eb76e4600097e843b

@AntoineRR AntoineRR force-pushed the test-rework branch 3 times, most recently from f4345f1 to 7284d58 Compare February 1, 2023 19:29
@sansyrox
Copy link
Member

sansyrox commented Feb 1, 2023

Hey @AntoineRR 👋

Thanks for the PR - the pre-commit.ci - pr is failing. I will do a thorough review in 20 minutes.

@AntoineRR
Copy link
Collaborator Author

Oh I didn't notice the precommit failing, I will fix that thanks

integration_tests/utils.py Outdated Show resolved Hide resolved
from utils import get


@pytest.mark.skip(reason="Fix middleware request headers modification")
Copy link
Member

Choose a reason for hiding this comment

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

This is broken?

Copy link
Collaborator Author

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!

Copy link
Member

@sansyrox sansyrox left a 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 😄

@AntoineRR
Copy link
Collaborator Author

Thanks for the review ! 😉 I will go through it and let you know when I did the changes

@AntoineRR AntoineRR force-pushed the test-rework branch 2 times, most recently from bd38ea4 to 304a57d Compare February 2, 2023 00:06
@AntoineRR
Copy link
Collaborator Author

PR updated @sansyrox 🙂

Copy link
Member

@sansyrox sansyrox left a 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.

integration_tests/test_basic_routes.py Show resolved Hide resolved
integration_tests/test_get_requests.py Outdated Show resolved Hide resolved
Copy link
Member

@sansyrox sansyrox left a 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! 😄

@sansyrox sansyrox merged commit 322edd4 into sparckles:main Feb 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants