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-103247: add `tearDown` to `test_loader` and clear module cache by sunmy2019 · Pull Request #104226 · python/cpython

Conversation

@sunmy2019

@sunmy2019 sunmy2019 commented May 6, 2023 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member
  • Issue: test_pickle/test_pickletools fail when running tests sequentially. #103247

Sorry, something went wrong.

Uh oh!

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

All reactions
@sunmy2019
add tearDown and clear module cache
ae45649
@sunmy2019
sunmy2019 requested review from brettcannon, encukou, ericsnowcurrently, ncoghlan and warsaw as code owners May 6, 2023 04:27
@bedevere-bot bedevere-bot mentioned this pull request May 6, 2023
test_pickle/test_pickletools fail when running tests sequentially. #103247
Closed
@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting review labels May 6, 2023
@sunmy2019 sunmy2019 added the skip news label May 6, 2023
@arhadthedev arhadthedev changed the title gh-103247: add tearDown and clear module cache gh-103247: add tearDown to test_loader and clear module cache May 6, 2023
@arhadthedev arhadthedev added topic-subinterpreters topic-importlib labels May 6, 2023
@itamaro

itamaro commented May 7, 2023

Copy link
Copy Markdown
Contributor

I was able to repro the reported failure on main, and confirmed this PR fixes it.

I'm not sure adding the tearDown is helpful though.
the uncache context manager uncaches the names both on enter and exit, and perform some checks on the names - two things that the tearDown doesn't perform.
while having the tearDown may prevent some failures (like this one), its existence might indicate that the uncache context manager is not really needed, which is not necessarily true (I don't know if it matters in practice, just noting the behavior differences).

in any case, I also confirmed that just adding the context manager to test_try_registration without the tearDown fixes the issue.

All reactions

Sorry, something went wrong.

Uh oh!

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

@sunmy2019

sunmy2019 commented May 7, 2023

Copy link
Copy Markdown
Member Author

I'm not sure adding the tearDown is helpful though.

Adding this is proposed by @FFY00. And I am neutral about it.

All reactions

Sorry, something went wrong.

Uh oh!

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

@sunmy2019

sunmy2019 commented May 7, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

I'm not sure adding the tearDown is helpful though.

I think you are correct, adding both is unnecessary.

tearDown method and util.uncache provides duplicate function. And util.uncache(name) provides more flexibility than the tearDown method.

tearDown here is a safety net. But do we want to expose the problem early or make the system more robust?

I prefer the former since it's a test case. Thus, I will remove this safe net for now.

itamaro reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@sunmy2019
remove tearDown
6fd7fec
itamaro
itamaro approved these changes May 7, 2023
View reviewed changes

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

LGTM!

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 May 7, 2023
@sunmy2019
sunmy2019 requested a review from FFY00 May 7, 2023 13:18
brettcannon
brettcannon approved these changes May 10, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels May 10, 2023
@brettcannon brettcannon added the needs backport to 3.11 only security fixes label May 10, 2023
@brettcannon
Merge branch 'main' into pythongh-103247
43ef567
@brettcannon
brettcannon enabled auto-merge (squash) May 10, 2023 00:35
@brettcannon
brettcannon merged commit 22f3425 into python:main May 10, 2023
@miss-islington

miss-islington commented May 10, 2023

Copy link
Copy Markdown
Contributor

Thanks @sunmy2019 for the PR, and @brettcannon for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

sunmy2019 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the awaiting merge label May 10, 2023
@bedevere-bot

bedevere-bot commented May 10, 2023

Copy link
Copy Markdown

GH-104345 is a backport of this pull request to the 3.11 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label May 10, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 10, 2023
@sunmy2019 @miss-islington
pythongh-103247: clear the module cache in a test in test_importlib/e…
06a0974
…xtensions/test_loader.py (pythonGH-104226)

(cherry picked from commit 22f3425)

Co-authored-by: sunmy2019 <[email protected]>
@sunmy2019
sunmy2019 deleted the gh-103247 branch May 10, 2023 03:40
JelleZijlstra pushed a commit that referenced this pull request May 10, 2023
@miss-islington @sunmy2019
[3.11] gh-103247: clear the module cache in a test in test_importlib/…
1cbf844
…extensions/test_loader.py (GH-104226) (#104345)

gh-103247: clear the module cache in a test in test_importlib/extensions/test_loader.py (GH-104226)
(cherry picked from commit 22f3425)

Co-authored-by: sunmy2019 <[email protected]>
carljm added a commit to carljm/cpython that referenced this pull request May 10, 2023
@carljm
Merge branch 'main' into fixloadsuperspec
5976df9
* main:
  pythonGH-102181: Improve specialization stats for SEND (pythonGH-102182)
  pythongh-103000: Optimise `dataclasses.asdict` for the common case (python#104364)
  pythongh-103538: Remove unused TK_AQUA code (pythonGH-103539)
  pythonGH-87695: Fix OSError from `pathlib.Path.glob()` (pythonGH-104292)
  pythongh-104263: Rely on Py_NAN and introduce Py_INFINITY (pythonGH-104202)
  pythongh-104010: Separate and improve docs for `typing.get_origin` and `typing.get_args` (python#104013)
  pythongh-101819: Adapt _io._BufferedIOBase_Type methods to Argument Clinic (python#104355)
  pythongh-103960: Dark mode: invert image brightness (python#103983)
  pythongh-104252: Immortalize Py_EMPTY_KEYS (pythongh-104253)
  pythongh-101819: Clean up _io windows console io after pythongh-104197 (python#104354)
  pythongh-101819: Harden _io init (python#104352)
  pythongh-103247: clear the module cache in a test in test_importlib/extensions/test_loader.py (pythonGH-104226)
  pythongh-103848: Adds checks to ensure that bracketed hosts found by urlsplit are of IPv6 or IPvFuture format (python#103849)
  pythongh-74895: adjust tests to work on Solaris (python#104326)
  pythongh-101819: Refactor _io in preparation for module isolation (python#104334)
  pythongh-90953: Don't use deprecated AST nodes in clinic.py (python#104322)
  pythongh-102327: Extend docs for "url" and "headers" parameters to HTTPConnection.request()
  pythongh-104328: Fix typo in ``typing.Generic`` multiple inheritance error message (python#104335)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brettcannon brettcannon brettcannon approved these changes
@itamaro itamaro itamaro approved these changes
@encukou encukou Awaiting requested review from encukou
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@ncoghlan ncoghlan Awaiting requested review from ncoghlan ncoghlan is a code owner
@warsaw warsaw Awaiting requested review from warsaw warsaw is a code owner
@FFY00 FFY00 Awaiting requested review from FFY00 FFY00 is a code owner

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-importlib topic-subinterpreters

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.

6 participants

@sunmy2019 @itamaro @miss-islington @bedevere-bot @brettcannon @arhadthedev
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.