Skip to content

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.

Sign up for GitHub

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
merged 10 commits into from
Nov 3, 2023
Merged

Reorganize test_util and make xfail marks precise #1729

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
09826f4
Revise test_util comment style
EliahKagan Oct 24, 2023
e2fa5e2
Pull rmtree tests out of TestUtils class
EliahKagan Oct 24, 2023
c4da058
Pull HIDE_WINDOWS_*_ERRORS tests out of TestUtils
EliahKagan Oct 24, 2023
aa1799f
Remove file_not_found_tmpdir fixture for TestRmtree
EliahKagan Oct 24, 2023
2fb8c64
Move permission_error_tmpdir skips to test cases
EliahKagan Oct 24, 2023
1dccb8e
Pull cygpath and decygpath tests out of TestUtils
EliahKagan Oct 24, 2023
487bc8a
Start making TestCygpath xfail markings granular
EliahKagan Oct 25, 2023
c3d3d1e
Finish making TestCygpath xfail markings granular
EliahKagan Oct 25, 2023
7d98f94
Let all TestRmtree tests run on 3.7
EliahKagan Oct 27, 2023
ba5bc45
Better explain the Windows and Unix cases
EliahKagan Oct 29, 2023
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
Pull cygpath and decygpath tests out of TestUtils 
This turns them into pure pytest tests, in a new class TestCygpath.
  • Loading branch information
@EliahKagan
EliahKagan committed Nov 3, 2023
commit 1dccb8e3a90ad2227d0689171526212cd5740451
98 changes: 51 additions & 47 deletions test/test_util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
import sys
import tempfile
import time
from unittest import SkipTest, mock, skipUnless
from unittest import SkipTest, mock

import ddt
import pytest
Expand Down Expand Up @@ -45,7 +45,7 @@

@pytest.fixture
def permission_error_tmpdir(tmp_path):
"""Fixture to test permissions errors situations where they are not overcome."""
"""Fixture to test permissions errors in situations where they are not overcome."""
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.

td.mkdir()
(td / "x").write_bytes(b"")
Expand Down Expand Up @@ -211,21 +211,9 @@ def test_env_vars_for_windows_tests(self, name, env_var_value, expected_truth_va
assert actual_parsed_value is expected_truth_value


class _Member:
"""A member of an IterableList."""

__slots__ = ("name",)

def __init__(self, name):
self.name = name

def __repr__(self):
return f"{type(self).__name__}({self.name!r})"


@ddt.ddt
class TestUtils(TestBase):
"""Tests for most utilities in :mod:`git.util`."""
@pytest.mark.skipif(sys.platform != "cygwin", reason="Paths specifically for Cygwin.")
class TestCygpath:
"""Tests for :func:`git.util.cygpath` and :func:`git.util.decygpath`."""

_norm_cygpath_pairs = (
(R"foo\bar", "foo/bar"),
Expand All @@ -248,54 +236,70 @@ class TestUtils(TestBase):
(R"\\?\UNC\server\D$\Apps", "//server/D$/Apps"),
)

# FIXME: Mark only the /proc-prefixing cases xfail, somehow (or fix them).
# FIXME: Mark only the /proc-prefixing cases xfail (or fix them).
@pytest.mark.xfail(
reason="Many return paths prefixed /proc/cygdrive instead.",
raises=AssertionError,
)
@skipUnless(sys.platform == "cygwin", "Paths specifically for Cygwin.")
@ddt.idata(_norm_cygpath_pairs + _unc_cygpath_pairs)
def test_cygpath_ok(self, case):
wpath, cpath = case
@pytest.mark.parametrize("wpath, cpath", _norm_cygpath_pairs + _unc_cygpath_pairs)
def test_cygpath_ok(self, wpath, cpath):
cwpath = cygpath(wpath)
self.assertEqual(cwpath, cpath, wpath)
assert cwpath == cpath, wpath

@pytest.mark.xfail(
reason=R'2nd example r".\bar" -> "bar" fails, returns "./bar"',
raises=AssertionError,
)
@skipUnless(sys.platform == "cygwin", "Paths specifically for Cygwin.")
@ddt.data(
(R"./bar", "bar"),
(R".\bar", "bar"), # FIXME: Mark only this one xfail, somehow (or fix it).
(R"../bar", "../bar"),
(R"..\bar", "../bar"),
(R"../bar/.\foo/../chu", "../bar/chu"),
@pytest.mark.parametrize(
"wpath, cpath",
[
(R"./bar", "bar"),
(R".\bar", "bar"), # FIXME: Mark only this one xfail (or fix it).
(R"../bar", "../bar"),
(R"..\bar", "../bar"),
(R"../bar/.\foo/../chu", "../bar/chu"),
],
)
def test_cygpath_norm_ok(self, case):
wpath, cpath = case
def test_cygpath_norm_ok(self, wpath, cpath):
cwpath = cygpath(wpath)
self.assertEqual(cwpath, cpath or wpath, wpath)
assert cwpath == (cpath or wpath), wpath

@skipUnless(sys.platform == "cygwin", "Paths specifically for Cygwin.")
@ddt.data(
R"C:",
R"C:Relative",
R"D:Apps\123",
R"D:Apps/123",
R"\\?\a:rel",
R"\\share\a:rel",
@pytest.mark.parametrize(
"wpath",
[
R"C:",
R"C:Relative",
R"D:Apps\123",
R"D:Apps/123",
R"\\?\a:rel",
R"\\share\a:rel",
],
)
def test_cygpath_invalids(self, wpath):
cwpath = cygpath(wpath)
self.assertEqual(cwpath, wpath.replace("\\", "/"), wpath)
assert cwpath == wpath.replace("\\", "/"), wpath

@skipUnless(sys.platform == "cygwin", "Paths specifically for Cygwin.")
@ddt.idata(_norm_cygpath_pairs)
def test_decygpath(self, case):
wpath, cpath = case
@pytest.mark.parametrize("wpath, cpath", _norm_cygpath_pairs)
def test_decygpath(self, wpath, cpath):
wcpath = decygpath(cpath)
self.assertEqual(wcpath, wpath.replace("/", "\\"), cpath)
assert wcpath == wpath.replace("/", "\\"), cpath


class _Member:
"""A member of an IterableList."""

__slots__ = ("name",)

def __init__(self, name):
self.name = name

def __repr__(self):
return f"{type(self).__name__}({self.name!r})"


@ddt.ddt
class TestUtils(TestBase):
"""Tests for most utilities in :mod:`git.util`."""

def test_it_should_dashify(self):
self.assertEqual("this-is-my-argument", dashify("this_is_my_argument"))
Expand Down