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

Conversation

Projects
None yet
5 participants
@ahopkins
Copy link
Member

commented Apr 23, 2019

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

@yunstanford
Copy link
Member

left a comment

Looks like request-async only supports Python 3.6+.

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

@ahopkins

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

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

@ahopkins

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

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

@ahopkins

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

@andreymal

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

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

@ahopkins

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2019

@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

This comment has been minimized.

Copy link

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.

@yunstanford
Copy link
Member

left a comment

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

@seemethere

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

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

@ahopkins

This comment has been minimized.

Copy link
Member Author

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.

Create requests-async based TestClient, remove aiohttp dependency, dr…
…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 force-pushed the testing-client branch from 84c8344 to ccd4c96 Apr 30, 2019

@ahopkins ahopkins changed the title WIP: Testing client Testing client Apr 30, 2019

@ahopkins

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

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

@ahopkins

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

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.

@yunstanford yunstanford merged commit ae2b8f0 into master May 2, 2019

4 checks passed

codecov/patch 100% of diff hit (target 91.4%)
Details
codecov/project Absolute coverage decreased by -0.08% but relative coverage increased by +8.59% compared to 6a4a3f6
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ahopkins ahopkins deleted the testing-client branch May 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.