This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] Allow C:\System Volume Information to be missing
ClosedPublic

Authored by mstorsjo on Mar 19 2021, 9:27 AM.

Details

Summary

If running in a Windows Container, there is no such directory at all.

If running from within bash on Windows Server, the directory seems to
be fully accessible. (The mechanics of this isn't fully understood, and
it doesn't seem to happen on desktop versions.)

If the directory isn't available with the expected behaviour, mark those
individual tests as unsupported. (The test as a whole is considered to
pass, but the unsupported test is mentioned in a test summary printed on
stdout.)

Diff Detail

Event Timeline

mstorsjo requested review of this revision.Mar 19 2021, 9:27 AM
mstorsjo created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2021, 9:27 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added inline comments.
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.exists/exists.pass.cpp
93

What happens if you call TEST_UNSUPPORTED from within GetWindowsInaccessibleDir?

The current code smells very bad to me — the caller unsupports the test, but the callee fprintfs the warning message about how the test is going to be unsupported. These two behaviors should happen in the same place one way or another.

Have you already investigated and rejected whether there's a way to reliably create an inaccessible directory for testing, instead of relying on this pre-existing directory?

mstorsjo added inline comments.Mar 19 2021, 10:12 AM
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.exists/exists.pass.cpp
93

Yes we've had multiple persons look into options for what to use for inaccessible directory tests already in an earlier review thread. It's not entirely trivial.

You're not really supposed to call any of the TEST_* macros from within the support header. The TEST_* macros expand to a bit code that assumes that it's directly within the TEST_CASE() {} block. And not all callers use such a setup (as opposed to just a plain main() function with asserts). The TEST_UNSUPPORTED() macro just does some bookkeeping and then calls return; and, and a plain return; to stop the execution of the current test doesn't work from nested functions.

Yes it's not entirely pretty, but this way, the function explains exactly what/why was amiss regarding the test directory, regardless of what the caller will do with the information, and the caller will either mark the whole test as unsupported, or skip parts of it. E.g. in D98443, in temp_directory_path.pass.cpp, the right course of action is just to skip a small bit of the test, not bail out of the whole test.

Tangential nitpicks. No objections from me; but as I have no idea about Windows I'll leave it to someone else to click "accept."

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.exists/exists.pass.cpp
93

You're not really supposed to call any of the TEST_* macros from within the support header.

Ah, at least part of my issue was a brain fart. I forgot this wasn't actually Google Test. 😛

E.g. in D98443, in temp_directory_path.pass.cpp, the right course of action is just to skip a small bit of the test

Tangent: That sounds like the "small bit" should be pulled out into its own TEST_CASE.
Another tangent: Over in D98443 I see you using TEST_REQUIRE(!p.empty()), but here it's if (p.empty()) TEST_UNSUPPORTED(). What's the difference?

libcxx/test/support/filesystem_test_helper.h
704

Incidentally, that loop could be shortened as

for (const auto &ent : fs::directory_iterator(root, ec)) {
   ...
}

or, if we care so much about ec for some reason, then maybe we should be using it.increment(ec) instead of ++it on line 708.

mstorsjo added inline comments.Mar 19 2021, 12:13 PM
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.exists/exists.pass.cpp
93

Originally, these cases were added in D98166. Then we didn't know about many valid cases where the dir wouldn't exist or behave correctly - so we chose to make the dir a hard requirement. Then I posted D98443 adding more cases of the same. Now I found valid cases where we can't rely on this dir for the tests, hence this patch. When/if this one is settled, I'll update D98443 accordingly.

libcxx/test/support/filesystem_test_helper.h
704

Oh, thanks, I'll have a go at such a rewrite.

mstorsjo updated this revision to Diff 332009.Mar 19 2021, 2:50 PM

Rewrite the loop in GetWindowsInaccessibleDir as suggested. The diff overall is harder to read, but the code is neater in the end.

mstorsjo updated this revision to Diff 332456.Mar 22 2021, 3:46 PM

Updating XFAIL records, added a (void) cast to the fs::exists() return value to fix including the support header when building with MS STL (for reference testing), as the MS STL declares fs::exists() as nodiscard.

@curdeius - WDYT about this one? It unbreaks these tests in the container env used in the CI runners, while keeps the tests testing as much as possible if testing in another environment.

mstorsjo updated this revision to Diff 335325.Apr 5 2021, 2:27 PM

Rebased to rerun with CI - showing off that this allows removing 4 cases of XFAIL.

While it does weaken the strength of the tests somewhat, it does allow running 4 more tests in the CI configuration, which on the other hand strengthens testing.

Still looks plausible to me, mod comment.

libcxx/test/support/filesystem_test_helper.h
691–702

This codepath actually confuses me. It's new (appears on the right-hand side but not the left-hand side of the diff), and it seems to be checking that ent.exists() (line 685) and yet !fs::exists(ent) (line 692)? This could well be consistent with reality — I mean, I'd expect std::filesystem to do a terrible/weird job of handling visible-but-inaccessible paths — but could you please just manually triple-check that GetWindowsInaccessibleDir() does still return a non-empty path sometimes, and hasn't been completely nerfed by this patch?

mstorsjo added inline comments.Apr 5 2021, 11:07 PM
libcxx/test/support/filesystem_test_helper.h
691–702

Yep, it does return a non-empty path on regular desktop windows, just re-checked it.

This check is new here, but it's essentially the same check as we had in the fs.op.exists test already; if the directory doesn't fulfill that, we can't use it as intended.

When the tested process is launched from within bash, running on windows server as the administrator user, the directory is fully accessible (for unknown reasons), this case is caught by this clause.

It's a bit non-obvious, but yes, ent.exists() is true - when iterating over the parent directory, it does return an entry for the special directory, and the status flags we get from FindNextFile (used for iterating a directory) preliminarily indicate that it exists. Checking with fs::exists() then instead does the equivalent of stat() - in this case opening a handle with CreateFile(FILE_READ_ATTRIBUTES) and inspecting it with GetFileInformationByHandleEx() - and that fails as we're not allowed to read it (i.e. fs::exists() sets the error code or throws an exception, it doesn't signal a clean negative).

@curdeius Is this ok with you, to continue reducing the number of XFAILs (in particular within the filesystem tests)?

curdeius accepted this revision as: curdeius.Apr 6 2021, 2:24 AM

LGTM. It would be indeed cool to have a real inaccessible directory and not need to do these hacks, but well.
Please sync with @Quuxplusone if he has no more comments before landing (Arthur, I let you accept as libc++ then).

Concerning your question, I think it will be cool indeed to get rid of all (if possible) XFAILs on windows.

Quuxplusone accepted this revision.Apr 6 2021, 10:33 AM

My only ask was "manually triple-check", which sounds like it's been done, so, ship it!

This revision is now accepted and ready to land.Apr 6 2021, 10:33 AM