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

App.run_test() is Incompatible with PyTest Fixtures #4998

Open
Chris3606 opened this issue Sep 13, 2024 · 6 comments
Open

App.run_test() is Incompatible with PyTest Fixtures #4998

Chris3606 opened this issue Sep 13, 2024 · 6 comments

Comments

@Chris3606
Copy link

Chris3606 commented Sep 13, 2024

Have you checked closed issues?

Yes

Issue

The App.run_test() function is incompatible with pytest fixtures. Consider the following code example:

import pytest
import pytest_asyncio
from textual.app import App
from textual.widgets import TextArea

class MyApp(App):
    BINDINGS = [('ctrl+a', 'add_widget', 'Add a widget.')]

    async def action_add_widget(self):
        await self.mount(TextArea())

@pytest_asyncio.fixture
async def my_pilot():
    app = MyApp()
    async with app.run_test() as pilot:
        yield pilot

@pytest.mark.asyncio
async def test_add_widget_programatically(my_pilot):
    await my_pilot.app.action_add_widget()

if __name__ == '__main__':
    app = MyApp()
    app.run()

Running this textual run works fine; you can press ctrl + a and the TextArea widget appears. However, if you runpytest, the test fails because calling the action_add_widget() function raised NoActiveAppError.

Environment:

textual==0.79.1
textual-dev==1.6.1
textual-serve==1.1.1
pytest==8.3.3
pytest-asyncio==0.24.0

I discussed this on a discord, and presumably there is a somewhat known incompatibility with pytest fixtures here. Moving the async with app.run_test() as pilot: line to within the test instead of using a fixture works fine for this case. However, I consider this a bug for a number of reasons.

First, not using fixtures is impractical for some use cases. The inability to use fixtures, for example, precludes the ability to use a single app instance to run multiple tests. A practical use case where this might be useful, is consider a CLI-like FTP client. One might want to connect to an FTP server, run a number of commands on the same server as separate test, and then disconnect (eg. session fixture scope).

Additionally, this issue has a code smell that makes me suspect undefined behavior/race conditions. A number of workarounds all work to solve the issue for this use case, including:

  • Moving the async_with statement to inside the test
  • Calling the action by simulating the keypress instead of calling the function
  • Adding await asyncio.sleep(0) to the fixture right before the yield

However, not all of these workarounds work for all use cases; in fact the only one that has thus far proven reliable is moving the async with statement (and my sample size is still relatively small). It seems to me to be in part a timing issue, thus the concern that the workaround is concealing a bigger issue.

Textual Diagnose Output

As far as I'm aware, since this only occurs with pytest, I can't do this.

@Textualize Textualize deleted a comment from github-actions bot Sep 13, 2024
@willmcgugan
Copy link
Collaborator

The root cause is that the fixture is created in one task, and the tests run in another task.

This means that the context variables created by run_test (active app) are not respected in the test.

The asyncio.sleep workaround has no effect for me, so I suspect there is something different in your environment.

Regardless, the context manager as a fixture approach is never going to work. I am looking in to a different mechanism to expose a pilot in a fixture.

@Chris3606
Copy link
Author

I wanted to post back here to point out a couple things.

First, my original code example was flawed. I forgot to await the result of mount. I edited the code example in my original comment to be correct; it still replicates the error (but with a more sensible error message).

Second, I've been researching this issue and as you're probably aware, there is a pretty lengthy ticket in pytest-asyncio's repository concerning the core issue; the summary is that they are aware of the issue but can't do much about it until at least the next release, due to how Python has changed the asyncio APIs.

In the mean-time, there is a workaround in that ticket in this comment, which involves a custom event loop policy that ensure that all tasks created from pytest_asyncio always use a shared context. It's hacky and could potentially be unreliable, but it does solve the issue in the trivial example I originally posted. I've yet to attempt to port it to a more complex one, but I suspect it probably works at least in cases where the re-use of contexts itself doesn't cause additional issues.

@mzebrak
Copy link

mzebrak commented Sep 27, 2024

I've run into the same problem while bumping the textual version (we've been sitting on a 0.63.0 for a while). In my case the test is green, but the issue happens during test teardown. During the test, there are multiple actions taking place like changing screens, writing to input etc. and that works fine.

