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

[mstorsjo]

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.)

[• Quuxplusone]

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]

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.

[• Quuxplusone]

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]

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]

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

[mstorsjo]

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.

[mstorsjo]

@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]

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.

[• Quuxplusone]

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]

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).

[mstorsjo]

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

[curdeius]

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]

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

libcxx/test/std/input.output/filesystems/class.directory_entry/directory_entry.cons/path.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03
 
-// XFAIL: LIBCXX-WINDOWS-FIXME
-
 // <filesystem>
 
 // class directory_entry
 
 // explicit directory_entry(const path);
 // directory_entry(const path&, error_code& ec);
 
 #include "filesystem_include.h"

 
 TEST_CASE(path_ctor_cannot_resolve) {
   using namespace fs;
 #ifdef _WIN32
   // Windows doesn't support setting perms::none to trigger failures
   // reading directories; test using a special inaccessible directory
   // instead.
   const path dir = GetWindowsInaccessibleDir();
-  TEST_REQUIRE(!dir.empty());
+  if (dir.empty())
+    TEST_UNSUPPORTED();
   const path file = dir / "file";
   {
     std::error_code ec = GetTestEC();
     directory_entry ent(file, ec);
     TEST_CHECK(ErrorIs(ec, std::errc::no_such_file_or_directory));
     TEST_CHECK(ent.path() == file);
   }
   {

libcxx/test/std/input.output/filesystems/class.directory_iterator/directory_iterator.members/ctor.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03
 
-// XFAIL: LIBCXX-WINDOWS-FIXME
-
 // <filesystem>
 
 // class directory_iterator
 
 // explicit directory_iterator(const path& p);
 // directory_iterator(const path& p, directory_options options);
 // directory_iterator(const path& p, error_code& ec);
 // directory_iterator(const path& p, directory_options options, error_code& ec);

 TEST_CASE(access_denied_test_case)
 {
     using namespace fs;
 #ifdef _WIN32
     // Windows doesn't support setting perms::none to trigger failures
     // reading directories; test using a special inaccessible directory
     // instead.
     const path testDir = GetWindowsInaccessibleDir();
-    TEST_REQUIRE(!testDir.empty());
+    if (testDir.empty())
+        TEST_UNSUPPORTED();
 #else
     scoped_test_env env;
     path const testDir = env.make_env_path("dir1");
     path const testFile = testDir / "testFile";
     env.create_dir(testDir);
     env.create_file(testFile, 42);
 
     // Test that we can iterator over the directory before changing the perms

libcxx/test/std/input.output/filesystems/class.rec.dir.itr/rec.dir.itr.members/ctor.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03
 
-// XFAIL: LIBCXX-WINDOWS-FIXME
-
 // <filesystem>
 
 // class directory_iterator
 
 //
 // explicit recursive_directory_iterator(const path& p);
 // recursive_directory_iterator(const path& p, directory_options options);
 // recursive_directory_iterator(const path& p, error_code& ec);

 TEST_CASE(access_denied_test_case)
 {
     using namespace fs;
 #ifdef _WIN32
     // Windows doesn't support setting perms::none to trigger failures
     // reading directories; test using a special inaccessible directory
     // instead.
     const path testDir = GetWindowsInaccessibleDir();
-    TEST_REQUIRE(!testDir.empty());
+    if (testDir.empty())
+        TEST_UNSUPPORTED();
 #else
     scoped_test_env env;
     path const testDir = env.make_env_path("dir1");
     path const testFile = testDir / "testFile";
     env.create_dir(testDir);
     env.create_file(testFile, 42);
 
     // Test that we can iterator over the directory before changing the perms

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

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03
 
-// XFAIL: LIBCXX-WINDOWS-FIXME
-
 // <filesystem>
 
 // bool exists(file_status s) noexcept
 // bool exists(path const& p);
 // bool exists(path const& p, std::error_code& ec) noexcept;
 
 #include "filesystem_include.h"
 #include <type_traits>

 
 TEST_CASE(test_exists_fails)
 {
 #ifdef _WIN32
     // Windows doesn't support setting perms::none to trigger failures
     // reading directories; test using a special inaccessible directory
     // instead.
     const path p = GetWindowsInaccessibleDir();
-    TEST_REQUIRE(!p.empty());
+    if (p.empty())
+        TEST_UNSUPPORTED();
 #else
     scoped_test_env env;
     const path dir = env.create_dir("dir");
     const path p = env.create_file("dir/file", 42);
     permissions(dir, perms::none);
 #endif
 
     std::error_code ec;

libcxx/test/support/filesystem_test_helper.h

 };
 
 inline fs::path GetWindowsInaccessibleDir() {
   // Only makes sense on windows, but the code can be compiled for
   // any platform.
   const fs::path dir("C:\\System Volume Information");
   std::error_code ec;
   const fs::path root("C:\\");
-  fs::directory_iterator it(root, ec);
-  if (ec)
-    return fs::path();
-  const fs::directory_iterator endIt{};
-  while (it != endIt) {
-    const fs::directory_entry &ent = *it;
-    if (ent == dir) {
-      // Basic sanity checks on the directory_entry
-      if (!ent.exists())
-        return fs::path();
-      if (!ent.is_directory())
-        return fs::path();
-      return ent;
+  for (const auto &ent : fs::directory_iterator(root, ec)) {
+    if (ent != dir)
+      continue;
+    if (!ent.exists() || !ent.is_directory()) {
+      fprintf(stderr, "The expected inaccessible directory \"%s\" was found "
+                      "but doesn't behave as expected, skipping tests "
+                      "regarding it\n", dir.string().c_str());
+    }
+    // Check that it indeed is inaccessible as expected
+    (void)fs::exists(ent, ec);
+    if (!ec) {
+      fprintf(stderr, "The expected inaccessible directory \"%s\" was found "
+                      "but seems to be accessible, skipping tests "
+                      "regarding it\n", dir.string().c_str());
     }
-    ++it;
+    return ent;
   }
+  fprintf(stderr, "No inaccessible directory \"%s\" found, skipping tests "
+                  "regarding it\n", dir.string().c_str());
   return fs::path();
 }
 
 #endif /* FILESYSTEM_TEST_HELPER_HPP */