GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fix an outdated test in asyncio by fantix · Pull Request #24477 · python/cpython

Conversation

@fantix

@fantix fantix commented Feb 7, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

The test was added in f3e2e092 before _StopError was removed in 41f69f4, after which this test will not fail without the fix it covers. This PR fixes the test to resume its coverage.

  • skip issue
  • skip news

Refs: MagicStack/uvloop#337

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@fantix
Fix an outdated test in asyncio
77ac8e7 
The test was added in f3e2e092 before `_StopError` was removed in
41f69f4, after which this test will not fail without the fix it covers.
This PR fixes the test to resume its coverage.
@fantix
fantix requested review from 1st1 and asvetlov as code owners February 7, 2021 20:40
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 7, 2021
@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Feb 7, 2021
@github-actions

github-actions Bot commented Mar 10, 2021

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Mar 10, 2021
MaxwellDupre
MaxwellDupre approved these changes Apr 2, 2022
View reviewed changes

@MaxwellDupre MaxwellDupre left a comment

Copy link
Copy Markdown
Contributor

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.

Ran 2287 tests in 83.131s
OK (skipped=18) All windows tests
Also hit base_events.
Looks good.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Apr 2, 2022
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Aug 4, 2022
@iritkatriel iritkatriel added the topic-asyncio label Nov 9, 2022
@iritkatriel
iritkatriel requested a review from kumaraditya303 December 7, 2022 19:58
@kumaraditya303 kumaraditya303 added skip issue skip news labels Dec 8, 2022
@kumaraditya303
Merge branch 'main' into fix-outdated-test
c793590
@kumaraditya303
kumaraditya303 requested a review from gvanrossum as a code owner December 8, 2022 07:48
@netlify

netlify Bot commented Dec 8, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

✅ Deploy Preview for python-cpython-preview canceled.

Name Link
🔨 Latest commit c793590
🔍 Latest deploy log https://app.netlify.com/sites/python-cpython-preview/deploys/639196c70ff0c10009fefc96
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kumaraditya303
kumaraditya303 merged commit e8fff51 into python:main Dec 8, 2022
@bedevere-bot bedevere-bot removed the awaiting core review label Dec 8, 2022
gvanrossum
gvanrossum reviewed Dec 8, 2022
View reviewed changes
Comment thread Lib/test/test_asyncio/test_base_events.py
self.loop.run_forever()
except KeyboardInterrupt:
pass
self.loop.call_later(0.01, func)

@gvanrossum gvanrossum Dec 8, 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.

A bit unfortunate that we had to add a 10 msec delay here -- in most cases that will waste 10 msec and occasionally it will not wait long enough and the test will flake-fail. Why won't call_soon work?

Sorry, something went wrong.

Uh oh!

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

@fantix fantix Dec 8, 2022

Copy link
Copy Markdown
Contributor 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.

Background: this test covers a bug that base exceptions like KeyboardInterrupt will cause the loop to be closed twice, leading to an early exit in the next loop run.

This test was added before _StopError was dropped, at that time the loop will stop as soon as _StopError is raised, even if the _ready queue still has a handle (which is the next run - the func() here), therefore this was a good test. But now the loop will exhaust the _ready queue before stopping, so this test couldn't cover the bug anymore without this PR. That's also why we cannot simply add func() into the _ready queue by call_soon().

You're right about the flakyness of using call_later() tho. Now I think a better fix would be to use chained call_soon() to skip the first iteration where the buggy stop occured:

Suggested change
self.loop.call_later(0.01, func)
# If the issue persists, the loop will exit early after the first iteration,
# in that case our func() in the second iteration won't be called.
self.loop.call_soon(self.loop.call_soon, func)

I verified and this error fails if the fix in f3e2e09 is reverted.

Sorry, something went wrong.

Uh oh!

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

@gvanrossum gvanrossum Dec 8, 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.

Clever solution, go ahead and make it a PR, assign to me or @kumaraditya303 for review.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@fantix fantix mentioned this pull request Dec 9, 2022
Fix potential flakiness in test_run_until_complete_baseexception #100148
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum left review comments
@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner
@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner
@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 is a code owner
+1 more reviewer
@MaxwellDupre MaxwellDupre MaxwellDupre approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

skip issue 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.

8 participants

@fantix @gvanrossum @MaxwellDupre @iritkatriel @the-knights-who-say-ni @ezio-melotti @bedevere-bot @kumaraditya303
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 01402520 — 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.