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

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


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


================
Comment at: libcxx/test/support/filesystem_test_helper.h:713
   return fs::path();
 }
 
----------------
Quuxplusone wrote:
> 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.
Oh, thanks, I'll have a go at such a rewrite.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98960