-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add support for Sanic 21+ #71
Conversation
@@ -0,0 +1,7 @@ | |||
# these are constrained by the files in tests/constraints/*.txt |
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.
sanic 20.* needs a separate requirements file to remove the sanic-testing
requirement.
r? @jwhitlock |
Codecov Report
@@ Coverage Diff @@
## main #71 +/- ##
==========================================
+ Coverage 97.80% 97.81% +0.01%
==========================================
Files 18 18
Lines 593 596 +3
Branches 97 98 +1
==========================================
+ Hits 580 583 +3
Misses 9 9
Partials 4 4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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 great, thanks @relud! I've got one ignorable suggestion. Please merge when ready.
|
||
|
||
@pytest.fixture(scope="function") | ||
def app(): | ||
app = Sanic("dockerflow") | ||
app = Sanic(f"dockerflow-{uuid.uuid4().hex}") |
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.
nice way to generate unique app IDs for tests!
yield test_client | ||
finally: | ||
s.close() | ||
return SanicTestClient(app) |
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.
Love to see code removed! 😍
@jwhitlock suggestion applied, needs your review again, and I don't know that i'll be able to merge it as I don't have write access to this repo. |
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.
Thanks! I've given you Write access, see if you can merge now.
fixes #70