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

✏️ Fix typos in data for tests #4958

Merged
merged 2 commits into from Jun 22, 2023
Merged

Conversation

ryanrussell
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented May 26, 2022

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (cf73051) 100.00% compared to head (63efdc0) 100.00%.

❗ Current head 63efdc0 differs from pull request most recent head c3bc61b. Consider uploading reports for the commit c3bc61b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##            master     #4958    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          540       532     -8     
  Lines        13969     13672   -297     
==========================================
- Hits         13969     13672   -297     
Impacted Files Coverage Δ
tests/test_schema_extra_examples.py 100.00% <ø> (ø)
tests/test_param_include_in_schema.py 100.00% <100.00%> (ø)

... and 36 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link
Contributor

📝 Docs preview for commit 63efdc0 at: https://628faf69384877273be629ca--fastapi.netlify.app

Copy link
Contributor

@iudeen iudeen left a comment

Choose a reason for hiding this comment

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

Good one!

Copy link

@Ryandaydev Ryandaydev left a comment

Choose a reason for hiding this comment

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

The changes look correct, but the branch the changes are in has errors running the tests when I check it out. So I can't give it a clean review.

@iudeen
Copy link
Contributor

iudeen commented Dec 21, 2022

@Ryandaydev it'd be great if you can list out the errors. I see the GitHub checks have passed for this PR.

@Ryandaydev
Copy link

Ryandaydev commented Dec 21, 2022

When I use gh checkout 4958, it looks at the ryanrussell/master branch. In that branch, it looks like the test-cov-html.sh file is an old version containing an argument that isn't in the main branch version.

So I can't run the test coverage script to get a clean test run.

Here is the error:
pytest: error: unrecognized arguments: --cov=fastapi --cov=tests --cov=docs_src --cov-report=term-missing:skip-covered --cov-report=xml --cov-report=html

@iudeen
Copy link
Contributor

iudeen commented Dec 21, 2022

@Ryandaydev I checked from local, it works fine.

1346 passed in 21.79s

Ensure you have installed all the dependencies needed.

pip install -e ."[dev,doc,test]" --upgrade

Then run:

coverage run -m pytest tests

@Ryandaydev
Copy link

Ryandaydev commented Dec 21, 2022

Thanks, the pip command resolved my issue. No objections now -- I'll mark it as approve.

Copy link

@Ryandaydev Ryandaydev left a comment

Choose a reason for hiding this comment

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

Minor change looks good. Tests ran successfully.

@tiangolo tiangolo changed the title Improve test readability ✏️ Fix typos in data for tests Jun 22, 2023
@tiangolo
Copy link
Owner

Great, thanks @ryanrussell! 🔍 🤓

And thanks for the reviews and comments everyone! ☕

@tiangolo tiangolo enabled auto-merge (squash) June 22, 2023 11:24
@tiangolo tiangolo merged commit b4b39d3 into tiangolo:master Jun 22, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet