Skip to content

gh-37883: Safely skip test_resource file size tests when limits are strict#145579

Open
Shrey-N wants to merge 16 commits intopython:mainfrom
Shrey-N:fixy-branch
Open

gh-37883: Safely skip test_resource file size tests when limits are strict#145579
Shrey-N wants to merge 16 commits intopython:mainfrom
Shrey-N:fixy-branch

Conversation

@Shrey-N
Copy link
Copy Markdown
Contributor

@Shrey-N Shrey-N commented Mar 6, 2026 •
edited by bedevere-app bot
Loading

This fixes the Standing issue #37883 where test_resource would fail on systems with file size limitations

Previous attempts (like #140872) tried to introduce massive refactoring and helper classes. Based on reviewer feedback I read there, I have tried a minimal fix

  • It checks if the system's hard limit is large enough, and if the OS actually allows the limit to be modified. If not, it uses self.skipTest with an informative message rather than failing with an unhandled OSError or ValueError.
  • It replaces the fragile dual try...finally resource restoration blocks with self.addCleanup(), guaranteeing that limits are reset even during a test assertion failure.

Fixes #37883

@bedevere-app bedevere-app bot added the tests Tests in the Lib/test dir label Mar 6, 2026
@python-cla-bot
Copy link
Copy Markdown

python-cla-bot bot commented Mar 6, 2026 •
edited
Loading

All commit authors signed the Contributor License Agreement.

CLA signed

@bedevere-app bedevere-app bot mentioned this pull request Mar 6, 2026
Open
@Shrey-N
Copy link
Copy Markdown
Contributor Author

Shrey-N commented Mar 6, 2026

I have added a news entry, though I am not sure if it's necessary or for this fix, Please let me know if I should remove it :)

encukou
encukou reviewed Mar 10, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

This looks good!

We don't need a NEWS entry for a test-only change.

@Shrey-N
Copy link
Copy Markdown
Contributor Author

Shrey-N commented Mar 10, 2026

@encukou Alrighty thank you, I was confused as to add it or not so thank you for clearing that up :), also I will incorporate your reviews in a second :)

@encukou
Copy link
Copy Markdown
Member

encukou commented Mar 13, 2026

I think you can roll back the last 4 commits.

In d21ceb8, the removed comment says:

            # Close will attempt to flush the byte we wrote
            # Restore limit first to avoid getting a spurious error

That should stay, i.e. the try/finally should go inside the with, not the other way around.

@Shrey-N
Copy link
Copy Markdown
Contributor Author

Shrey-N commented Mar 13, 2026

Alright will look into it asap

@Shrey-N
Copy link
Copy Markdown
Contributor Author

Shrey-N commented Mar 15, 2026 •
edited
Loading

@encukou I believe everything should work now :) Sorry for the late reply

@Shrey-N
Copy link
Copy Markdown
Contributor Author

Shrey-N commented Mar 29, 2026

@encukou Hi, I believe I fixed the tests that were failing from my side in this PR, I am pretty sure this time, it's not my fault :) lmk if I am to make any other changes :)

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

Reviewers

@encukou encukou encukou left review comments

Assignees

No one assigned

Labels

awaiting review skip news tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

test_resource fails when file size is limited

2 participants

@Shrey-N @encukou