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

docs: Fix doc build #1833

Merged
merged 3 commits into from Apr 24, 2020
Merged

docs: Fix doc build #1833

merged 3 commits into from Apr 24, 2020

Conversation

chenjr0719
Copy link
Member

This PR aims to fix the doc build filed on readthedocs.

close #1832

readthedocs.yml Outdated Show resolved Hide resolved
@chenjr0719
Copy link
Member Author

BTW, the CI failed because of the new release of pytest-asyncio. Do you think we need to use the previous version instead? The pytest-asyncio is defined in tox.ini:
https://github.com/huge-success/sanic/blob/ae40f960ff0aa1c0ce24c50411faae6f16790acf/tox.ini#L12

@Tronic
Copy link
Member

Tronic commented Apr 22, 2020

BTW, the CI failed because of the new release of pytest-asyncio. Do you think we need to use the previous version instead?

I wonder what else should be upgraded for the new version to work. In any case, if pinning an older version in tox.ini gets the job done, it's OK.

Since this is the only PR hit by the test problem so far, I suggest that you do the change on this same PR despite it being a separate problem (because otherwise this bogus test failure will prevent merging).

Thanks for keeping the plumbing working! :)

@yunstanford
Copy link
Member

Do we need pytest-asyncio ?

@chenjr0719
Copy link
Member Author

Do we need pytest-asyncio ?

We have a lot of tests are implemented in async so I think yes we need it. For example:
https://github.com/huge-success/sanic/blob/ae40f960ff0aa1c0ce24c50411faae6f16790acf/tests/test_asgi.py#L245-L258

And, this issue is caused by the unmeet required version of pytest. The pytest-asyncio is required pytest>=5.4.0 but we pin it to 5.2.1. However, if using pytest>=5.4.0, then the pytest-sugar will cause another error. 🤔 They also discuss about this dependency issue at Teemu/pytest-sugar#187.

So, I would suggest that we can bump to pytest>=5.4.0 and remove pytest-sugar to fix current CI failed first. And if we really need pytest-sugar, we can add it back when they fix the dependency issue. What do you think?

@yunstanford
Copy link
Member

pytest-sanic will handle those async tests.

@codecov
Copy link

codecov bot commented Apr 24, 2020

Codecov Report

Merging #1833 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1833   +/-   ##
=======================================
  Coverage   92.18%   92.18%           
=======================================
  Files          23       23           
  Lines        2315     2315           
  Branches      425      425           
=======================================
  Hits         2134     2134           
  Misses        141      141           
  Partials       40       40           

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 ae40f96...9f18a3f. Read the comment docs.

@chenjr0719
Copy link
Member Author

OK, then I think the pytest-asyncio is not necessary anymore. After removing it, the CI now working. Thanks, @yunstanford. 😃

Copy link
Member

@yunstanford yunstanford left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@yunstanford yunstanford merged commit 638322d into sanic-org:master Apr 24, 2020
@chenjr0719 chenjr0719 deleted the fix_doc_build branch April 25, 2020 03:17
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.

Document build failed
3 participants