gh-93357: Lay the foundation for further work in `asyncio.test_streams`: port server cases to IsolatedAsyncioTestCase #93369

arhadthedev · 2022-05-31T07:04:30Z

This PR removes ad-hoc implementation of async tests by using all relatively new features of standard IsolatedAsyncIoTestCase.

Edit: this PR also removes messages variable used to pass exceptions between a main and a server threads. That's because the server is now async, lives in a main thread, and needs no intermediary to pass exceptions into a test harness.

To make changes possible to review, this PR changes a limited set of tests only. Further commits will expand the change further up and down.

For this, a single test class is temporarily split into three parts; two of them (StreamTests and StreamTests2) isolate old cases below and above the upgraded ones, and one (NewStreamTests) are the updated ones themselves. The reason is because subclassing from both old test_utils.TestCase and new unittest.IsolatedAsyncioTestCase leads to conflicts aborting each test with RuntimeError: cannot enter context: <...> is already entered.

Issue: Replace use of test.test_asyncio.utils.TestCase with unittest.IsolatedAsyncioTestCase #93357

Lib/test/test_asyncio/test_streams.py

arhadthedev · 2022-08-01T15:31:10Z

I've made a rewrite of description for both the PR and the issue to explain a target more clearly.

gvanrossum · 2022-09-27T17:49:00Z

This is a bit difficult to review since there is a lot of code being refactored. But I will give it a try (is there any other asyncio expert who is interested?).

sobolevn

I forgot to click "Submit" :)

Misc/NEWS.d/next/Tests/2022-05-31-09-58-10.gh-issue-93357.GL6KS0.rst

sobolevn · 2022-09-25T16:00:29Z

Lib/test/test_asyncio/test_streams.py

        # See http://bugs.python.org/issue35065
-        messages = []
-        self.loop.set_exception_handler(lambda loop, ctx: messages.append(ctx))


Looks like .set_exception_handler is now missing from this test. Is it fine?

Yes, I've removed all usages of messages inter-thread communication list:

- messages = [] - self.loop.set_exception_handler(lambda loop, ctx: messages.append(ctx))

- self.assertEqual(messages, [])

because the server is now async, lives in a main thread, and needs no intermediary to pass exceptions into a test harness.

By the way, thank you for noticing, I've added the clarification as the second paragraph of the first PR comment.

gvanrossum

Thanks! I've skimmed the code and it looks fine to me. Together with the expert reviews you've already received and addressed I think it's ready to merge.

I've read your full plan and once this lands please go ahead with the next steps!

kumaraditya303 · 2022-10-04T18:02:20Z

@gvanrossum This change broke ssl bots on main. See https://github.com/python/cpython/actions/runs/3184337370/jobs/5192630200

arhadthedev · 2022-10-04T18:05:31Z

@kumaraditya303 Thank you for reporting; I'm investigating now to fix it ASAP.

gvanrossum · 2022-10-04T18:07:13Z

Okay, give us updates as you go along -- IIRC the custom is to revert if no fix has been seen within 24 hours.

gvanrossum · 2022-10-04T18:11:04Z

FWIW my guess would be that you can't actually use async def test_blah(self) in a class derived from test_utils.TestCase.

arhadthedev · 2022-10-05T06:05:27Z

@gvanrossum I've created gh-97896 to fix the break. I'm sorry for the delay; initially I wanted to undo the affected lines but then I ended up with wrapping them into another IsolatedAsyncioTestCase thus making the affected tests async along with three more in-between.

~~Probably, OpenSSL runs need to be included into the mandatory checks to avoid such accidental oversights later.~~ Edit: I looked into the log under #93369 (comment); the failed tests seem to be skipped until a PR is merged. Can we resolve the problem by making in-PR runs to fail on warnings like after-merge ones do?

gvanrossum · 2022-10-05T18:04:31Z

There's a script that runs all the tests that depend on openssl, turning warnings into errors. I ran it manually like this:

