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

grass.gunittest: Fix parsing exclusion from config file on Windows #4324

Merged
merged 13 commits into from
Sep 18, 2024

Conversation

echoix
Copy link
Member

@echoix echoix commented Sep 15, 2024

Description

Refactored the function fnmatch_exclude_with_base in grass.gunittest.loader in order to work across platforms, including Windows.

I tried multiple approaches and researched a bit, and the first really working solution was the custom fnmatch_ex function that pytest uses, since pathlib's Path.match() doesn't work properly and doesn't behave like Path.glob() concerning **s.
https://github.com/pytest-dev/pytest/blob/9515dfa58a144f3644fd29b256113d723c9c1955/src/_pytest/pathlib.py#L414-L447

def fnmatch_ex(pattern: str, path: str | os.PathLike[str]) -> bool:
    """A port of FNMatcher from py.path.common which works with PurePath() instances.

    The difference between this algorithm and PurePath.match() is that the
    latter matches "**" glob expressions for each part of the path, while
    this algorithm uses the whole path instead.

    For example:
        "tests/foo/bar/doc/test_foo.py" matches pattern "tests/**/doc/test*.py"
        with this algorithm, but not with PurePath.match().

    This algorithm was ported to keep backward-compatibility with existing
    settings which assume paths match according this logic.

    References:
    * https://bugs.python.org/issue29249
    * https://bugs.python.org/issue34731
    """
    path = PurePath(path)
    iswin32 = sys.platform.startswith("win")

    if iswin32 and sep not in pattern and posix_sep in pattern:
        # Running on Windows, the pattern has no Windows path separators,
        # and the pattern has one or more Posix path separators. Replace
        # the Posix path separators with the Windows path separator.
        pattern = pattern.replace(posix_sep, sep)

    if sep not in pattern:
        name = path.name
    else:
        name = str(path)
        if path.is_absolute() and not os.path.isabs(pattern):
            pattern = f"*{os.sep}{pattern}"
    return fnmatch.fnmatch(name, pattern)

However, after inlining more and more of that fnmatch_ex function, comparing how it behaves with our inputs (Windows and Linux) and what branches are taken, we don't use a mix of absolute and relative paths, and we never supported **, and we never only supplied a name without separator. So, since on Windows the working fnmatch_ex needs a pattern with backslashes \ for the path and the pattern, and on Linux it needs forward slash on both; since pathlib's Path representation already normalizes a ./ of a relative path; and since pathlib's Path/PurePath classes works properly with * and **, especially when that variable is converted to a string at the end, I could simply replace all our logic by creating Path instances for the full name and pattern, and call the fnmatch with the string representations of it. I used PurePath instead of a concrete Path, as I didn't need to actually manipulate files, and the subclasses can be instantiated on any OS if further tests would need to run cross-platform.

Also included:

  • the files list variable in discover_modules could not exist in all code paths, depending on the inputs. Initialized it with a list containing the same elements as all_files, without directly assigning all_files to files.

Motivation and context

Fixes #4319

How has this been tested?

Up until my final clean rebase, I was using extra prints inside the function, to not have to wait at the end of the test run to see if the files were properly excluded.

    print(f"Ed: fnmatch_exclude_with_base_pathlib, exclude: {exclude}")
    print(f"Ed: fnmatch_exclude_with_base_pathlib, patterns: {patterns}")
    print(f"Ed: fnmatch_exclude_with_base_pathlib, base: {base}")
    print(f"Ed: fnmatch_exclude_with_base_pathlib, base_path: {base_path}")
    print(f"Ed: fnmatch_exclude_with_base_pathlib, files: {files}")
    print(f"Ed: fnmatch_exclude_with_base_pathlib, not_excluded: {not_excluded}")

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing
    functionality to not work as before)

Checklist

  • My code follows the code style
    of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@echoix echoix added this to the 8.5.0 milestone Sep 15, 2024
@github-actions github-actions bot added Python Related code is in Python libraries labels Sep 15, 2024
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

Using pathlib is definitively better!

python/grass/gunittest/loader.py Outdated Show resolved Hide resolved
python/grass/gunittest/loader.py Outdated Show resolved Hide resolved
python/grass/gunittest/loader.py Show resolved Hide resolved
python/grass/gunittest/loader.py Outdated Show resolved Hide resolved
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

I really meant to request changes, not approve.

python/grass/gunittest/loader.py Outdated Show resolved Hide resolved
@echoix echoix merged commit 9420d00 into OSGeo:main Sep 18, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libraries Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Gunittest doesn't respect exclusions in .gunittest.cfg on Windows
2 participants