Improve P0218R1_filesystem test reliability and temp filename generation (#4665)

Author: StephanTLavavej

tests/std/include/temp_file_name.hpp

@@ -7,11 +7,11 @@
 #include <string>
 
 [[nodiscard]] inline std::string temp_file_name() {
-    std::string ret{"temp_file_"};
+    std::string ret{"msvc_stl_"};
     std::uniform_int_distribution<int> dist{0, 15};
     std::random_device rd;
 
-    for (int i = 0; i < 64; ++i) { // 64 hexits = 256 bits of entropy
+    for (int i = 0; i < 32; ++i) { // 32 hexits = 128 bits of entropy
         ret.push_back("0123456789ABCDEF"[dist(rd)]);
     }
 

tests/std/include/test_filesystem_support.hpp

@@ -6,32 +6,31 @@
 #define _SILENCE_EXPERIMENTAL_FILESYSTEM_DEPRECATION_WARNING
 
 #include <algorithm>
-#include <cstring>
 #include <experimental/filesystem>
 #include <filesystem>
 #include <iterator>
 #include <random>
 #include <string>
 
-inline std::string get_test_directory_subname(const char* const testName, const size_t testNameLength) {
+template <class T>
+std::string get_test_directory_subname(const T& testName) {
     using namespace std;
     random_device rd;
     uniform_int_distribution<> dist(0, 15);
-    string subName(testName, testNameLength);
+    string subName(testName);
     subName.push_back('_');
-    generate_n(back_inserter(subName), 16, [&] { return "0123456789ABCDEF"[dist(rd)]; });
+    generate_n(back_inserter(subName), 32, [&] { return "0123456789ABCDEF"[dist(rd)]; });
     return subName;
 }
 
-inline std::experimental::filesystem::path get_test_directory(const char* const testName) {
-    return std::experimental::filesystem::temp_directory_path()
-         / get_test_directory_subname(testName, strlen(testName));
+inline std::experimental::filesystem::path get_experimental_test_directory(const char* const testName) {
+    return std::experimental::filesystem::temp_directory_path() / get_test_directory_subname(testName);
 }
 
 #if _HAS_CXX17
 #include <string_view>
 
-inline std::filesystem::path get_new_test_directory(std::string_view testName) {
-    return std::filesystem::temp_directory_path() / get_test_directory_subname(testName.data(), testName.size());
+inline std::filesystem::path get_test_directory(std::string_view testName) {
+    return std::filesystem::temp_directory_path() / get_test_directory_subname(testName);
 }
 #endif // _HAS_CXX17

tests/std/tests/Dev11_1066931_filesystem_rename_noop/test.cpp

@@ -345,7 +345,7 @@ int main() {
     error_code ec;
     const auto previousCd = fs::current_path(ec);
     assert_success(ec);
-    const auto testDir = get_test_directory("filesystem_rename_noop");
+    const auto testDir = get_experimental_test_directory("filesystem_rename_noop");
     printf("changing directory to \"%ls\"\n", testDir.native().c_str());
     fs::create_directory(testDir, ec);
     assert_success(ec);

tests/std/tests/P0218R1_filesystem/test.cpp

@@ -53,10 +53,10 @@ template <typename Elem, typename Traits>
     return str.size() >= prefix.size() && Traits::compare(str.data(), prefix.data(), prefix.size()) == 0;
 }
 
-struct test_temp_directory {
-    error_code ec;
+struct [[nodiscard]] test_temp_directory {
     path directoryPath;
-    explicit test_temp_directory(const string_view testName) : directoryPath(get_new_test_directory(testName)) {
+    explicit test_temp_directory(const string_view testName) : directoryPath(get_test_directory(testName)) {
+        error_code ec;
         remove_all(directoryPath, ec);
         if (ec) {
             wcerr << L"Warning, couldn't clean up " << directoryPath << L" before test.\n";
@@ -68,7 +68,11 @@ struct test_temp_directory {
         }
     }
 
+    test_temp_directory(const test_temp_directory&)            = delete;
+    test_temp_directory& operator=(const test_temp_directory&) = delete;
+
     ~test_temp_directory() noexcept {
+        error_code ec;
         remove_all(directoryPath, ec);
         if (ec) {
             wcerr << L"Warning, couldn't clean up " << directoryPath << L" after test.\n";
@@ -1420,8 +1424,8 @@ void test_recursive_directory_iterator() {
     test_directory_iterator_common_parts<recursive_directory_iterator>("recursive_directory_iterator"sv);
 
     {
-        const test_temp_directory recursiveTests("recursive_directory_iterator specific"sv);
-        create_file_containing(recursiveTests.directoryPath / L"a.txt"sv, L"hello");
+        const test_temp_directory tempDir("recursive_directory_iterator-specific"sv);
+        create_file_containing(tempDir.directoryPath / L"a.txt"sv, L"hello");
 
         // _NODISCARD directory_options  options() const;
         // _NODISCARD int                depth() const;
@@ -1430,19 +1434,19 @@ void test_recursive_directory_iterator() {
         // void disable_recursion_pending();
         {
             error_code ec;
-            recursive_directory_iterator good_dir(recursiveTests.directoryPath, directory_options::none, ec);
+            recursive_directory_iterator good_dir(tempDir.directoryPath, directory_options::none, ec);
             if (!EXPECT(good(ec))) {
                 return;
             }
 
             EXPECT(good_dir.options() == directory_options::none);
 
             recursive_directory_iterator good_dir2(
-                recursiveTests.directoryPath, directory_options::skip_permission_denied, ec);
+                tempDir.directoryPath, directory_options::skip_permission_denied, ec);
             EXPECT(good_dir2.options() == directory_options::skip_permission_denied);
 
             recursive_directory_iterator good_dir3(
-                recursiveTests.directoryPath, directory_options::follow_directory_symlink, ec);
+                tempDir.directoryPath, directory_options::follow_directory_symlink, ec);
             EXPECT(good_dir3.options() == directory_options::follow_directory_symlink);
 
             EXPECT(good_dir.depth() == 0);
@@ -1461,7 +1465,7 @@ void test_recursive_directory_iterator() {
 
         // void pop();
         {
-            recursive_directory_iterator good_dir(recursiveTests.directoryPath, directory_options::none);
+            recursive_directory_iterator good_dir(tempDir.directoryPath, directory_options::none);
             good_dir.pop();
             EXPECT(good_dir == recursive_directory_iterator{});
         }
@@ -1481,10 +1485,10 @@ void test_recursive_directory_iterator() {
 
     // Also test VSO-649431 <filesystem> follow_directory_symlinks with a broken symlink causes iteration to break
     {
-        const test_temp_directory followSymlinkTests("recursive_directory_iterator_VSO-649431"sv);
-        const path aaa = followSymlinkTests.directoryPath / L"aaa"sv;
-        const path bbb = followSymlinkTests.directoryPath / L"bbb"sv;
-        const path ccc = followSymlinkTests.directoryPath / L"ccc"sv;
+        const test_temp_directory tempDir("recursive_directory_iterator-VSO-649431"sv);
+        const path aaa = tempDir.directoryPath / L"aaa"sv;
+        const path bbb = tempDir.directoryPath / L"bbb"sv;
+        const path ccc = tempDir.directoryPath / L"ccc"sv;
         error_code ec;
         create_directory_symlink(nonexistentPaths[0], bbb, ec);
         if (ec) {
@@ -1496,7 +1500,7 @@ void test_recursive_directory_iterator() {
                 directory_options::follow_directory_symlink, directory_options::skip_permission_denied,
                 directory_options::follow_directory_symlink | directory_options::skip_permission_denied};
             for (const auto& option : options) {
-                recursive_directory_iterator first(followSymlinkTests.directoryPath, option);
+                recursive_directory_iterator first(tempDir.directoryPath, option);
                 assert(first != recursive_directory_iterator{});
                 EXPECT(first->is_directory());
                 EXPECT(!first->is_symlink());
@@ -2883,7 +2887,7 @@ void test_invalid_conversions() {
 }
 
 void test_status() {
-    const test_temp_directory tempDir("test_status"sv);
+    const test_temp_directory tempDir("status"sv);
     const path& testDir = tempDir.directoryPath;
     const path testFile(testDir / L"test_file"sv);
     const path testLink(testDir / L"test_link"sv);
@@ -3328,9 +3332,9 @@ void test_rename() {
     const path fileA(tempDir.directoryPath / L"filea.txt"sv);
     const path fileB(tempDir.directoryPath / L"fileb.txt"sv);
 
-    create_directories(dir.native(), ec);
+    create_directories(dir, ec);
     EXPECT(good(ec));
-    create_directory(otherDir.native(), ec);
+    create_directory(otherDir, ec);
     EXPECT(good(ec));
     create_file_containing(fileA, L"hello");
     create_file_containing(fileB, L"world");
@@ -3342,15 +3346,26 @@ void test_rename() {
     EXPECT(good(ec));
     EXPECT(read_file_contents(fileA) == L"hello");
 
+#ifndef _MSVC_INTERNAL_TESTING // TRANSITION, skip this for all MSVC-internal test runs.
+    // As of 2024-05-09, these rename() tests sporadically fail in MSVC-internal private test runs with
+    // "Access is denied" error codes. We've never observed such failures in MSVC-internal PR/CI checks,
+    // MSVC-internal local test runs, GitHub PR/CI checks, or GitHub local test runs. There's no significant
+    // compiler interaction here, so we can live with GitHub-only test coverage. Although we don't know the
+    // root cause, we suspect that this is related to the physical machines that are used for MSVC-internal
+    // private test runs, so we should check whether they've been replaced in a year or two.
+
     // If new_p resolves to an existing non-directory file, new_p is removed
     rename(fileA, fileB, ec);
     EXPECT(good(ec));
-    EXPECT(!exists(fileA.native()));
+    EXPECT(!exists(fileA));
     EXPECT(read_file_contents(fileB) == L"hello");
 
     // Standard rename where target doesn't exist
-    rename(fileB, fileA);
+    rename(fileB, fileA, ec);
+    EXPECT(good(ec));
+    EXPECT(!exists(fileB));
     EXPECT(read_file_contents(fileA) == L"hello");
+#endif // ^^^ no workaround ^^^
 
     // Bad cases
     EXPECT(throws_filesystem_error([&] { rename(dir, otherDir); }, "rename", dir, otherDir));
@@ -3364,7 +3379,7 @@ void test_space() {
     const path file(dir / L"test_space_file.txt"sv);
 
     error_code ec;
-    create_directory(dir.native(), ec);
+    create_directory(dir, ec);
     EXPECT(good(ec));
     create_file_containing(file, L"hello");
 
@@ -3643,7 +3658,7 @@ void test_create_directory() {
 }
 
 void test_create_dirs_and_remove_all() {
-    const test_temp_directory tempDir("create_dirs_and_remove_all"sv);
+    const test_temp_directory tempDir("create_directories-and-remove_all"sv);
     const path& r = tempDir.directoryPath;
 
     // test long path support

tests/std/tests/VSO_0000000_path_stream_parameter/test.cpp

@@ -18,7 +18,7 @@ int main() {
     error_code ec;
 
     {
-        const auto testDir = get_test_directory("path_stream_parameter");
+        const auto testDir = get_experimental_test_directory("path_stream_parameter");
         fs::create_directories(testDir, ec);
         assert(!ec);
 
@@ -81,7 +81,7 @@ int main() {
 
 #if _HAS_CXX17
     {
-        const auto testDir = get_new_test_directory("path_stream_parameter");
+        const auto testDir = get_test_directory("path_stream_parameter");
         fs::create_directories(testDir.native(), ec);
         assert(!ec);
 

tests/tr1/include/temp_file_name.h

@@ -5,13 +5,14 @@
 #define TEST_NAMEX "<cstdio>"
 
 #include "tdefs.h"
-#include "temp_file_name.h"
 #include <assert.h>
 #include <cstdio>
 #include <errno.h>
 #include <stdarg.h>
 #include <string.h>
 
+#include <temp_file_name.hpp>
+
 #undef clearerr // tested in stdio2.c
 #undef feof
 #undef ferror

tests/tr1/tests/cstdio/test.cpp

@@ -5,13 +5,14 @@
 #define TEST_NAMEX "<cwchar>, part 1"
 
 #include "tdefs.h"
-#include "temp_file_name.h"
 #include <assert.h>
 #include <cwchar>
 #include <stdarg.h>
 #include <stdlib.h>
 #include <string.h>
 
+#include <temp_file_name.hpp>
+
 #pragma warning(disable : 4793) // function compiled as native
 
 

tests/tr1/tests/cwchar1/test.cpp

@@ -5,11 +5,12 @@
 #define TEST_NAME "<fstream>, part 1"
 
 #include "tdefs.h"
-#include "temp_file_name.h"
 #include <assert.h>
 #include <fstream>
 #include <string>
 
+#include <temp_file_name.hpp>
+
 void test_main() { // test basic workings of char fstream definitions
     STD string tn_str = temp_file_name();
     const char* tn    = tn_str.c_str();

tests/tr1/tests/fstream1/test.cpp

@@ -5,11 +5,12 @@
 #define TEST_NAME "<fstream>, part 2"
 
 #include "tdefs.h"
-#include "temp_file_name.h"
 #include <assert.h>
 #include <fstream>
 #include <wchar.h>
 
+#include <temp_file_name.hpp>
+
 void test_main() { // test basic workings of wide fstream definitions
     const auto temp_name = temp_file_name();
     const char* tn       = temp_name.c_str();