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 — gh-130474: Create and implement flaky resource by jtranq · Pull Request #130489 · python/cpython

Conversation

@jtranq

@jtranq jtranq commented Feb 23, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown

I've created the resource and implemented it in some obvious places where comments pointed to flaky tests. There were certain tests I left alone as although they were marked flaky by inline comments, there were other decorators addressing this, for example certain tests that were marked as being flaky without the GIL are already marked with the @support.requires_gil_enabled decorator which I think makes more sense.

As with other resources, the use-case for decorating tests is just:

from test.support import requires_resource

@requires_resource('flaky')
def test_flaky_test():
...

With this set-up, in CI environments we can configure the CI to run flaky tests separately and to automatically retry failed flaky tests::

python -m test --use=flaky --rerun

  • Issue: Add a new test resource to mark flaky tests #130474

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jtranq
Addressing creation and implementation of new flaky resrouce
00b6c4d
@jtranq
jtranq requested review from a team and gpshead as code owners February 23, 2025 21:03
@ghost

ghost commented Feb 23, 2025 •
edited by ghost
Loading

Uh oh!

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

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app

bedevere-app Bot commented Feb 23, 2025

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added the awaiting review label Feb 23, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Feb 23, 2025
Add a new test resource to mark flaky tests #130474
Closed
@blurb-it
📜🤖 Added by blurb_it.
31c5118
@tomasr8
tomasr8 self-requested a review February 23, 2025 21:31
@picnixz
picnixz self-requested a review February 23, 2025 22:16
picnixz
picnixz reviewed Feb 23, 2025
View reviewed changes

@picnixz picnixz 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.

Some tests are not necessarily flaky. You should also update the command-line of libregretest python -m test --help

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/_test_multiprocessing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_glob.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_signal.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_smtplib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/Tests/2025-02-23-21-20-52.gh-issue-130474.dEckCL.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@jtranq @picnixz
Apply suggestions from code review
820842f 
Co-authored-by: Bénédikt Tran <[email protected]>
@picnixz

picnixz commented Feb 23, 2025

Copy link
Copy Markdown
Member

Oh, the docs are failing. I thought that we had documented it but no. My bad.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
picnixz
picnixz reviewed Feb 23, 2025
View reviewed changes
Comment thread Misc/NEWS.d/next/Tests/2025-02-23-21-20-52.gh-issue-130474.dEckCL.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
jtranq and others added 3 commits February 23, 2025 18:07
@jtranq
update the command-line of libregretest
505e25f
@jtranq @picnixz
Apply suggestions from code review
34a1557 
Co-authored-by: Bénédikt Tran <[email protected]>
@jtranq
Change flaky to outer-decorator
81cc32c
picnixz
picnixz reviewed Feb 23, 2025
View reviewed changes
Comment thread Lib/test/test_signal.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
jtranq and others added 2 commits February 23, 2025 18:20
@jtranq @picnixz
Apply suggestions from code review
0c7f3fc 
Co-authored-by: Bénédikt Tran <[email protected]>
@jtranq
Remove unecessary import
1657217
picnixz
picnixz reviewed Feb 24, 2025
View reviewed changes

@picnixz picnixz 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.

Ok, so I'm not sure we should skip testSend for now because while it's marked as flaky, I've never seen it fail beforehand.

OTOH, test_stress_modifying_handlers is only flaky on free-threaded build so, if possible, we should not mark test_stress_modifying_handlers as flaky at all (testing on the regular would not be flaky at all!) Sorry for missing the point before but for my defense it's 1 AM here!

As for test_selflink, it's the only test that is really flaky actually and skipped (it's usually not flaky on an idle system, it's flaky on a stressed system). I would leave marking flaky tests in a separate PR (except for test_selflink).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_signal.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
jtranq and others added 3 commits February 23, 2025 19:26
@jtranq @picnixz
Apply suggestions from code review
711cfdd 
Co-authored-by: Bénédikt Tran <[email protected]>
@jtranq
Condense imports
4fb099c
@jtranq
Remove import and flaky decorator in test_smtplib
9fee4db
picnixz
picnixz reviewed Feb 24, 2025
View reviewed changes
Comment thread Lib/test/test_signal.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
jtranq added 2 commits February 23, 2025 19:41
@jtranq
Revert test_signal
47c2fea
@jtranq
Revert test_glob
806df62
picnixz
picnixz reviewed Feb 24, 2025
View reviewed changes
Comment thread Lib/test/test_signal.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_glob.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
jtranq and others added 2 commits February 23, 2025 20:10
@jtranq
Re-decorate function to flaky in test_glob
0dc2487
@jtranq @picnixz
Apply suggestions from code review
3298da3 
Co-authored-by: Bénédikt Tran <[email protected]>
picnixz
picnixz approved these changes Feb 24, 2025
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Feb 24, 2025
AA-Turner
AA-Turner reviewed Feb 24, 2025
View reviewed changes
Comment thread Misc/NEWS.d/next/Tests/2025-02-23-21-20-52.gh-issue-130474.dEckCL.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@jtranq @AA-Turner
Apply suggestions from code review
1b12e5c 
Co-authored-by: Adam Turner <[email protected]>
@picnixz
picnixz removed request for a team and gpshead February 24, 2025 01:43
AA-Turner
AA-Turner approved these changes Feb 24, 2025
View reviewed changes
@AA-Turner
AA-Turner requested a review from vstinner February 24, 2025 01:46
@AA-Turner

AA-Turner commented Feb 24, 2025

Copy link
Copy Markdown
Member

Looks ok, but Victor is probably the best one to review changes to libregrtest.

A
All reactions

Sorry, something went wrong.

Uh oh!

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

rossburton commented Feb 24, 2025

Copy link
Copy Markdown
Contributor

As for test_selflink, it's the only test that is really flaky actually and skipped (it's usually not flaky on an idle system, it's flaky on a stressed system). I would leave marking flaky tests in a separate PR (except for test_selflink).

Reading the linked issue in the comment for test_selflink, this doesn't appear to be a idle vs stressed issue at all (and it's potentially fixed with other improvements).
All reactions

Sorry, something went wrong.

Uh oh!

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

picnixz commented Feb 24, 2025

Copy link
Copy Markdown
Member

It is though? at least according to my readings of #109959 (comment). Anyway, the thing is that it's still flaky as it randomly fails.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Feb 24, 2025
View reviewed changes

@vstinner vstinner 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 dislike this change, If a test is known to be flaky, it should be either fixed or removed.

I suggest to remove test_selflink() in test_glob.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jtranq
Merge branch 'main' into flaky-resource
af0ff4d
@vstinner

vstinner commented Feb 25, 2025

Copy link
Copy Markdown
Member

I suggest to remove test_selflink() in test_glob.

I created #130551 to remove the flaky test.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner requested changes Feb 27, 2025
View reviewed changes

@vstinner vstinner 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 don't think that this "flaky" resource is a good idea.

I created #130551 to remove the flaky test.

I merged my PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot removed the awaiting merge label Feb 27, 2025
@bedevere-app

bedevere-app Bot commented Feb 27, 2025

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added the awaiting changes label Feb 27, 2025
@picnixz

picnixz commented Mar 2, 2025

Copy link
Copy Markdown
Member

In light of #130474 (comment), I think it was a bad idea (my bad idea). At first, it seemed great but hiding a flaky test behind a decorator wouldn't help solving the problem, we could even forget about it!

So I'll close the issue and this PR as the flaky tests I thought were flaky are being fixed (most of them happen on the free-threaded build and seem to be real issues, namely we should have written the tests differently). Thanks for the work though and sorry again!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz picnixz closed this Mar 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner requested changes
@AA-Turner AA-Turner AA-Turner approved these changes
@picnixz picnixz picnixz approved these changes
@tomasr8 tomasr8 Awaiting requested review from tomasr8

Uh oh!

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

Assignees

No one assigned

Labels

awaiting changes

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.

5 participants

@jtranq @picnixz @AA-Turner @rossburton @vstinner
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.