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

v0.68.0 test_deadlock is failing #4649

Closed
Antiz96 opened this issue Jun 14, 2024 · 12 comments
Closed

v0.68.0 test_deadlock is failing #4649

Antiz96 opened this issue Jun 14, 2024 · 12 comments

Comments

@Antiz96
Copy link

Antiz96 commented Jun 14, 2024

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 #4643 & #4647

I remain available if any more info are needed :)

Copy link

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Author

Antiz96 commented Jun 14, 2024

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
Copy link
Collaborator

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
Copy link
Author

Antiz96 commented Jun 14, 2024

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

@willmcgugan
Copy link
Collaborator

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
Copy link
Author

Antiz96 commented Jun 14, 2024

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! 🎉

@Antiz96
Copy link
Author

Antiz96 commented Jun 14, 2024

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! 🙏

@willmcgugan
Copy link
Collaborator

No problem. I can push that to main now.

@Antiz96
Copy link
Author

Antiz96 commented Jun 14, 2024

Alright, I patched e9ad400 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 :)

@Antiz96 Antiz96 closed this as completed Jun 14, 2024
Copy link

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

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

2 participants