Skip to content

Python 3.13 test failures #616

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

Closed
gsauthof opened this issue Dec 21, 2023 · 10 comments
Closed

Python 3.13 test failures #616

gsauthof opened this issue Dec 21, 2023 · 10 comments

Comments

@gsauthof
Copy link
Contributor

gsauthof commented Dec 21, 2023

The Fedora Python maintainers regularly build all python packages with Python pre-release versions in order to catch issues early.

In the last such test asyncssh was flagged: https://bugzilla.redhat.com/show_bug.cgi?id=2251916

Basically a few test cases report variations of FileNotFoundError issues, such as:

FileNotFoundError: [Errno 2] No such file or directory: 'echo'

Perhaps some mocking behavior changed in Python 3.13, on which asyncssh depends on.

Of course, these test case failures may very well also be caused by a genuine new bug or regression in that python 3.13 alpha version.

@ronf
Copy link
Owner

ronf commented Dec 22, 2023

Thanks for the heads-up! I was able to reproduce the errors here with Python 3.13.0a2, and confirm that these errors do not appear when testing the latest AsyncSSH against Python 3.12. The errors seem related to tests involving UNIX domain sockets, but I'm not quite sure yet why the sockets aren't being found when the test tries to remove them during cleanup. I'm wondering if maybe Python made to change to automatically clean these files up itself. I need to go through the release notes and do some additional testing here.

@ronf
Copy link
Owner

ronf commented Dec 22, 2023

Aha - from the release notes:

@ronf
Copy link
Owner

ronf commented Dec 22, 2023

Could you give the following a try?

diff --git a/tests/test_agent.py b/tests/test_agent.py
index 28ca730..2f0b83c 100644
--- a/tests/test_agent.py
+++ b/tests/test_agent.py
@@ -85,7 +85,10 @@ class _Agent:
         self._server.close()
         await self._server.wait_closed()

-        os.remove(self._path)
+        try:
+            os.remove(self._path)
+        except OSError:
+            pass


 class _TestAgent(AsyncTestCase):
diff --git a/tests/test_forward.py b/tests/test_forward.py
index cae199d..4d30eda 100644
--- a/tests/test_forward.py
+++ b/tests/test_forward.py
@@ -651,7 +651,10 @@ class _TestTCPForwarding(_CheckForwarding):
             async with conn.forward_local_path_to_port('local', '', 7):
                 await self._check_local_unix_connection('local')

-        os.remove('local')
+        try:
+            os.remove('local')
+        except OSError:
+            pass

     @unittest.skipIf(sys.platform == 'win32',
                      'skip UNIX domain socket tests on Windows')
@@ -665,7 +668,10 @@ class _TestTCPForwarding(_CheckForwarding):
             with self.assertRaises(OSError):
                 await conn.forward_local_path_to_port('local', '', 7)

-        os.remove('local')
+        try:
+            os.remove('local')
+        except OSError:
+            pass

     @asynctest
     async def test_forward_local_port_pause(self):
@@ -798,7 +804,11 @@ class _TestTCPForwarding(_CheckForwarding):

         server.close()
         await server.wait_closed()
-        os.remove('local')
+
+        try:
+            os.remove('local')
+        except OSError:
+            pass

     @asynctest
     async def test_forward_remote_specific_port(self):
@@ -1020,7 +1030,10 @@ class _TestUNIXForwarding(_CheckForwarding):
             await listener.wait_closed()
             listener.close()

-        os.remove('echo')
+        try:
+            os.remove('echo')
+        except OSError:
+            pass

     @asynctest
     async def test_unix_server_open(self):
@@ -1053,7 +1066,10 @@ class _TestUNIXForwarding(_CheckForwarding):
             async with conn.start_unix_server(_unix_listener_non_async, path):
                 await self._check_local_unix_connection('echo')

-        os.remove('echo')
+        try:
+            os.remove('echo')
+        except OSError:
+            pass

     @asynctest
     async def test_unix_server_failure(self):
@@ -1071,7 +1087,10 @@ class _TestUNIXForwarding(_CheckForwarding):
             async with conn.forward_local_path('local', '/echo'):
                 await self._check_local_unix_connection('local')

