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

Treat warnings as errors #1687

Merged
merged 19 commits into from
Jun 16, 2021
Merged

Treat warnings as errors #1687

merged 19 commits into from
Jun 16, 2021

Conversation

tomchristie
Copy link
Member

@tomchristie tomchristie commented Jun 15, 2021

Closes #1686
Closes #1670

  • Use -Werror when running pytest, to treat warnings as errors.
  • Use try..finally around auth_flow generator usage, with a .close() in the finally block.
  • Use getattr in __del__ to handle a case where an attribute might not exist on a partially initialised instance.
  • Use a context managed open() file in a test case.
  • Use ssl.PROTOCOL_TLS_CLIENT instead of deprecated ssl.PROTOCOL_TLS
  • Use context.minimum_version = instead of context.options |= ssl.OP_NO_* on Python 3.10+

@tomchristie tomchristie added the tooling Changes to our CI/CD, tests setup, etc. label Jun 15, 2021
@tomchristie
Copy link
Member Author

Weirdly, at this point the 3.10 test runs are hanging, but there's not any feedback to guide where or why.

@j178
Copy link
Member

j178 commented Jun 16, 2021

Ran tests locally, I find that it is because the uvicorn server failed to start if we treat warnings as error:
image

We need to get uvicorn running under 3.10 without warnings first.

Related: encode/uvicorn#1070

@tomchristie
Copy link
Member Author

@j178 Fantastic bit of digging, thanks! Okay, so blocked on a 3.10 compatriot uvicorn release. Gotcha.

setup.cfg Outdated
@@ -15,7 +15,7 @@ profile = black
combine_as_imports = True

[tool:pytest]
addopts = -rxXs
addopts = -rxXs -Werror
Copy link
Member

Choose a reason for hiding this comment

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

use filterwarnings=error

Copy link
Member

@j178 j178 Jun 16, 2021

Choose a reason for hiding this comment

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

This reminds me that we can use

filterwarnings = 
    error
    default:::uvicorn

to exempt uvicorn warnings, so that we will not be blocked by uvicorn.

Copy link
Member Author

Choose a reason for hiding this comment

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

use filterwarnings=error

Could you expand on that @graingert?

to exempt uvicorn warnings, so that we will not be blocked by uvicorn.

Oh, that's a great tip, @j178. I'm not sure what I need to do to inline that here, but will look into it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah okay, cribbing an example from here pytest-dev/pytest#4180

[tool:pytest]
addopts =
  -W error
  -v
filterwarnings =
  error
  # ignored by default
  ignore::ImportWarning
  ignore::PendingDeprecationWarning
  # raised by Cython, usually harmless
  ignore:numpy.dtype size changed:RuntimeWarning
  ignore:numpy.ufunc size changed:RuntimeWarning
  # raised by Werkzeug
  ignore:Request.is_xhr is deprecated:DeprecationWarning
  # raised by Moto
  ignore:Flags not at the start of the expression:DeprecationWarning

Copy link
Member

Choose a reason for hiding this comment

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

@tomchristie
Copy link
Member Author

to exempt uvicorn warnings, so that we will not be blocked by uvicorn.

Okay, gave that a try. Looks like 3.10 is still stuck.

@j178
Copy link
Member

j178 commented Jun 16, 2021

@tomchristie drop -Werror from addopts will work

@tomchristie
Copy link
Member Author

Alrighty, ready for review I think.

@tomchristie tomchristie merged commit b43af72 into master Jun 16, 2021
@tomchristie tomchristie deleted the pytest-treat-warnings-as-errors branch June 16, 2021 14:34
@tomchristie tomchristie mentioned this pull request Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling Changes to our CI/CD, tests setup, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch CI to treat warnings as errors uses deprecated ssl.PROTOCOL_TLS
3 participants