Teardown fails in that way:

image

tests/tui/test_transfer.py:131 (test_transfers_finalize_cart[True])
def finalizer() -> None:
        """Yield again, to finalize."""
    
        async def async_finalizer() -> None:
            try:
                await gen_obj.__anext__()
            except StopAsyncIteration:
                pass
            else:
                msg = "Async generator fixture didn't stop."
                msg += "Yield only once."
                raise ValueError(msg)
    
>       event_loop.run_until_complete(async_finalizer())

../../.pyenv/versions/workplace/lib/python3.10/site-packages/pytest_asyncio/plugin.py:296: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../../.pyenv/versions/3.10.12/lib/python3.10/asyncio/base_events.py:649: in run_until_complete
    return future.result()
../../.pyenv/versions/workplace/lib/python3.10/site-packages/pytest_asyncio/plugin.py:288: in async_finalizer
    await gen_obj.__anext__()
tests/tui/conftest.py:134: in prepared_env
    async with app.run_test() as pilot:
../../.pyenv/versions/3.10.12/lib/python3.10/contextlib.py:206: in __aexit__
    await anext(self.gen)
clive/__private/ui/app.py:159: in run_test
    async with super().run_test(
../../.pyenv/versions/3.10.12/lib/python3.10/contextlib.py:206: in __aexit__
    await anext(self.gen)
../../.pyenv/versions/clive/lib/python3.10/site-packages/textual/app.py:1798: in run_test
    with app._context():
../../.pyenv/versions/3.10.12/lib/python3.10/contextlib.py:142: in __exit__
    next(self.gen)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = Clive(title='Clive (Quit)', classes={'-dark-mode'}, pseudo_classes={'dark', 'focus'})

    @contextmanager
    def _context(self) -> Generator[None, None, None]:
        """Context manager to set ContextVars."""
        app_reset_token = active_app.set(self)
        message_pump_reset_token = active_message_pump.set(self)
        try:
            yield
        finally:
>           active_message_pump.reset(message_pump_reset_token)
E           ValueError: <Token var=<ContextVar name='active_message_pump' at 0x7f610ce55cb0> at 0x7f6108c1a5c0> was created in a different Context

But in other tests, I can see the failure earlier.

About the asyncio.sleep(0) workaround - we actually needed a similar thing but not via asyncio.sleep but as pilot.pause() in one of our fixtures even BEFORE 0.63.0. Test using that fixture now fails before teardown, in the fixture, right after pilot.sleep() was placed and working before.

The mentioned workaround of

there is a workaround in that ticket in this comment

doesn't work for me as we're on python3.10 and Task has no context param there.

@Chris3606
Copy link
Author

Chris3606 commented Sep 27, 2024

The mentioned workaround of

there is a workaround in that ticket in this comment

doesn't work for me as we're on python3.10 and Task has no context param there.

Ah yes, I should have mentioned that only works on >= Python 3.11. Above the comment I linked (but on the same issue), there are workarounds that supposedly work for earlier Python versions (3.11 however deprecated some things so there was a new workaround needed for that version). I did not try the other workarounds (my code base requires 3.12); but it sounded like people had success with them.

@PabloLec
Copy link

Hello,

I'm experiencing the same error as @mzebrak, which only started happening after upgrading from 0.79.1 to 0.80.0, and it's still an issue.

The problem seems to be related to the recent asyncio changes mentioned earlier.

For reference, my tests were passing with the run_test() fixture from Python 3.8 to 3.12 just fine until Textual 0.80.0: https://github.com/PabloLec/RecoverPy/blob/77135c7ebefb7bd9ce0c94d38da7cc2f03283a20/tests/integration/test_full_workflow.py
(see async def pilot)

@mzebrak
Copy link

mzebrak commented Sep 30, 2024

I can confirm backporting Task from 3.11 to 3.10 and overriding event_loop with additional shared context-vars logic worked for me on 3.10.12.
However, I don't know if this works on Python 3.8, which I see you support @PabloLec. Might be worth trying.

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

No branches or pull requests

4 participants