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 coverage -> 100% #102

Closed
tomchristie opened this issue Jun 29, 2018 · 10 comments · Fixed by #981 or #2394
Closed

Test coverage -> 100% #102

tomchristie opened this issue Jun 29, 2018 · 10 comments · Fixed by #981 or #2394

Comments

@tomchristie
Copy link
Member

tomchristie commented Jun 29, 2018

Yes please

Edit by @Kludex:
Report created on 12 July 2021:

Name Missing PR
tests/test_config.py 12 #1348
uvicorn/_handlers/http.py 63-72, 83
uvicorn/config.py 15, 24-28, 250, 393-395, 406-408, 434-435, 441-443
uvicorn/main.py 39-48, 411-431, 435 #1154
uvicorn/middleware/proxy_headers.py 44 #1349
uvicorn/protocols/http/flow_control.py 25, 38-40, 44-45
uvicorn/protocols/http/h11_impl.py 124-125, 239-240, 260-261, 313, 319, 417, 420, 435
uvicorn/protocols/http/httptools_impl.py 124-125, 152-153, 157, 315, 321, 419, 422, 454, 456
uvicorn/protocols/utils.py 14-17 #1132
uvicorn/protocols/websockets/websockets_impl.py 100-101
uvicorn/protocols/websockets/wsproto_impl.py 25, 75, 88, 98, 111, 113, 123, 129, 132-135, 167-178, 216-223
uvicorn/server.py 27-28, 64-66, 85, 89-91, 96-97, 112-128, 132-138, 142-151, 163-166, 185-186, 192, 205, 248-250, 254, 266, 277-280, 284-287, 296, 310-313
uvicorn/subprocess.py 69-76 #1122
uvicorn/supervisors/basereload.py 43-48, 86
uvicorn/supervisors/watchgodreload.py 16, 59
TOTAL 96%

Important

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@tomchristie tomchristie added this to the Version 1.0 milestone Jul 25, 2018
@Jitsusama
Copy link
Contributor

Hello @tomchristie.

I'm a big believer in testing all the things, and I might be willing to help poke at this. My question is what style of testing are you hoping to adopt? End-to-end style functional testing, unit testing with mocks, unit testing against state, integration testing at certain strategic points, or some cross-pollination of all of the above?

I personally like testing against features. I have started to lean towards having a cross-cutting functional test for a happy path of a feature, then performing integration testing through third party libraries, and unit testing internally, against state when the final outcome doesn't involve a third party library, or against a mock of an adapter when a third party library is involved, but I'm open to whatever style you're most comfortable with.

@tomchristie
Copy link
Member Author

I'd just be pragmatic here.

  • http & ws protocols - there's a handful of uncovered lines here at the moment. The existing tests exercise the protocol classes, and assert that the responses on the wire are correct - any new tests there should follow in the same line.
  • config - There's a bunch of untested lines here - I'd just keep it really simple and test the Config class in isolation. Instantiate it, call load() or something if needed, assert that properties on it are as expected.
  • main & workers - You'll probably need multi-threading or multi-process tests against these, actually checking that you're able to make a real HTTP request against the server. First steps would be just 1. making a single request when running main. and 2. making a single request when running with gunicorn. (Not sure what you'd need to do to tackle that one)

@Kludex
Copy link
Member

Kludex commented Mar 7, 2021

I'll be working on this.

@Kludex
Copy link
Member

Kludex commented Mar 7, 2021

Do we want to add https://about.codecov.io/ ?

@euri10
Copy link
Member

euri10 commented Mar 8, 2021 via email

@Kludex
Copy link
Member

Kludex commented Mar 8, 2021

Looks like my description on the PR was a bad one 😅 it closed the issue. Sorry about that.

@euri10 euri10 reopened this Mar 8, 2021
@Kludex
Copy link
Member

Kludex commented Mar 11, 2021

We need to think about a strategy when we have multiple Python versions, the usual way is to have the coverage merged between all the environment tests, so if line x is covered by Python 3.6, then it means it's already covered. An example:

if sys.version_info < (3, 8):
    from typing_extensions import Literal, TypedDict  # A
else:
    from typing import Literal, TypedDict  # B

To achieve 100% coverage it's impossible, because line A will never be covered by Python >= 3.8 and line B will never be covered by Python < 3.8.

But in theory, if Python 3.6 covers line A, and Python 3.8 covers line B, then we have 100% coverage.

Suggestions:

  1. Use codecov. Setup is easy. I understand is not on the other encode projects, but afaik, there's not many "environment possibilities" on the other projects (correct me if I'm wrong).
  2. Save coverage data at each test environment and merge then together in a newly created step (job) called coverage, on which the coverage for uvicorn is calculated.
  3. Just # pragma: no cover those lines.

@tomchristie
Copy link
Member Author

We handle environment-specific coverage in REST framework by having a compat.py that's exempt from coverage.

Tho I'm also okay with no cover'ing lines in envrionment specific cases.

I probably wouldn't go for route (1) or (2) since they're more heavyweight tooling approaches.

@Kludex
Copy link
Member

Kludex commented Mar 12, 2021

On my quick look, I didn't understand how compat solves that problem exactly. I'm going to check more later.

But if no cover is fine, what I think we should do:

  • Choose an environment to be the one that should have the less "no coverings" (which I assume can be Python 3.8).
  • no cover everything that is needed to match the same percentage on all the environments.
  • Pin the --fail-under to this new percentage, so we can only upgrade this number.

I'm going to open a PR with this today. Hope I'm not missing anything 😅

@Kludex
Copy link
Member

Kludex commented Jul 10, 2021

I'm going to focus on the coverage project now, as we're almost finished with the mypy one.

I think we can make use of this coverage plugin to ignore the right environments on the tests and don't over ignore tests. Likewise, I'm going to see if we can really make use of it.

humrochagf added a commit to humrochagf/uvicorn that referenced this issue Aug 15, 2021
- Mostly test main as a cli client
- Test main functions directly when it's impossible to do is as a cli client (i.e. app non string case)
- Add no cover to the module entrypoint main call

Related to: encode#102
Kludex pushed a commit that referenced this issue Aug 16, 2021
- Mostly test main as a cli client
- Test main functions directly when it's impossible to do is as a cli client (i.e. app non string case)
- Add no cover to the module entrypoint main call

Related to: #102
@Kludex Kludex unpinned this issue Sep 22, 2021
@Kludex Kludex pinned this issue Sep 22, 2021
@Kludex Kludex mentioned this issue Oct 7, 2021
2 tasks
@abersheeran abersheeran unpinned this issue Nov 12, 2021
@abersheeran abersheeran pinned this issue Nov 12, 2021
Kludex pushed a commit that referenced this issue Nov 17, 2021
- Mostly test main as a cli client
- Test main functions directly when it's impossible to do is as a cli client (i.e. app non string case)
- Add no cover to the module entrypoint main call

Related to: #102
@tomchristie tomchristie unpinned this issue Feb 10, 2022
Kludex pushed a commit to sephioh/uvicorn that referenced this issue Oct 29, 2022
- Mostly test main as a cli client
- Test main functions directly when it's impossible to do is as a cli client (i.e. app non string case)
- Add no cover to the module entrypoint main call

Related to: encode#102
@br3ndonland br3ndonland mentioned this issue Jun 4, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants