GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-93357: Lay the foundation for further work in `asyncio.test_streams`: port server cases to IsolatedAsyncioTestCase by arhadthedev · Pull Request #93369 · python/cpython

Conversation

arhadthedev commented May 31, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          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


          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

Comment thread

Lib/test/test_asyncio/test_streams.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/test/test_asyncio/test_streams.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

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

arhadthedev commented Aug 1, 2022

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

gvanrossum commented Sep 27, 2022

Copy link

Copy Markdown

Member

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?).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sobolevn reviewed

Sep 27, 2022

View reviewed changes

sobolevn left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to click "Submit" :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/test/test_asyncio/test_streams.py

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

sobolevn Sep 25, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

arhadthedev Oct 2, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

arhadthedev Oct 2, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


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

dcc10dd

gvanrossum approved these changes

Oct 4, 2022

View reviewed changes

gvanrossum left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

kumaraditya303 commented Oct 4, 2022

Copy link

Copy Markdown

Member Author

@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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gvanrossum commented Oct 5, 2022

Copy link

Copy Markdown

Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sobolevn commented Oct 5, 2022

Copy link

Copy Markdown

Member

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

gvanrossum pushed a commit that referenced this pull request

Oct 7, 2022


          gh-97983: Revert "Lay the foundation for further work in asyncio.test…

f99bb20

…_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.

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`.

mpage pushed a commit to mpage/cpython that referenced this pull request

Oct 11, 2022


          pythongh-97983: Revert "Lay the foundation for further work in asynci…

e75ada1

…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.

Reviewers

sobolevn

sobolevn left review comments

gvanrossum

gvanrossum approved these changes

1st1

Awaiting requested review from 1st1

1st1 is a code owner

asvetlov

Awaiting requested review from asvetlov

asvetlov is a code owner

serhiy-storchaka

Awaiting requested review from serhiy-storchaka

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news tests

Tests in the Lib/test dir

topic-asyncio

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

arhadthedev commented May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arhadthedev commented Aug 1, 2022

Uh oh!

gvanrossum commented Sep 27, 2022

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sobolevn Sep 25, 2022

Choose a reason for hiding this comment

Uh oh!

arhadthedev Oct 2, 2022

Choose a reason for hiding this comment

Uh oh!

arhadthedev Oct 2, 2022

Choose a reason for hiding this comment

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

kumaraditya303 commented Oct 4, 2022

Uh oh!

arhadthedev commented Oct 4, 2022

Uh oh!

gvanrossum commented Oct 4, 2022

Uh oh!

gvanrossum commented Oct 4, 2022

Uh oh!

arhadthedev commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum commented Oct 5, 2022

Uh oh!

sobolevn commented Oct 5, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

arhadthedev commented May 31, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

arhadthedev commented Oct 5, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.