./python.exe -m test.ssltests

I agree this ought to be part of the standard CI run, this has bitten me multiple times.

sobolevn · 2022-10-05T18:19:24Z

I agree this ought to be part of the standard CI run, this has bitten me multiple times.

I will open a new issue for it!

…olatedAsyncioTestCase (python#93369)" This reverts commit ce8fc18.

…_streams: port server cases to IsolatedAsyncioTestCase" (#98015) This PR reverts gh-93369 and gh-97896 because they've made asyncio tests unstable. After these PRs were merged, random GitHub action jobs of random commits started to fail unrelated tests and test framework methods. The reverting is necessary because such shrapnel failures are a symptom of some underlying bug that must be found and fixed first. I had a hope that it's a server overload because we already have extremely rare disc access errors. However, one and a half day passed, and the failures continue to emerge both in PRs and commits. Affected issue: gh-93357. First reported in #97940 (comment). * Revert "gh-93357: Port test cases to IsolatedAsyncioTestCase, part 2 (#97896)" This reverts commit 09aea94. * Revert "gh-93357: Start porting asyncio server test cases to IsolatedAsyncioTestCase (#93369)" This reverts commit ce8fc18.

…yncioTestCase (python#93369) Lay the foundation for further work in `asyncio.test_streams`.

…o.test_streams: port server cases to IsolatedAsyncioTestCase" (python#98015) This PR reverts pythongh-93369 and pythongh-97896 because they've made asyncio tests unstable. After these PRs were merged, random GitHub action jobs of random commits started to fail unrelated tests and test framework methods. The reverting is necessary because such shrapnel failures are a symptom of some underlying bug that must be found and fixed first. I had a hope that it's a server overload because we already have extremely rare disc access errors. However, one and a half day passed, and the failures continue to emerge both in PRs and commits. Affected issue: pythongh-93357. First reported in python#97940 (comment). * Revert "pythongh-93357: Port test cases to IsolatedAsyncioTestCase, part 2 (python#97896)" This reverts commit 09aea94. * Revert "pythongh-93357: Start porting asyncio server test cases to IsolatedAsyncioTestCase (python#93369)" This reverts commit ce8fc18.

Port server cases to IsolatedAsyncioTestCase

a052bf7

arhadthedev requested review from 1st1 and asvetlov as code owners May 31, 2022 07:04

bedevere-bot added tests Tests in the Lib/test dir awaiting review labels May 31, 2022

arhadthedev added 5 commits May 31, 2022 11:00

Add forgotten setUp and tearDown into StreamTests2

1f8977b

Use subtests to localize errors

3205970

Fix a wrongly used address

9f32482

Fix another typo

e41c7ca

Remove manual server starting

35e4f26

AA-Turner added the topic-asyncio label May 31, 2022

serhiy-storchaka reviewed Jun 1, 2022

View reviewed changes

Lib/test/test_asyncio/test_streams.py Outdated Show resolved Hide resolved

Lib/test/test_asyncio/test_streams.py Show resolved Hide resolved

arhadthedev added 3 commits June 1, 2022 09:39

Address a review of Serhiy Storchaka

92e30e5

Merge branch 'main' into teststreams-utilsTestCase-servers

e706f2c

Merge branch 'main' into teststreams-utilsTestCase-servers

690e451

serhiy-storchaka self-requested a review August 1, 2022 18:10

Merge branch 'main' into teststreams-utilsTestCase-servers

cabc398

gvanrossum self-requested a review as a code owner September 27, 2022 17:48

sobolevn reviewed Sep 27, 2022

View reviewed changes

Remove a What's New entry as proposed by Nikita Sobolev

dcc10dd

gvanrossum approved these changes Oct 4, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Oct 4, 2022

gvanrossum added the skip news label Oct 4, 2022

gvanrossum merged commit ce8fc18 into python:main Oct 4, 2022

bedevere-bot removed the awaiting merge label Oct 4, 2022

arhadthedev deleted the teststreams-utilsTestCase-servers branch October 4, 2022 17:57

ezio-melotti mentioned this pull request Oct 5, 2022

test_asyncio failures in the "Ubuntu with SSL" tests #97894

Closed

arhadthedev mentioned this pull request Oct 5, 2022

gh-93357: Port test cases to IsolatedAsyncioTestCase, part 2 #97896

Merged

sobolevn mentioned this pull request Oct 5, 2022

Run SSL tests on each CI run #97923

Closed

arhadthedev mentioned this pull request Oct 6, 2022

Replace use of test.test_asyncio.utils.TestCase with unittest.IsolatedAsyncioTestCase #93357

Closed

21 tasks

arhadthedev added a commit to arhadthedev/cpython that referenced this pull request Oct 7, 2022

Revert "pythongh-93357: Start porting asyncio server test cases to Is…

c17e930

…olatedAsyncioTestCase (python#93369)" This reverts commit ce8fc18.

arhadthedev mentioned this pull request Oct 7, 2022

gh-97983: Revert "Lay the foundation for further work in asyncio.test_streams: port server cases to IsolatedAsyncioTestCase" #98015

Merged

arhadthedev mentioned this pull request Oct 9, 2022

Fix a bunch of failing tests arhadthedev/cpython#11

Open

mpage pushed a commit to mpage/cpython that referenced this pull request Oct 11, 2022

pythongh-93357: Start porting asyncio server test cases to IsolatedAs…

81fcd2c

…yncioTestCase (python#93369) Lay the foundation for further work in `asyncio.test_streams`.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-93357: Lay the foundation for further work in `asyncio.test_streams`: port server cases to IsolatedAsyncioTestCase #93369

gh-93357: Lay the foundation for further work in `asyncio.test_streams`: port server cases to IsolatedAsyncioTestCase #93369

arhadthedev commented May 31, 2022 •

edited

Loading

arhadthedev commented Aug 1, 2022

gvanrossum commented Sep 27, 2022

sobolevn left a comment

sobolevn Sep 25, 2022

arhadthedev Oct 2, 2022

arhadthedev Oct 2, 2022

gvanrossum left a comment

kumaraditya303 commented Oct 4, 2022

arhadthedev commented Oct 4, 2022

gvanrossum commented Oct 4, 2022

gvanrossum commented Oct 4, 2022

arhadthedev commented Oct 5, 2022 •

edited

Loading

gvanrossum commented Oct 5, 2022

sobolevn commented Oct 5, 2022

gh-93357: Lay the foundation for further work in asyncio.test_streams: port server cases to IsolatedAsyncioTestCase #93369

gh-93357: Lay the foundation for further work in asyncio.test_streams: port server cases to IsolatedAsyncioTestCase #93369

Conversation

arhadthedev commented May 31, 2022 • edited Loading

arhadthedev commented Aug 1, 2022

gvanrossum commented Sep 27, 2022

sobolevn left a comment

Choose a reason for hiding this comment

sobolevn Sep 25, 2022

Choose a reason for hiding this comment

arhadthedev Oct 2, 2022

Choose a reason for hiding this comment

arhadthedev Oct 2, 2022

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

kumaraditya303 commented Oct 4, 2022

arhadthedev commented Oct 4, 2022

gvanrossum commented Oct 4, 2022

gvanrossum commented Oct 4, 2022

arhadthedev commented Oct 5, 2022 • edited Loading

gvanrossum commented Oct 5, 2022

sobolevn commented Oct 5, 2022

gh-93357: Lay the foundation for further work in `asyncio.test_streams`: port server cases to IsolatedAsyncioTestCase #93369

gh-93357: Lay the foundation for further work in `asyncio.test_streams`: port server cases to IsolatedAsyncioTestCase #93369

arhadthedev commented May 31, 2022 •

edited

Loading

arhadthedev commented Oct 5, 2022 •

edited

Loading