Skip to content

gh-131031: Fix separated running of pickle tests - #133218

Closed
donbarbos wants to merge 2 commits into
python:mainfrom
donbarbos:issue-131031
Closed

gh-131031: Fix separated running of pickle tests#133218
donbarbos wants to merge 2 commits into
python:mainfrom
donbarbos:issue-131031

Conversation

@donbarbos

@donbarbosdonbarbos commented Apr 30, 2025

Copy link
Copy Markdown
Contributor

I found that tests stopped running successfully directly after this commit (PR #122373)

@donbarbosdonbarbos changed the title Fix separated running of pickle testsgh-131031: Fix separated running of pickle testsApr 30, 2025
@donbarbos

Copy link
Copy Markdown
ContributorAuthor

cc @tomasr8 (sorry for choosing you but current pickle expert is inactive)

@tomasr8

Copy link
Copy Markdown
Member

Are you sure the assertions are being executed? Since self.dumps(obj, proto) raises, anything that comes after in the with block will not execute.

@donbarbos
donbarbos marked this pull request as draft May 1, 2025 21:43
@donbarbos

donbarbos commented May 3, 2025

Copy link
Copy Markdown
ContributorAuthor

@tomasr8 sorry for the first commit.
As far as I know, changing __main__ is bad practice, but it seems that this issue cannot be solved otherwise, since the problem is that __main__ is different for different launch methods, and I was inspired by a similar PR #131371

@donbarbos
donbarbos marked this pull request as ready for review May 3, 2025 17:02
@tomasr8

Copy link
Copy Markdown
Member

This looks ok, but I'm not expert in his area :/ @serhiy-storchaka could you have a look?

@jasonzitao

This comment was marked as abuse.

@jasonzitao

This comment was marked as abuse.

@serhiy-storchakaserhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good PR, but using a global class which is not imported in test_pickle.py instead of AbstractPickleTests would fix the tests too. See #133356.

Faking the __main__ module looks heavyweight, we don't know what side effects can it have. We would use this approach if there was no other way.

real_main = sys.modules.get("__main__")
fake_main = types.ModuleType("__main__")

sys.modules["__main__"] = fake_main

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can use support.swap_item(). It is already used in other tests to fake a module.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@donbarbos@tomasr8@jasonzitao@serhiy-storchaka