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.

Sign up for GitHub

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

Jump to bottom

Config coverage increase #626

Merged
merged 29 commits into from
Apr 23, 2020
Merged

Config coverage increase #626

merged 29 commits into from
Apr 23, 2020

Conversation

euri10
Copy link
Member

@euri10 euri10 commented Apr 11, 2020 •
edited
Loading

Little steps towards #102

Added tests to increase config.py coverage (a little bit of main.py too), went from 81 to 99%.

Incidentally when I added the ini log config file in that commit (aff30ae) it broke the http protocol logs tests here and it looks the fix from #512 is necessary or no logs are sent indeed.

Also trying to understand the specifics of <3.7 for loggers it appears to me the check is not necessary anymore (last commit 32b2c72)

@euri10 euri10 marked this pull request as draft April 11, 2020 15:30
@euri10 euri10 marked this pull request as ready for review April 11, 2020 15:46
@euri10 euri10 marked this pull request as draft April 11, 2020 19:26
@euri10 euri10 marked this pull request as ready for review April 12, 2020 04:09
@euri10 euri10 changed the title WIP: config coverage increase Config coverage increase Apr 12, 2020
@euri10 euri10 changed the title Config coverage increase WIP Config coverage increase Apr 12, 2020
@euri10 euri10 marked this pull request as draft April 12, 2020 08:01
@euri10
Copy link
Member Author

euri10 commented Apr 12, 2020

need to get why pypy timeout

@euri10 euri10 marked this pull request as ready for review April 12, 2020 08:32
@euri10 euri10 changed the title WIP Config coverage increase Config coverage increase Apr 12, 2020
tomchristie
tomchristie approved these changes Apr 21, 2020
View reviewed changes
Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

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

Seems okay, yup.

I'd generally suggest that we try to keep PRs split up wherever ossible (eg. the change in the disable_existing_loggers=False seems unrelated to the coverage, as does the sys.version_info < (3,7) case. Having said that I don't think we should let those block this from progressing.

Looks like there's some conflicts that'll need resolving, but otherwise, great - let's move on with this! 😃

@euri10
Added test for env_file config to increase coverage
87ad889
@euri10
Added test for reload_dirs
f3ddf6e 
flake8
@euri10
Added test for workers set with env vars
c89ba6c
@euri10
Added test for forwarded_allow_ips
f640b07
@euri10
Added test for use_colors
36b6360
@euri10
Added test for log_config ini file
9a12c79
@euri10
Commenting that ini file config for now it breaks mass http tests !
e3f9fa9
@euri10
Seems like the fix in https://github.com/encode/uvicorn/pull/512/files …
ac361d6 
…is necessary
@euri10
Added log_level tests, as str or int
ef86999
@euri10
Added access_log set to False
d11ad7e
@euri10
Added test for failing app import string
31bf6fe
@euri10
Added test for should_reload property
5920492
@euri10
Added test for uds
34f686b
@euri10
Added optional python-dotenv
d3318a2
@euri10
Skipping uds tests on windows
3e0b500
@euri10
Added fd tests, not quite sure about those and uds though
c76bbd6
@euri10
Rebased against master
f6983fd
@euri10
Added oserror test when tring to use sockets on windows
1341496
@euri10
Test rebind socket oserror
1ec6388
@euri10
Copy link
Member Author

euri10 commented Apr 23, 2020

Seems okay, yup.

I'd generally suggest that we try to keep PRs split up wherever ossible (eg. the change in the disable_existing_loggers=False seems unrelated to the coverage, as does the sys.version_info < (3,7) case. Having said that I don't think we should let those block this from progressing.

Looks like there's some conflicts that'll need resolving, but otherwise, great - let's move on with this!

ok I rebased it and all tests pass

I'm sorry for not separating them and will keep them seperated next time.

just one thing I'd like you to confirm,not sure you saw it, it's the change here in uvicorn/config.py

just asking because you seemed not ok about it in #511

@tomchristie
Copy link
Member

just one thing I'd like you to confirm, not sure you saw it, it's the change here in uvicorn/config.py

Well, ideally keep it separate from this PR.

I'm not sure, I'm not convinced about it, but we also get a lot of feedback around the behaviour there, so... 🤷‍♂️

@euri10 euri10 merged commit 2b93b84 into encode:master Apr 23, 2020
This was referenced Apr 23, 2020
Merged
Closed
Merged
tomchristie added a commit that referenced this pull request Apr 24, 2020
@tomchristie
Revert "Config coverage increase (#626)"
ffd8757 
This reverts commit 2b93b84.
@tomchristie tomchristie mentioned this pull request Apr 24, 2020
Merged
tomchristie added a commit that referenced this pull request Apr 24, 2020
@tomchristie
Revert "Config coverage increase" (#650)
e7f6006 
* Revert "Config coverage increase (#626)"

This reverts commit 2b93b84.

* Tests should capture output

* Drop flaky gunicorn test
@euri10 euri10 deleted the config_coverage branch August 31, 2020 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomchristie tomchristie tomchristie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@euri10 @tomchristie