tests/test_types.py::test_file_surrogates test is missing the skipif not _non_utf8_filenames_supported marker

[RuRo]

The title says it all. There are a bunch of tests in tests/test_types.py that do some checks with non-utf8 filenames. Some filesystems don't support non-utf8 filenames, so the main test_path_surrogates test is conditionally disabled with

@pytest.mark.skipif(
    not _non_utf8_filenames_supported(),
    reason="The current OS or FS doesn't support non-UTF-8 filenames.",
)

However, the next two tests (test_file_surrogates and test_file_error_surrogates) are missing these marks.

The fix should be pretty simple - add the above-mentioned decorator to these tests as well.

[j7an]

@RuRo Are the two tests still failing on your system? @NickCao had fixed your previous issue by disabling test_file_surrogates for NixOS. Please add the test outputs of your system so we can triage further.

[RuRo]

Are the two tests still failing on your system?

I haven't tested this recently, but I see no reason why the results would change since the tests weren't fixed.

NickCao had fixed your previous issue by disabling test_file_surrogates for NixOS.

The problem isn't specific to NixOS, but is instead specific to the filesystem (in my case it's ZFS with utf8only, but iirc there are other filesystems with similar restrictions). Ideally, downstream source distributions shouldn't have to disable broken/flaky tests and you already have the appropriate skipif conditional implemented (not _non_utf8_filenames_supported()), it was just accidentally skipped for these two tests.

Please add the test outputs of your system so we can triage further.

You can see an example of the test outputs in the linked nixpkgs issue.

[davidism]

The problem is that the skip check seems to trigger for MacOS, but the two tests that are not decorated right now do pass on MacOS. We didn't "accidentally" skip them, there's no need for it on all the platforms (Windows, Mac, Linux) we can test on GitHub workflows. We can't just guess at these things, why does one test fail but others don't with the current setup?

[davidism]

Ideally, downstream source distributions shouldn't have to disable broken/flaky tests and you already

I disagree in general, downstreams that have their own requirements need to make calls on what those requirements mean versus what test failures in the libraries they package mean. That's not up to upstream.

[RuRo]

Ideally, downstream source distributions shouldn't have to disable broken/flaky tests and you already

I disagree in general, downstreams that have their own requirements need to make calls on what those requirements mean versus what test failures in the libraries they package mean. That's not up to upstream.

You seem to be under the impression that this issue is something specific to NixOS/nixpkgs ("downstreams that have their own requirements"). It is not. Nixpkgs is just a build system/package manager.

The build fails on my system, because I choose to use ZFS as my filesystem (unless you are saying that click just doesn't support ZFS). The same tests would also fail if I ran them directly from source/github or even if I was using a completely different distribution/package manager (eg Ubuntu). As long as my filesystem doesn't accept non-utf8 filenames, these tests should fail.

IMHO, it doesn't make sense for nixpkgs to have to disable these tests (since the nixpkgs/NixOS maintainers didn't do anything to break them).

The problem is that the skip check seems to trigger for MacOS, but the two tests that are not decorated right now do pass on MacOS.

Ah, I see, I wasn't aware of that. Unfortunately, I don't have access to a Mac, so I can only speculate about the reasons behind these differences. I seem to recall that some apple filesystems also have utf8 filename restrictions (similar to ZFS). It's possible that there are some edge cases where MacOS and ZFS handle invalid filenames a bit differently leading to this issue. Another possibility is that since these tests are only creating invalid files inside /tmp, the tests might pass on MacOS if /tmp is mounted as a tmpfs (which does support non-utf8 filenames) instead of /tmp being a regular directory inside the root filesystem (and thus subject to all of its restrictions).

Unfortunately, I am a bit busy right now, so I don't have the time to investigate this too thoroughly. Hopefully, I'll have a little bit more time on the weekend.

[RuRo]

Actually, looking at the failing test outputs a bit closer, the issue might be simply that the pytest.raises block inside test_file_surrogates is expecting a specific error to occur (No such file or directory aka ENOENT aka Errno 3), but the exact error depends on the filesystem. In the case of ZFS the error message would be Invalid or incomplete multibyte or wide character aka EILSEQ aka Errno 84.