Textualize / textual

The lean application framework for Python. Build sophisticated user interfaces with a simple Python API. Run your apps in the terminal and a web browser.
https://textual.textualize.io/
MIT License
24.07k stars 741 forks source link

v0.68.0 test_deadlock is failing #4649

Closed Antiz96 closed 2 weeks ago

Antiz96 commented 2 weeks ago

Hi,

I'm trying to build v0.68.0 on Arch Linux using this PKGBUILD (which contains build and tests instructions) but the test_deadlock is failing with the following error:

=================================== FAILURES ===================================
________________________________ test_deadlock _________________________________

    @skip_windows
    def test_deadlock():
        """Regression test for https://github.com/Textualize/textual/issues/4643"""
        app_path = (Path(__file__) / "../deadlock.py").resolve().absolute()
        result = subprocess.run(
            f'echo q | python "{app_path}"', shell=True, capture_output=True
        )
        print(result.stdout)
>       assert result.returncode == 0
E       assert 1 == 0
E        +  where 1 = CompletedProcess(args='echo q | python "/build/python-textual/src/textual-0.68.0/tests/deadlock.py"', returncode=1, st...eadlock.py", line 6, in <module>\n    from textual.app import App\nModuleNotFoundError: No module named \'textual\'\n').returncode

tests/test_pipe.py:20: AssertionError
----------------------------- Captured stdout call -----------------------------
b''
=============================== warnings summary ===============================
tests/test_tooltips.py::test_removing_tipper_should_remove_tooltip
  /build/python-textual/src/textual-0.68.0/test-env/lib/python3.12/site-packages/textual/strip.py:99: RuntimeWarning: coroutine 'test_async_reactive_watch_callbacks_go_on_the_watcher.<locals>.MyApp.callback' was never awaited
    ] = FIFOCache(4)
  Enable tracemalloc to get traceback where the object was allocated.
  See https://docs.pytest.org/en/stable/how-to/capture-warnings.html#resource-warnings for more info.

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
FAILED tests/test_pipe.py::test_deadlock - assert 1 == 0
= 1 failed, 2473 passed, 1 skipped, 1 deselected, 5 xfailed, 1 warning in 117.14s (0:01:57) =

Tests works with v0.67.1 though.
It's apparently related to https://github.com/Textualize/textual/issues/4643 & https://github.com/Textualize/textual/pull/4647

I remain available if any more info are needed :)

github-actions[bot] commented 2 weeks ago

Thank you for your issue. Give us a little time to review it.

PS. You might want to check the FAQ if you haven't done so already.

This is an automated reply, generated by FAQtory

willmcgugan commented 2 weeks ago

This works on our CI. Not sure what might be different in your environment.

Could you add print(result.stderr) to see if that produces any more information?

willmcgugan commented 2 weeks ago

Ah wait. I see the error if I scroll right. This test is depending on textual being installed in the current environment. Can you install the package in your test runner?

Antiz96 commented 2 weeks ago

Ah wait. I see the error if I scroll right. This test is depending on textual being installed in the current environment.

Indeed, I haven't paid attention to No module named \'textual\'\n').returncode

Can you install the package in your test runner?

The test env is also the build env (Arch packages are built in a clean chroot and the tests are ran right after the build in that same env). I'm afraid having the textual package depending on itself for the tests to pass will go against our packaging guidelines (as said in this comment, that would be a "chicken-egg" problem).

I'll check if I can manage to make the tests use the path to the newly built binary (or add it to $PATH locally during tests) on my side.

willmcgugan commented 2 weeks ago

Presumably if I was to change the subprocess command to use {sys.executable} rather than python, that would ensure it is in the same environment?

Antiz96 commented 2 weeks ago

I'm afraid I cannot say... But I'd be happy to test if you're able to provide a patch!

willmcgugan commented 2 weeks ago

Can you give this a try?

diff --git a/tests/test_pipe.py b/tests/test_pipe.py
index 21329b302..92593ac4d 100644
--- a/tests/test_pipe.py
+++ b/tests/test_pipe.py
@@ -14,7 +14,8 @@ def test_deadlock():
     """Regression test for https://github.com/Textualize/textual/issues/4643"""
     app_path = (Path(__file__) / "../deadlock.py").resolve().absolute()
     result = subprocess.run(
-        f'echo q | python "{app_path}"', shell=True, capture_output=True
+        f'echo q | "{sys.executable}" "{app_path}"', shell=True, capture_output=True
     )
     print(result.stdout)
+    print(result.stderr)
     assert result.returncode == 0
Antiz96 commented 2 weeks ago

Can you give this a try?

diff --git a/tests/test_pipe.py b/tests/test_pipe.py
index 21329b302..92593ac4d 100644
--- a/tests/test_pipe.py
+++ b/tests/test_pipe.py
@@ -14,7 +14,8 @@ def test_deadlock():
     """Regression test for https://github.com/Textualize/textual/issues/4643"""
     app_path = (Path(__file__) / "../deadlock.py").resolve().absolute()
     result = subprocess.run(
-        f'echo q | python "{app_path}"', shell=True, capture_output=True
+        f'echo q | "{sys.executable}" "{app_path}"', shell=True, capture_output=True
     )
     print(result.stdout)
+    print(result.stderr)
     assert result.returncode == 0

Works! :tada:

Antiz96 commented 2 weeks ago

Do you eventually intend to merge this on upstream side or do you prefer me to keep this patch on the Arch package side instead?

In any case, thanks a lot for you quick help! :pray:

willmcgugan commented 2 weeks ago

No problem. I can push that to main now.

Antiz96 commented 2 weeks ago

Alright, I patched https://github.com/Textualize/textual/commit/e9ad400559c14645e56b7a98a467295c6817686b into the v0.68.0 textual Arch package while waiting for a new release to include it. Thanks once again!

I guess this issue can be closed now :)

github-actions[bot] commented 2 weeks ago

Don't forget to star the repository!

Follow @textualizeio for Textual updates.