Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Reorganize test_util and make xfail marks precise #1729

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

Merged
Byron merged 10 commits into gitpython-developers:main from EliahKagan:xfail
Nov 3, 2023
Merged
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Move permission_error_tmpdir skips to test cases 
This moves test skipping logic from the permission_error_tmpdir
fixture to the two TestRmtree test case methods that use it.
This produces some duplication, which is undesirable, but it makes
it so that which tests are skipped under what conditions is
immediately clear, easy to identify as skipping logic (which is
usually achieved by decoration), and clear in its relationship
to the skipping logic associated with other TestRmtree test cases.
  • Loading branch information
EliahKagan committed Nov 3, 2023
commit 2fb8c64a898e4c4bf93febadffa216c8a6ff2411
21 changes: 16 additions & 5 deletions test/test_util.py
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,6 @@
@pytest.fixture
def permission_error_tmpdir(tmp_path):
"""Fixture to test permissions errors situations where they are not overcome."""
if sys.platform == "cygwin":
raise SkipTest("Cygwin can't set the permissions that make the test meaningful.")
if sys.version_info < (3, 8):
raise SkipTest("In 3.7, TemporaryDirectory doesn't clean up after weird permissions.")

td = tmp_path / "testdir"
Copy link
Member

Choose a reason for hiding this comment

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

I keep choking on this as it seems more fragile than it has to be given that one could also create a tempdir with a unique name as well.

Copy link
Member Author

@EliahKagan EliahKagan Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can you tell me more about this concern? I'll be working on test_util.py again soon, so I'll have a good opportunity to make further improvements. However, tmp_path is already always unique: it's a Path object for a path like /tmp/hw9aphg (or a more systematically named path, but uniquely picked by pytest), not /tmp itself.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, perfect, then there is no concern, I just wasn't aware and too overwhelmed/not very focussed at the time.

Copy link
Member Author

@EliahKagan EliahKagan Nov 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

then there is no concern

Ah, excellent.

Some of the code this PR modifies are the tests I introduced it in #1700. Both before and after the changes here, these tests may give the mistaken impression that variables like td refer to objects representing paths like /tmp/tmpdir that are always the same, rather than paths like /tmp/hw9aphg/tmpdir (or, with pytest, a more systematic scheme but still one that makes sure to use an initially nonexistent location).

After all, if tmp_path really corresponds to a unique and newly created directory, why not use it rather than creating a tmp_path / "testdir" subdirectory?

I do this for several reasons:

  • I think of tmp_path as a resource the testing framework and/or custom fixtures manage: it is created before the test and (where appropriate and feasible) removed after the test. So for testing the behavior of git.util.rmtree in deleting a directory, I make a subdirectory for that, even if I wouldn't have to.
  • Currently, I have configured pytest not to attempt to delete the temporary directory it creates afterwards, if the test failed. This allows the directory to be examined to troubleshoot a regression that causes a failure. But this would be confusing to actually do, if there is confusion between the pytest-managed temporary directory that may be deleted if the test succeeds, and the directory that the code under test attempts to delete.
  • In at least one test, conceptually zero directories are created, and the purpose of the temporary directory is to ensure that it doesn't have an entry of a particular name in it, so that this nonexistent entry can itself be used, to test what happens when the code under test raises a FileNotFoundError.
  • In some forthcoming tests I plan to propose soon (but that I've had in mind for a short while, including when working on this PR), two directories will be created. Making them subdirectories of the tmp_path directory will make the test code a bit simpler, as I won't have to use a more versatile but complex fixture to create multiple pytest-managed temporary directories. More importantly, it will allow the two directories being affected by git.util.rmtree to be distinguished from each other when examining the tests and their results, rather than having them both have meaningless names that change each time.

This may be insufficiently clear in reading the test code, and if so, I would be interested in improving the situation, perhaps by changing some variable names, adding/expanding some docstrings or comments, or both. But maybe that wouldn't help; I'm not sure.

(Edit: Regarding the broader issue of test-runner parallelism, I've posted a separate top-level comment below.)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation! Personally I think that it's OK to leave it as is but refactor the next time one stumbles over it. Please note that I didn't read the top-level comment yet, to which I will probably reply separately.

EliahKagan reacted with thumbs up emoji
td.mkdir()
(td / "x").write_bytes(b"")
Expand Down Expand Up @@ -107,6 +102,14 @@ def test_deletes_dir_with_readonly_files(self, tmp_path):

assert not td.exists()

@pytest.mark.skipif(
sys.platform == "cygwin",
reason="Cygwin can't set the permissions that make the test meaningful.",
)
@pytest.mark.skipif(
sys.version_info < (3, 8),
reason="In 3.7, TemporaryDirectory doesn't clean up after weird permissions.",
)
def test_wraps_perm_error_if_enabled(self, mocker, permission_error_tmpdir):
"""rmtree wraps PermissionError when HIDE_WINDOWS_KNOWN_ERRORS is true."""
# Access the module through sys.modules so it is unambiguous which module's
Expand All @@ -122,6 +125,14 @@ def test_wraps_perm_error_if_enabled(self, mocker, permission_error_tmpdir):
with pytest.raises(SkipTest):
rmtree(permission_error_tmpdir)

@pytest.mark.skipif(
sys.platform == "cygwin",
reason="Cygwin can't set the permissions that make the test meaningful.",
)
@pytest.mark.skipif(
sys.version_info < (3, 8),
reason="In 3.7, TemporaryDirectory doesn't clean up after weird permissions.",
)
def test_does_not_wrap_perm_error_unless_enabled(self, mocker, permission_error_tmpdir):
"""rmtree does not wrap PermissionError when HIDE_WINDOWS_KNOWN_ERRORS is false."""
# See comments in test_wraps_perm_error_if_enabled for details about patching.
Expand Down

AltStyle によって変換されたページ (->オリジナル) /