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

opt into strict mode by default #380

Merged
merged 2 commits into from Jul 12, 2022
Merged

Conversation

graingert
Copy link
Member

@graingert graingert commented Jul 4, 2022

Fixes #293

It's looking like a critical mass of projects have set asyncio_mode= if they need it, and an increasing number of projects are getting compatibility issues where they set filterwarnings=error, do not use pytest-asyncio, but are installed into an environment with it already.

In addition pytest has now deprecated returning not-None values https://docs.pytest.org/en/latest/deprecations.html#returning-non-none-value-in-test-functions and so in a future version test suites returning Coros will fail, rather than pass with a warning

@Tinche
Copy link
Member

Tinche commented Jul 4, 2022

Hi,

I disagree with this. The default mode should be auto.

@graingert
Copy link
Member Author

graingert commented Jul 4, 2022

@Tinche the deprecation warning documents that once the deprecation period of the legacy mode ends the default mode will be strict:

LEGACY_MODE = DeprecationWarning(
"The 'asyncio_mode' default value will change to 'strict' in future, "
"please explicitly use 'asyncio_mode=strict' or 'asyncio_mode=auto' "
"in pytest configuration file."
)

@seifertm
Copy link
Contributor

seifertm commented Jul 5, 2022

I've always been a bit torn between which mode should be the default. Strict mode provides the greatest compatibility, but forces users to explicitly mark the tests. On the other hand, auto mode provides the greatest convenience. I believe both have a right to exist and switching between them is just one line away for any new project.

However, I do think that we should stick with the mode that was announced in the deprecation warning. I fear that anything else will create confusion.

@Tinche
Copy link
Member

Tinche commented Jul 5, 2022

The depreciation warning should definitely not force us to make the wrong call here. I'm sorry I missed this when I approved the initial PR that introduced it, but I seem to remember the consensus being we would default to auto.

auto is the novice user use case, and the most common use case. strict is when your tests need to coexist with other async frameworks - it should definitely exist, but it's an advanced feature only for authors of very complex libraries (if they need to be tested on multiple frameworks).

My opinion is it would be a disservice to the community (especially new users getting into async) to make the simplest and most common use case require configuration, and the super-complex use case the default. Also note that asyncio doesn't really stand on equal footing with the other async frameworks due to it being in the standard library.

@asvetlov what's your take on this?

@graingert
Copy link
Member Author

graingert commented Jul 5, 2022

The PR was created to allow pytest-asyncio to exist installed in a users virtual environment without interacting with other pytest plugins and suites

@njsmith
Copy link

njsmith commented Jul 6, 2022

The argument for making strict the default is that people aren't careful to only run tests inside a hermetically-constructed venv with only and exactly the packages they need. And the way pytest plugins work, just having the package in your environment at all means pytest will suck it in and start changing the semantics of any tests run in that environment.

So if you make strict the default, then asyncio packages need to install it and add one line of config to turn it on, same as how most pytest plugins work. (E.g. installing pytest-cov doesn't start generating coverage reports unless you use --cov-* options, pytest-xdist doesn't start distributing tests unless pass --numprocesses auto, etc.)

If you make auto the default, then all the packages that aren't using pytest-asyncio will need to add configuration to defensively disable pytest-asyncio, or else get bug reports from folks who tried to run tests in an environment that happened to have pytest-asyncio installed, and got an inscrutable error message.

@Tinche
Copy link
Member

Tinche commented Jul 6, 2022

You do have a point, but what you're saying is:

A subset of users,

  • who have a misconfigured virtual environment, with packages they do not want installed (which is lunacy, by the way, what test harness can work like this?)
  • who have async tests, but these tests are for a different async plugin than pytest-asyncio

Will get a strange error unless they apply a little configuration? I'm inclined to treat this as a red herring.

My gut feeling is that situations like these will be an order of magnitude more rare than the alternative, which is people just using pytest-asyncio by itself so I would rather place the burden of configuration on the smaller cohort. To your other point, there are also plenty of pytest plugins that change the behavior just by being present.

But, as I said, I do see your point. If both of my co-maintainers (@seifertm and @asvetlov) also want strict as the default, alright.

@graingert
Copy link
Member Author

graingert commented Jul 6, 2022

Will get a strange error unless they apply a little configuration? I'm inclined to treat this as a red herring.

They do not get an error, for example running the starlette test suite all the tests pass - however the tests against trio never run

@seifertm
Copy link
Contributor

seifertm commented Jul 7, 2022

My gut feeling is that situations like these will be an order of magnitude more rare than the alternative, which is people just using pytest-asyncio by itself so I would rather place the burden of configuration on the smaller cohort.

You're right: The most widespread use case is to install pytest-asyncio as part of a virtualenv. To be honest, I didn't even think of global installations until #293 was reported. I also agree that we should make pytest-asyncio easy to use for the standard use case.

However, the problem I see with defaulting to auto mode is that the resulting incompatibilities are much harder to understand. For example, the compatibility issue with pytest-trio (#124) caused the following non-sensical error message:
RuntimeError: must be called from async context

If I understand @graingert's comment correctly and there are cases where no error is raised, but tests are simply swallowed that's even worse.

If I were a user it wouldn't be obvious to me that merely installing a package messes with my test suite. I'd easily lose a day debugging the error message above. On the other hand if the README of a new library tells me need to decorate my tests with @pytest.mark.asyncio or switch to auto mode, that's easy to understand.

This is why I believe switching to --asyncio-mode=strict as the new default is the better choice.

@Tinche
Copy link
Member

Tinche commented Jul 10, 2022

Alright, since you folks feel so strongly, I can live with the default being strict.

@graingert graingert marked this pull request as ready for review Jul 10, 2022
@seifertm
Copy link
Contributor

seifertm commented Jul 11, 2022

This PR changes the default mode to "strict". It doesn't remove legacy mode and that's probably a good idea.

Here's my suggestion: We create a new release release (v0.19?) directly after this has been merged. All three modes will stay, but the default will have changed. If anyone needs more time moving to strict or auto, they can temporarily enable legacy mode.

The release after that removes legacy mode entirely. Does that plan sound reasonable?

@graingert Could you add an entry to the Changelog? It should clearly state that this is a breaking change and that users can switch back to the deprecated legacy mode if needed.

CHANGELOG.rst Outdated Show resolved Hide resolved
Clarify that no code changes were necessary for Python 3.11 support.

Co-authored-by: Thomas Grainger <tagrain@gmail.com>
Tinche
Tinche approved these changes Jul 11, 2022
@seifertm seifertm merged commit 07beb80 into pytest-dev:master Jul 12, 2022
8 checks passed
@graingert graingert deleted the strict-mode branch Jul 12, 2022
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.

4 participants