-        os.remove('local')
+        try:
+            os.remove('local')
+        except OSError:
+            pass

     @asynctest
     async def test_forward_local_port_to_path_accept_handler(self):
@@ -1149,8 +1168,11 @@ class _TestUNIXForwarding(_CheckForwarding):
         server.close()
         await server.wait_closed()

-        os.remove('echo')
-        os.remove('local')
+        try:
+            os.remove('echo')
+            os.remove('local')
+        except OSError:
+            pass

     @asynctest
     async def test_forward_remote_path_to_port(self):
@@ -1167,11 +1189,14 @@ class _TestUNIXForwarding(_CheckForwarding):
                     path, '127.0.0.1', server_port):
                 await self._check_local_unix_connection('echo')

-        os.remove('echo')
-
         server.close()
         await server.wait_closed()

+        try:
+            os.remove('echo')
+        except OSError:
+            pass
+
     @asynctest
     async def test_forward_remote_path_failure(self):
         """Test failure of forwarding a remote UNIX domain path"""
@@ -1184,7 +1209,10 @@ class _TestUNIXForwarding(_CheckForwarding):
             with self.assertRaises(asyncssh.ChannelListenError):
                 await conn.forward_remote_path(path, 'local')

-        os.remove('echo')
+        try:
+            os.remove('echo')
+        except OSError:
+            pass

     @asynctest
     async def test_forward_remote_path_not_permitted(self):

It seems to work for me here, and these changes also work fine in older Python versions.

That said, this change to automatically clean up the sockets seems like a backward compatibility problem. I'm sure there's lots of other code out there which tries to clean these files up, and may not guard for failure on that.

@gsauthof
Copy link
Contributor Author

gsauthof commented Dec 23, 2023

I've tested that patch on Fedora rawhide (i.e. the bleeding edge Fedora flavor) and with it all tests succeed, as expected.

I agree that other code might run into similar issues with such a library change.
Unless it creates all the sockets in a temporary directory which is removed recursively, at the end of all tests/work.

@ronf
Copy link
Owner

ronf commented Dec 23, 2023

Yeah - AsyncSSH’s “AsyncTestCase” (and the associated asynctest decorator) derives from a class which uses TemporaryDirectory to do this, but at the moment that’s done at the class level, not at the individual test level, as there’s some shared infrastructure which is expensive to set up that is done only once per class instantiation. As such, as there a handful of cases where the individual tests do their own cleanup of specific things they create, and the tests returning errors were some of the examples of that.

I’m going to go ahead and check this into “develop” for now and will plan on making this a part of the next release, as it shouldn’t harm anything.

Do you know the timing of when Fedora would want to pick up Python 3.13? It looks like it is still a ways off (https://peps.python.org/pep-0719/ says 9+ months before 3.13.0 final).

@gsauthof
Copy link
Contributor Author

gsauthof commented Dec 23, 2023

Fedora targets Fedora 41 for the Python 3.13 update, i.e. it would be due end of 2024.

Fedora is on a half-year release schedule, i.e. even ones are due April-ish and odd ones around October-ish or so.
For example, Fedora 39 was released on November, 7th.

@ronf
Copy link
Owner

ronf commented Dec 23, 2023

Thanks for the info and the early warning! The guard code is now available as commit 5816813 in "develop" just in case.

I also replied to the original commit of the cleanup code asking about backward compatibility. It looks like there's a new option to disable the cleanup, but that would require a code change in the caller since it defaults to doing the cleanup, and that code change would have to be conditional on Python version since the new option would only exist in 3.13 and later.

@gsauthof
Copy link
Contributor Author

FWIW, I've just pushed an update to Fedora Rawhide that integrates that commit as a patch because Fedora switched to python 3.13 (pre-release) there for good.

@ronf
Copy link
Owner

ronf commented Jun 27, 2024

Thanks for that -- it looks like the fix committed here just missed the last AsyncSSH release back in December.

If all goes well, I plan to do a new AsyncSSH release some time next week, and this commit will be included in that, hopefully eliminating the need for your patch long-term.

@ronf
Copy link
Owner

ronf commented Jul 3, 2024

This fix is now available in AsyncSSH 2.15.0.

@ronf ronf closed this as completed Aug 18, 2024
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