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-59215: unittest: _top_level_dir is incorrectly persisted #15242

Merged

Conversation

ZackerySpytz
Copy link
Contributor

@ZackerySpytz ZackerySpytz commented Aug 13, 2019

Co-Authored-By: Igor Sobreira <igor@igorsobreira.com>
sys.path[:] = original_sys_path
self.addCleanup(restore)

os.path.isfile = lambda path: True
Copy link
Member

Choose a reason for hiding this comment

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

Is unittest.mock.patch a better option here instead of addCleanup?

def test_discover_should_not_persist_top_level_dir_between_calls(self):
    with unittest.mock.patch('os.path.isfile', return_value=True):
        with unittest.mock.patch('os.path.isdir', return_value=True):
            loader = unittest.TestLoader()
            loader.suiteClass = str
            dir = '/foo/bar'
            top_level_dir = '/foo'

            loader.discover(dir, top_level_dir=top_level_dir)
            self.assertEqual(loader._top_level_dir, None)

            loader._top_level_dir = dir2 = '/previous/dir'
            loader.discover(dir, top_level_dir=top_level_dir)
            self.assertEqual(loader._top_level_dir, dir2)

Copy link
Member

Choose a reason for hiding this comment

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

Nowadays that can be self.enterContext. But, as is, the PR is consistent with other nearby tests; if this needs changing it would be best to change all of them (in a separate PR).

@erlend-aasland erlend-aasland changed the title bpo-15010, unittest: _top_level_dir is incorrectly persisted gh-59215: unittest: _top_level_dir is incorrectly persisted Jan 5, 2024
@erlend-aasland erlend-aasland added needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Jan 5, 2024
Copy link
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.

IMO, this is the right solution, even if _top_level_dir isn't perfect.
However, as evidenced by the changed test, this is a change in behaviour. IMO, it needs at least a versionchanged note in the docs, saying that top_level_dir is only stored for the duration of the discover call.

I've sent proposed docs changes as a PR to this PR, here: https://github.com/ZackerySpytz/cpython/pull/39/files [now merged here]
If an unittest expert has time to look over them, that would be great :)

sys.path[:] = original_sys_path
self.addCleanup(restore)

os.path.isfile = lambda path: True
Copy link
Member

Choose a reason for hiding this comment

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

Nowadays that can be self.enterContext. But, as is, the PR is consistent with other nearby tests; if this needs changing it would be best to change all of them (in a separate PR).

@encukou encukou merged commit fc5f68e into python:main Apr 3, 2024
33 checks passed
@miss-islington-app
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 3, 2024
…ythonGH-15242)

(cherry picked from commit fc5f68e)

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 3, 2024
…ythonGH-15242)

(cherry picked from commit fc5f68e)

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Apr 3, 2024

GH-117508 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Apr 3, 2024
@bedevere-app
Copy link

bedevere-app bot commented Apr 3, 2024

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

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Apr 3, 2024
encukou added a commit that referenced this pull request Apr 23, 2024
…GH-15242) (GH-117508)

* gh-59215: unittest: restore _top_level_dir at end of discovery (GH-15242)
(cherry picked from commit fc5f68e)


Co-authored-by: Zackery Spytz <zspytz@gmail.com>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants