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

Testing client #1562

Merged
merged 1 commit into from May 2, 2019
Merged

Testing client #1562

merged 1 commit into from May 2, 2019

Conversation

ahopkins
Copy link
Member

New test_client based upon request-async for usage with #1475

Copy link
Member

@yunstanford yunstanford left a comment

Choose a reason for hiding this comment

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

Looks like request-async only supports Python 3.6+.

We'll have to drop Py3.5 support if merging this.

@ahopkins
Copy link
Member Author

Yeah, I saw that last night after I saw the first test fail. I was thinking of taking a look at it and then hoping that (1) it was a simple fix like F strings and (2) @tomchristie would accept a PR to make it 3.5. If not, then we @huge-success/sanic-core-devs have a decision to make.

@ahopkins ahopkins changed the title Testing client WIP: Testing client Apr 24, 2019
@tomchristie
Copy link
Contributor

Great work @ahopkins!

The biggest 3.5 blocker will be async generators syntax. https://www.python.org/dev/peps/pep-0525/

async for item in ...:

It looks to me like sanic doens't yet use this syntax in any of it's API, since I can't see any (older style). __aiter__ or __anext__, and since the streaming request/response docs use loop style APIs rather than async iterator styles.

It'd be a bit of a pain to backport to 3.5 for those. It's feasible, but it'd likely be something that I'd still want to drop as soon as possible.

Personally I'd probably advocate for this being a good time to consider a version with 3.6+ support, but that's for you folks to take a call on.

@yunstanford
Copy link
Member

yunstanford commented Apr 24, 2019

I'm fine with dropping py3.5 support, but probably should announce that first or shows deprecation time.

@ahopkins
Copy link
Member Author

ahopkins commented Apr 24, 2019

I too would be in favor of dropping 3.5, but it does seem to account for a decent portion of downloads.

This is based off stats from 2019-01-01 thru today.

python_version downloads  
3.6 102,431 41%
3.7 77,564 31%
3.5 42,955 17%
null 25,451 10%
2.7 2,426 1%
3.4 247 0%
3.8 49 0%
2.6 3 0%
TOTAL 251,126  
SELECT
  REGEXP_EXTRACT(details.python, r"([0-9]+\.[0-9]+)") AS python_version,
  COUNT(*) AS downloads
FROM TABLE_DATE_RANGE(
  [the-psf:pypi.downloads],
  TIMESTAMP("2019-01-01"),
  CURRENT_TIMESTAMP()
)
WHERE file.project="sanic"
GROUP BY python_version
ORDER BY downloads DESC

@tomchristie
Copy link
Contributor

Are you able to limit the results to only include downloads against the current sanic release?

That'd be more indicative of what Python versions are typically being used by new projects (rather than including deployments that may be pinned to older versions anyways).

@tomchristie
Copy link
Contributor

(I'm arguing backwards there, because I think it'd be a good version jump time anyways, but hey. 😆)

@ahopkins
Copy link
Member Author

ahopkins commented Apr 24, 2019

When I filter for 18.12.0 and 19.3.1, it does drop from 17% to 9% You are right.

python_version downloads  
3.7 57852 40%
3.6 55204 38%
null 17201 12%
3.5 12368 9%
2.7 1779 1%
3.4 151 0%
3.8 48 0%
2.6 2 0%
TOTAL 144,605  
SELECT
  REGEXP_EXTRACT(details.python, r"([0-9]+\.[0-9]+)") AS python_version,
  COUNT(*) AS downloads
FROM TABLE_DATE_RANGE(
  [the-psf:pypi.downloads],
  TIMESTAMP("2019-01-01"),
  CURRENT_TIMESTAMP()
)
WHERE file.project="sanic"
AND (file.version="18.12.0" OR file.version="19.3.1")
GROUP BY python_version
ORDER BY downloads DESC

@ahopkins
Copy link
Member Author

@andreymal
Copy link
Contributor

Debian stable don't have py3.6+ yet, so I think this is not a good idea to drop py3.5

@ahopkins
Copy link
Member Author

@andreymal The current direction we are leaning towards is to drop 3.5 going forward, but extending our 18.12LTS support thru the 3.5 end of life in September 2020.

@codecov
Copy link

codecov bot commented Apr 28, 2019

Codecov Report

Merging #1562 into master will decrease coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1562      +/-   ##
==========================================
- Coverage    91.4%   91.31%   -0.09%     
==========================================
  Files          18       18              
  Lines        1804     1820      +16     
  Branches      344      347       +3     
==========================================
+ Hits         1649     1662      +13     
- Misses        131      132       +1     
- Partials       24       26       +2
Impacted Files Coverage Δ
sanic/testing.py 96.03% <100%> (+0.74%) ⬆️
sanic/request.py 97.36% <0%> (-1.32%) ⬇️
sanic/router.py 95.43% <0%> (-0.46%) ⬇️
sanic/app.py 92.47% <0%> (+0.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a4a3f6...ccd4c96. Read the comment docs.

Copy link
Member

@yunstanford yunstanford left a comment

Choose a reason for hiding this comment

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

LGTM, but let’s squash commits and add concrete commit msg, so that it helps generate changelog.

@seemethere
Copy link
Member

Agree with @yunstanford, let's squash these commits then it's a LGTM for me as well.

@ahopkins
Copy link
Member Author

ahopkins commented Apr 30, 2019

Will do. But there are a few more changes I want to make. Some tests use aiohttp directly and need to be refactored or else we will need both libraries, which seems bloated to me. Still WIP for current moment.

I'll finish shortly.

…op Python 3.5

Update all tests to be compatible with requests-async
Cleanup testing client changes with black and isort
Remove Python 3.5 and other meta doc cleanup
rename pyproject and fix pep517 error
Add black config to tox.ini
Cleanup tests and remove aiohttp
tox.ini change for easier development commands
Remove aiohttp from changelog and requirements
Cleanup imports and Makefile
@ahopkins ahopkins changed the title WIP: Testing client Testing client Apr 30, 2019
@ahopkins
Copy link
Member Author

ahopkins commented Apr 30, 2019

@yunstanford and @seemethere This should be ready to roll assuming all of the tests pass. 🤞

@ahopkins
Copy link
Member Author

FYI - To help you in your review since everything is squashed now...

The files that changed since the last time are tests/test_keep_alive_timeout.py and tests/test_request_timeout.py, with a minor change to sanic/testing.py.

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.

None yet

5 participants