dabeaz / curio

Good Curio!
Other
4.01k stars 240 forks source link

Pytest plugin conflicts with AnyIO's #327

Closed agronholm closed 3 years ago

agronholm commented 3 years ago

Curio's pytest plugin wraps any test coroutine function that contains the keyword curio anywhere. This causes problems for the AnyIO pytest plugin which, when testing against Curio, does cause curio to appear on this list. The fix is to do:

if pyfuncitem.get_closest_marker('curio'):

instead of

if 'curio' in pyfuncitem.keywords:

in curio/pytest_plugin.py.

This of course would be easier to test if the async tests in curio's test suite were coroutine functions and relied on curio's own pytest plugin to run them.

dabeaz commented 3 years ago

If someone wants to rewrite Curio's test suite, I'd consider a pull request.

agronholm commented 3 years ago

If someone wants to rewrite Curio's test suite, I'd consider a pull request.

I'll give that a shot if it means this change can go in.

dabeaz commented 3 years ago

It depends on what the change involves. If it's going to load up Curio with things like type-hints, external dependencies, and extra tooling, then it will probably be rejected. Would suggest writing and proposing a small example first.

agronholm commented 3 years ago

The only change I'm interested in is the one-line change outlined in the issue description. I thought you wanted me to write the test suite to use coroutine functions instead of running a local coroutine main function explicitly with the kernel fixture.

dabeaz commented 3 years ago

I'm okay with the one-line change. The issue of Curio's own test-suite is a bigger one. It's something that I might look at someday, but it's not a high priority right now. If someone else wanted to look at it, I'm fine with that as long as it's mindful to the constraints of extra tooling/cruft above.

agronholm commented 3 years ago

I made the comment about the test suite only because this piece of code not seem to be covered by the test suite so proving that it works correctly would be difficult.

dabeaz commented 3 years ago

Maybe just a single test to test that then. It's contributed code that I've had no involvement with so it's somewhat off my radar to be honest (sorry).

agronholm commented 3 years ago

I'll add that test.

agronholm commented 3 years ago

All done; ready for review.

agronholm commented 3 years ago

As AnyIO v2.0.0 final release approaches, I would appreciate a patch release that includes this fix. Otherwise developers will get confused when their async tests fail in subtle ways. I've worked around this by adding -p no:curio to AnyIO's own test suite, but I'm hoping to avoid having to document this as a general workaround.

dabeaz commented 3 years ago

I am now planning to remove the pytest plugin from Curio entirely (moving it to examples). There have been other reported problems with it. Curio itself does not use it.

agronholm commented 3 years ago

AnyIO's test plugin also works for any curio based libraries and apps (need to set the anyio_backend fixture to curio). It's also used by AnyIO's own test suite.

agronholm commented 3 years ago

Thank you for the new release!