Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

• X
• Mastodon
• Reddit
• LinkedIn

Comment @coderabbitai help to get the list of available commands and usage tips.

@fanninpm thanks for all the comments! I'll go through them, and resolve them! (Also hope you had a great New Year + Happy Holiday season!)

Could you also clarify what you mean by rider (I could not understand from the wikipedia link, and didn't find anything online). Do you mean the dual function of updating the library + adding the tests?

(And if so, how would you suggest proceeding? Opening a new PR/closing this one?)

@fanninpm, nevermind, I see what you mean 😓 I completely did not realize that unrelated commits had came with this PR. Thanks for catching it!

@fanninpm everything should be good; I've made the formatting changes. As a clarification, I found some of the tests were automatically skipped in RustPython, but were running fine on CPython. The reason is commented below each instance of this occurring

@fanninpm
So for all the commented out code, when uncommented out, the code crashes the test, and it is unable to run at all

For all the CPython specific tests, If I don't add the explicit skip, it still skips, but for a different reason (for example if I remove the skip in PyTask_CFutureSubclass_Tests in test_tasks.py it skips it for the reason skipped 'requires the C _asyncio module' (because as the code says, it needs a CPython specific module).

I figured better to be thorough and add the explicit skip, so when "TODO: RustPython" is searched, it shows up. Let me know if you want me to remove it

So for all the commented out code, when uncommented out, the code crashes the test, and it is unable to run at all

Does the @unittest.skip() decorator prevent this from happening?

For all the CPython specific tests, If I don't add the explicit skip, it still skips, but for a different reason (for example if I remove the skip in PyTask_CFutureSubclass_Tests in test_tasks.py it skips it for the reason skipped 'requires the C _asyncio module' (because as the code says, it needs a CPython specific module).

I figured better to be thorough and add the explicit skip, so when "TODO: RustPython" is searched, it shows up. Let me know if you want me to remove it

It might be better to just leave a comment, rather than a @unittest.skip() decorator in this instance.

So for all the commented out code, when uncommented out, the code crashes the test, and it is unable to run at all

Does the @unittest.skip() decorator prevent this from happening?

No it does not

For all the CPython specific tests, If I don't add the explicit skip, it still skips, but for a different reason (for example if I remove the skip in PyTask_CFutureSubclass_Tests in test_tasks.py it skips it for the reason skipped 'requires the C _asyncio module' (because as the code says, it needs a CPython specific module).
I figured better to be thorough and add the explicit skip, so when "TODO: RustPython" is searched, it shows up. Let me know if you want me to remove it

It might be better to just leave a comment, rather than a @unittest.skip() decorator in this instance.

Ok, I'll make the changes

@fanninpm Made all changes! I was able to change all the skips into expectedFailures (/removed the ones in test_tasks), and I removed all the CPython skips. Please let me know if there is anything else I should fix (assuming the tests all pass)!

Re: the CPython specific tests, should I also add an issue for those?

As an update, I've narrowed down the hang to an issue in the wait method of the Condition class. I think it's an issue with the AST generation? (but not 100%)

The test that is failing is test_locks.ConditionTests.test_cancelled_error_re_aquire
The relevant function is below

async def wait(self):
    """Wait until notified.

    If the calling task has not acquired the lock when this
    method is called, a RuntimeError is raised.

    This method releases the underlying lock, and then blocks
    until it is awakened by a notify() or notify_all() call for
    the same condition variable in another task.  Once
    awakened, it re-acquires the lock and returns True.

    This method may return spuriously,
    which is why the caller should always
    re-check the state and be prepared to wait() again.
    """
    if not self.locked():
        raise RuntimeError('cannot wait on un-acquired lock')

    fut = self._get_loop().create_future()
    self.release()
    try:
        try:
            self._waiters.append(fut)
            try:
                await fut                          # A) It reaches here fine
                return True
            finally:
                self._waiters.remove(fut)

        finally:
            # Must re-acquire lock even if wait is cancelled.
            # We only catch CancelledError here, since we don't want any
            # other (fatal) errors with the future to cause us to spin.
            err = None
            while True:                             # B) The code enters a loop here to reacquire the lock
                try:
                    await self.acquire()            # C) While waiting here, the task is cancelled
                    break
                except exceptions.CancelledError as e:
                    err = e                         # D) The code reaches here fine

            if err is not None:
                try:
                    raise err                       # E) The error is reraised here
                finally:
                    err = None                      # F) It reaches here
    except BaseException:
        self._notify(1)                             # G) It is supposed to exit here
        raise

Whereas in CPython, the program is supposed to go from F) to G), RustPython instead goes from F) to B), re-entering the final statement again (reaquiring the lock again at C), reaching a deadlock state, and hanging.

Thanks! That looks like a compiler bug. Is it possible to create a minimal reproducible script?

'the C _asyncio module' is a c module but probably not a CPython specific feature. We just need to implement it later.

Though we usually prefer to update asyncio and test_asyncio at the same time, test_asyncio anyway even didn't exist in RustPython.

Let's split this PR to 2 parts. Upgrade asyncio first, and see what's happening to other libraries. Currently it is not able to check because test_asyncio is hanging and other tests are not running. If no problem, asyncio will be merged. Then let's try to add test_asyncio again. It will take time due to the compiler problem

