approvals / ApprovalTests.cpp

Native ApprovalTests for C++ on Linux, Mac and Windows
https://approvaltestscpp.readthedocs.io/en/latest/
Apache License 2.0
310 stars 50 forks source link

Impossible to use UNC filename in TemplatedCustomNamer [`removeRedundantDirectorySeparators()` breaks use of UNC paths, which contain `\\`] #225

Open TeamPlatform1 opened 4 months ago

TeamPlatform1 commented 4 months ago

If the TemplatesCustomNamer is instructed to create a filename starting with "//SomeHost/SomeDir..", the double (back)slash is removed in "removeRedundantDirectorySeparators", which makes it impossible to use UNC paths.

claremacrae commented 4 months ago

Hi,

If the TemplatesCustomNamer is instructed to create a filename starting with "//SomeHost/SomeDir..", the double (back)slash is removed in "removeRedundantDirectorySeparators", which makes it impossible to use UNC paths.

Please give a source code sample that we can use to test the problem.

And what backslash are you referring to? I only see forward slashes.

TeamPlatform1 commented 4 months ago

A snippet from our main.cpp:

::ApprovalTests::initializeApprovalTestsForGoogleTests();
APPROVAL_TESTS_REGISTER_MAIN_DIRECTORY
auto defaultNamer = ::ApprovalTests::Approvals::useAsDefaultNamer([]()
  {
    auto approvalFilesBaseDir = kernel::utilities::Environment::GetEnvironmentVariable(L"KERNEL_TEST_APPROVAL_FILES_BASE_DIR");
    return std::make_shared<ApprovalTests::TemplatedCustomNamer>(
      kernel::utilities::Convert::WStringToUTF8String(approvalFilesBaseDir) + 
      "/{RelativeTestSourceDirectory}/{TestFileName}.{TestCaseName}.{ApprovedOrReceived}.{FileExtension}"
    );
  });

In the environment variable I tried using something like "//server/base/path" and "\\server\base\path" (we're on windows).

In both cases the doubled (back)slash is eliminated in "removeRedundantDirectorySeparators". Looks like using a UNC path is currently impossible.

claremacrae commented 4 months ago

Thanks.

In case it helps in future, this is how to format code so it's readable here:

```cpp
<your code...>
```

I've made that edit in your post above.

claremacrae commented 4 months ago

Thank you for the code.

In both cases the doubled (back)slash is eliminated in removeRedundantDirectorySeparators(). Looks like using a UNC path is currently impossible.

Yes, I'm sure that's the problem.

I see that we have no explicit tests for that function.

The fix would be be to:

  1. add tests for the existing behaviour of removeRedundantDirectorySeparators(),
  2. and then teach it to not treat double-slashes at the start of absolute paths as redundant...
claremacrae commented 4 months ago
  1. add tests for the existing behaviour of removeRedundantDirectorySeparators(),

Likely something like this:

TEST_CASE("Path retains initial slashes in UNC paths")
{
    const std::string start = R"(\\a\b\c.txt)";
    Path path = Path(start);
    CHECK("//a/b/c.txt" == path.toString("/"));
    CHECK("\\\\a\\b\\c.txt" == path.toString("\\"));
}