Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-116057: Use relative recursion limits when testing os.walk and Path.walk #116058

Merged
merged 2 commits into from
Mar 10, 2024

Conversation

mhsmith
Copy link
Member

@mhsmith mhsmith commented Feb 28, 2024

@aisk
Copy link
Contributor

aisk commented Feb 29, 2024

This is a change for the test, therefore as the devguide said, the news entry is not needed.

@mhsmith
Copy link
Member Author

mhsmith commented Feb 29, 2024

There are already quite a few news entries in the "Tests" section for the next 3.13 alpha, so I'm not clear what the convention is here.

@aisk
Copy link
Contributor

aisk commented Mar 1, 2024

Ah, you are right, so I'm not clear with this too now. 😂

Hi @erlend-aasland, could you please help us with this convention? Should we add a news entry for a test case fix or enhancement?

@erlend-aasland
Copy link
Contributor

Normally, we don't require news entries for test changes, unless they are substansive.

@mhsmith
Copy link
Member Author

mhsmith commented Mar 1, 2024

OK, I've removed it.

@mhsmith
Copy link
Member Author

mhsmith commented Mar 9, 2024

@barneygale: Are you able to review this? It's a very simple change.

@erlend-aasland
Copy link
Contributor

Looks good to me. Let's give @barneygale some days to chime in, before landing this.

Copy link
Contributor

@barneygale barneygale left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. This seems fine, thank you.

@barneygale
Copy link
Contributor

barneygale commented Mar 9, 2024

(not a blocking issue:) I suppose infinite_recursion() is normally used to increase the recursion limit? It feels just a touch misleading to be calling it to reduce the recursion limit. Or perhaps I've misunderstood.

@mhsmith
Copy link
Member Author

mhsmith commented Mar 9, 2024

It's not a very accurate name, but it's already used in this way in many other places, so if we want to improve that I think it would be better done in a separate PR.

@erlend-aasland
Copy link
Contributor

@barneygale, @mhsmith: perhaps also consider consolidating the test setup of test_glob_above_recursion_limit and test_walk_above_recursion_limit in a test helper.

@barneygale
Copy link
Contributor

@erlend-aasland I don't mind too much either way, but there's a decent argument that violating DRY in test code is a Good Thing: https://mtlynch.io/good-developers-bad-tests/

@erlend-aasland erlend-aasland merged commit 2339e7c into python:main Mar 10, 2024
32 checks passed
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Ubuntu NoGIL 3.x has failed when building commit 2339e7c.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/1225/builds/1698) and take a look at the build logs.
  4. Check if the failure is related to this commit (2339e7c) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/1225/builds/1698

Failed tests:

  • test.test_multiprocessing_spawn.test_manager

Failed subtests:

  • test_mymanager_context_prestarted - test.test_multiprocessing_spawn.test_manager.WithManagerTestMyManager.test_mymanager_context_prestarted

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.nogil/build/Lib/test/_test_multiprocessing.py", line 3030, in test_mymanager_context_prestarted
    self.assertEqual(manager._process.exitcode, 0)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: -15 != 0

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
… and Path.walk() (python#116058)

Replace test.support.set_recursion_limit with test.support.infinite_recursion.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
… and Path.walk() (python#116058)

Replace test.support.set_recursion_limit with test.support.infinite_recursion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_walk_above_recursion_limit uses absolute limits
5 participants