[libcxx-commits] [PATCH] D98960: [libcxx] [test] Allow C:\System Volume Information to be missing

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

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



================
Comment at: libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.exists/exists.pass.cpp:82
+    if (p.empty())
+        TEST_UNSUPPORTED();
 #else
----------------
mstorsjo wrote:
> Quuxplusone wrote:
> > 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?
> 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.
> 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?


================
Comment at: libcxx/test/support/filesystem_test_helper.h:713
   return fs::path();
 }
 
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98960/new/

https://reviews.llvm.org/D98960