Tests fail on windows because of newline

[OrangeUtan]

Setup:

• Fork current main branch

$ git clone https://github.com/OrangeUtan/beet
$ poetry install
$ poetry run pytest -vv

Excerpt:

snapshot = snapshot, directory = 'load_templates_plugin'
@pytest.mark.parametrize("directory", os.listdir("tests/examples"))  # type: ignore
    def test_build(snapshot: Any, directory: str):
        with run_beet(directory=f"tests/examples/{directory}") as ctx:
>           assert snapshot("data_pack") == ctx.data

⠇

E                   Drill down into differing attribute content:
E                     content: 'say hello world\r\n' != 'say hello world\n'
E                     - say hello world
E                     + say hello world
E                     ?                +
tests\test_examples.py:12: AssertionError

⠇

36 failed, 59 passed, 8 skipped in 4.06s

Tests seem to fail because of the differences of newlines: Windows \r\n vs Linux \n.

Since the project uses snapshot testing, I don't know how easily that can be fixed. In the case that it can, I recommend adding a matrix to the Github Action (an example from my repo)

[vberlier]

Good catch! File handles are lazy and don't load or parse their content unless absolutely necessary so right now when comparing files for equality I'm using their serialized representation. One change I actually made a few days ago was implementing comparisons using the deserialized value for json files as I was running into a different field ordering in CI. But it's still not ideal so I just made the __eq__ implementation a bit smarter. There's a new fast path when both files have the same source_path that avoids unnecessary loads, and the comparison falls back to the deserialized value if the serialized representation wasn't the same.

This should take care of the inconsistency with line endings on windows. Other than that there's not really any os-specific behavior, so I'm probably not going to start running tests on windows. I really like to keep my CI as fast as possible so unless I actually have platform-specific code in a project I try to keep it linux-only.

Tell me if you encounter any other issue with this.

[OrangeUtan]

In that case I'm going to play the part of the botnet matrix. Call me Neo :D

[OrangeUtan]

The saga continues.
The newline fix worked, no tests fail because of it anymore (from 36 failing down to 12).
But the second Windows/Linux differences are the paths, \ vs /:

Excerpt:

snapshot = snapshot, directory = 'whitelist_nested'

    @pytest.mark.parametrize("directory", os.listdir("tests/config_examples"))  # type: ignore
    def test_config_resolution(snapshot: Any, directory: str):
        project_config = load_config(f"tests/config_examples/{directory}/beet.json")
>       assert snapshot("json") == project_config.dict()

⠇

                 'description': '',
E         -                'directory': 'tests\\config_examples\\whitelist_nested',
E         ?                                   ^^               ^^
E         +                'directory': 'tests/config_examples/whitelist_nested',

⠇

tests\test_config.py:12: AssertionError
12 failed, 83 passed, 8 skipped in 4.86s

Also a quirk I noticed with OSX in my own repos: Apple seems to have a different file ordering, had an issue where I assumed the same order of reading files on every OS.

[OrangeUtan]

Issue seems to be the ProjectConfig class. Its directory attribute seems to be stored as a string, therefore the platform dependent paths. Idealy this could be fixed by changing the attribute to a pathlib.Path. However, since this is only a problem that occurs during tests on Windows, that seems overkill.

I quickly tested with a hacky fix that just converts all path strings to posix path strings:

def fix_paths(cfg: ProjectConfig):
    if cfg.directory:
        cfg.directory = Path(cfg.directory).as_posix()
    if cfg.output:
        cfg.output = Path(cfg.output).as_posix()

    cfg.data_pack.load = [Path(p).as_posix() for p in cfg.data_pack.load]
    cfg.resource_pack.load = [Path(p).as_posix() for p in cfg.resource_pack.load]

    if cfg.templates:
        cfg.templates = [Path(p).as_posix() for p in cfg.templates]

    for entry in cfg.pipeline:
        if isinstance(entry, ProjectConfig):
            fix_paths(entry)

    return cfg


@pytest.mark.parametrize("directory", os.listdir("tests/config_examples"))  # type: ignore
def test_config_resolution(snapshot: Any, directory: str):
    project_config = load_config(f"tests/config_examples/{directory}/beet.json")
    fix_paths(project_config)
    assert snapshot("json") == project_config.dict()

That seems to fix all the problems with Paths.
But Windows is a vile beast that doesn't go down with even 2 hits:

tmp_path = WindowsPath('C:/Users/Michael/AppData/Local/Temp/pytest-of-Michael/pytest-55/test_match0')

def test_match(tmp_path: Path):        
        with MultiCache(tmp_path) as cache:
>           cache["test:foo"]

tests\test_cache.py:155:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
beet\core\container.py:132: in __getitem__
    value = self.missing(key)
beet\core\cache.py:179: in missing
    cache = Cache(self.path / key)
beet\core\cache.py:35: in __init__
    if self.index_path.is_file()
C:\Python39\lib\pathlib.py:1439: in is_file
    return S_ISREG(self.stat().st_mode)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
self = WindowsPath('C:/Users/Michael/AppData/Local/Temp/pytest-of-Michael/pytest-56/test_match0/test:foo/index.json')

E       OSError: [WinError 123] Die Syntax für den Dateinamen, Verzeichnisnamen oder die Datenträgerbezeichnung ist falsch: 'C:\\Users\\Michael\\AppData\\Local\\Temp\\pytest-of-Michael\\pytest-56\\test_match0\\test:foo\\index.json

Now Windows personally complains (thats why the error is in German, sry). Seems the cache checks 'C:/Users/Michael/AppData/Local/Temp/pytest-of-Michael/pytest-56/test_match0/test:foo/index.json' with path.is_file(). But : is not allowd in file names on Windows, thus the cache file test:foo fails

[vberlier]

Whoa thanks! Sorry I was busy. I didn't anticipate this but you're right, resolving the config file produces different paths on windows. I'm fine with the fix_paths function but instead of doing the adjustments before comparing the snapshot, I ended up handling this in a custom snapshot format that converts the paths to the current platform when the snapshot gets loaded d4d3f91

I also changed the cache matching test to use test-foo instead of test:foo. While I was at it I set up the project on windows on my end, and it looks like everything's working fine now, so I'm gonna close the issue but again let me know if anything else comes up.

[OrangeUtan]

Great stuff, tests work like a charm now. Thanks for all the effort :D