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

✅ Add the docs_src directory to test coverage and update tests #1904

Merged
merged 14 commits into from
Jul 29, 2021

Conversation

Kludex
Copy link
Sponsor Collaborator

@Kludex Kludex commented Aug 16, 2020

I've just spotted a typo in the test script:

pytest --cov=fastapi --cov=tests --cov=docs/src --cov-report=term-missing --cov-report=xml tests ${@}

docs/src is a typo, we want to cover docs_src.

If you don't trust me :trollface: you can check at the end of the test:

Coverage.py warning: Module docs/src was never imported. (module-not-imported)

I'll be increasing the coverage for those tests on this PR.

Report on the missing coverage:
image

Changes:

  • Add :skip-covered in the terminal Pytest report. It makes it easy to read missing coverage.
  • Fix test script.
    • Remove --cov=tests from test script
    • Fix typo docs/src -> docs_src
  • Fix double test call.
    • If a function that starts with test is imported in the tests, there's no need for wrapping it. If you do so, it calls the test function twice.
  • Add missing tests to be covered.

After the fixes:
image

EDIT:
November 4, 2020: This PR is still useful. And I've added the coverage on the tests once again.
November 10, 2020: Rebase it, everything is fine!

@codecov
Copy link

codecov bot commented Aug 16, 2020

Codecov Report

Merging #1904 (66e9c7f) into master (4d208b2) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##            master     #1904     +/-   ##
===========================================
  Coverage   100.00%   100.00%             
===========================================
  Files          243       390    +147     
  Lines         7419      9747   +2328     
===========================================
+ Hits          7419      9747   +2328     
Impacted Files Coverage Δ
...est_tutorial/test_bigger_applications/test_main.py 100.00% <ø> (ø)
docs_src/settings/app02/main.py 100.00% <100.00%> (ø)
docs_src/settings/app02/test_main.py 100.00% <100.00%> (ø)
...orial/test_conditional_openapi/test_tutorial001.py 100.00% <100.00%> (ø)
tests/test_tutorial/test_settings/test_app02.py 100.00% <100.00%> (ø)
tests/test_tutorial/test_testing/test_main_b.py 100.00% <100.00%> (ø)
.../test_tutorial/test_websockets/test_tutorial003.py 100.00% <100.00%> (ø)
fastapi/params.py 100.00% <0.00%> (ø)
fastapi/routing.py 100.00% <0.00%> (ø)
fastapi/encoders.py 100.00% <0.00%> (ø)
... and 159 more

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 c09e950...66e9c7f. Read the comment docs.

@github-actions
Copy link
Contributor

📝 Docs preview for commit a57d719cbfe0cb2d4dcb0afbdaceb46171c12a7c at: https://5f399310ac0d4928ee19262e--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit 42e8e73 at: https://5f39a0cb98faa3d93b585386--fastapi.netlify.app

@Kludex Kludex changed the title WIP: ✅ Add tests related to docs_src ✅ Add tests related to docs_src Aug 16, 2020
@Kludex Kludex changed the title ✅ Add tests related to docs_src ✅ Add docs_src tests Aug 16, 2020
@Kludex Kludex changed the title ✅ Add docs_src tests ✅ Add docs_src/ tests Aug 16, 2020
@Kludex Kludex changed the title ✅ Add docs_src/ tests ✅ Add 'docs_src' tests Aug 16, 2020
@github-actions
Copy link
Contributor

📝 Docs preview for commit 5244b0a at: https://5f39aeda46997952ac1355c1--fastapi.netlify.app

scripts/test.sh Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2020

📝 Docs preview for commit 44fe4952dce9e3f2b9c614eddc230f464ec1d255 at: https://5fa32afa0b2657112b5b7f05--fastapi.netlify.app

@Kludex
Copy link
Sponsor Collaborator Author

Kludex commented Nov 10, 2020

@tiangolo I've rebased it, I'm not sure why the docs stage is failing on the CI, but I saw another PR failing with the same reason.

@github-actions
Copy link
Contributor

📝 Docs preview for commit 20df532 at: https://5fb6c9dbfdba270137694c33--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit b372943 at: https://605129a909eae400d1973228--fastapi.netlify.app

@tiangolo tiangolo changed the title ✅ Add 'docs_src' tests ✅ Add the docs_src directory to test coverage and update tests Jul 29, 2021
@tiangolo tiangolo merged commit fa2c750 into tiangolo:master Jul 29, 2021
@tiangolo
Copy link
Owner

Great, thanks @Kludex! 🚀 ✔️

I updated it a bit, updated the docs where necessary to match the changed examples, and added a couple of missing tests for full coverage. Thanks! 🍰 ☕

test_main_b.test_create_item_bad_token()
test_main_b.test_read_inexistent_item()
test_main_b.test_read_item()
test_main_b.test_read_item_bad_token()
Copy link
Sponsor Collaborator Author

Choose a reason for hiding this comment

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

This doesn't duplicate the tests as well?

Copy link
Owner

Choose a reason for hiding this comment

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

If I remember correctly, the tests from docs_src are not run by pytest, so the way to run them and have coverage for that is to do this.

Sorry for the long delay! 🙈 I wanted to personally address each issue/PR and they piled up through time, but now I'm checking each one in order.

Copy link
Sponsor Collaborator Author

@Kludex Kludex Nov 27, 2022

Choose a reason for hiding this comment

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

I remember doing what I did because before my changes the tests were running twice, and I think (I didn't check) this makes them run twice (those were your changes, just for context...), because (I think) the import is enough to make pytest find the tests. Anyway... 👀

Copy link
Owner

Choose a reason for hiding this comment

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

Hum, maybe I have to review this again.

Although, one possible explanation is if you were running pytest and I was running pytest tests.

But anyway, yeah, maybe for later. 😅

solomein-sv pushed a commit to solomein-sv/fastapi that referenced this pull request Jul 30, 2021
…ngolo#1904)

Co-authored-by: Sebastián Ramírez <tiangolo@gmail.com>
@Kludex Kludex deleted the fix/coverage branch November 27, 2022 17:41
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

2 participants