Thanks! That looks like a compiler bug. Is it possible to create a minimal reproducible script?

'the C _asyncio module' is a c module but probably not a CPython specific feature. We just need to implement it later.

Though we usually prefer to update asyncio and test_asyncio at the same time, test_asyncio anyway even didn't exist in RustPython.

Let's split this PR to 2 parts. Upgrade asyncio first, and see what's happening to other libraries. Currently it is not able to check because test_asyncio is hanging and other tests are not running. If no problem, asyncio will be merged. Then let's try to add test_asyncio again. It will take time due to the compiler problem

I'll work on getting something reproducible! In the meantime, Reopening this PR (Force pushing accidentally closed the issue)

Great! Failing test is unexpected success

======================================================================
UNEXPECTED SUCCESS: test_loop_factory (test.test_unittest.test_async_case.TestAsyncCase.test_loop_factory)
----------------------------------------------------------------------

Great! Failing test is unexpected success

======================================================================
UNEXPECTED SUCCESS: test_loop_factory (test.test_unittest.test_async_case.TestAsyncCase.test_loop_factory)
----------------------------------------------------------------------

Is it certain that this is not a fluke?

usually not when 3 CI envs agree on

@terryluan12 could you remove expectedFailure marker from the successful one?

@youknowone @fanninpm It seems test_multiprocessing_fork is hanging somewhat intermittently; How should we proceed?

@terryluan12 If you can find out what test is the cause, mark it as @unittest.skip("TODO: RUSTPYTHON; randomly hang") But it can be done in different PR

Sounds good. I'm looking right now. I think I found it, but just need to confirm

Sounds good! In terms of test_rapid_restart, it doesn't randomly hang; The tests are dynamically generated for 3 or so types, and for two of these types, they pass, and for the last one they fail. I think I've got it so that the expectedFailure is dynamically added though, so should be removed soon.

Lol, okay, this is so hacked together. Thoughts @youknowone @fanninpm ? (I annotated the lines added, but they will not be on the final commit)

def test_rapid_restart(self):
    failing_test = "WithManagerTestManagerRestart"   # RUSTPYTHON
    rustpython_errors = False                        # RUSTPYTHON
    try:                                             # RUSTPYTHON
        authkey = os.urandom(32)
        manager = QueueManager(
            address=(socket_helper.HOST, 0), authkey=authkey,
            serializer=SERIALIZER, shutdown_timeout=SHUTDOWN_TIMEOUT)
        try:
            srvr = manager.get_server()
            addr = srvr.address
            # Close the connection.Listener socket which gets opened as a part
            # of manager.get_server(). It's not needed for the test.
            srvr.listener.close()
            manager.start()

            p = self.Process(target=self._putter, args=(manager.address, authkey))
            p.start()
            p.join()
            queue = manager.get_queue()
            self.assertEqual(queue.get(), 'hello world')
            del queue
        finally:
            if hasattr(manager, "shutdown"):
                manager.shutdown()

        manager = QueueManager(
            address=addr, authkey=authkey, serializer=SERIALIZER,
            shutdown_timeout=SHUTDOWN_TIMEOUT)
        try:
            manager.start()
            self.addCleanup(manager.shutdown)
        except OSError as e:
            if e.errno != errno.EADDRINUSE:
                raise
            # Retry after some time, in case the old socket was lingering
            # (sporadic failure on buildbots)
            time.sleep(1.0)
            manager = QueueManager(
                address=addr, authkey=authkey, serializer=SERIALIZER,
                shutdown_timeout=SHUTDOWN_TIMEOUT)
            if hasattr(manager, "shutdown"):
                self.addCleanup(manager.shutdown)
    except Exception as e:                           # RUSTPYTHON
        if self.__class__.__name__ != failing_test:  # RUSTPYTHON
            raise                                    # RUSTPYTHON
    else:                                            # RUSTPYTHON
        if self.__class__.__name__ == failing_test:  # RUSTPYTHON
            raise                                    # RUSTPYTHON

We usually don't edit the inside of test without a good reason. Because editing inside increase the cost of updating the test to a new version. Is this test critical enough to enable by editing test code? Otherwise marking the entire test expectedFailure is more desired.

We usually don't edit the inside of test without a good reason. Because editing inside increase the cost of updating the test to a new version. Is this test critical enough to enable by editing test code? Otherwise marking the entire test expectedFailure is more desired.

I'm not sure what's meant by criticality, but the main issue is that these tests are run/created dynamically. So this test is used for the WithProcessesTestManagerRestart, WithThreadsTestManagerRestart, and WithManagerTestManagerRestart test classes.

As a result, if it's marked expectedFailure, then it will be expectedFailure for all three test classes; but the issue is that it succeeds for two out of the three classes, so no matter what, tests will fail

You can create an override of the method in the subclass
Searching " super()" in test code will show examples

Code has been automatically formatted

The code in this PR has been formatted using cargo fmt --all.
Please pull the latest changes before pushing again:

git pull origin update_asyncio

Could you please rebase this branch on a fresh copy of main? #6410 has just been merged and contains a lot of changes to the test suite.

This must be almost done. Could you rebase this one to the main?

I believe the failure in the ubuntu CI, is the same issue as #6716. Other than that, this should be good to rereview!