From e46610f9f849a724e01b39b09b755da98a077e54 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 9 May 2024 14:36:56 -0700 Subject: [PATCH 01/14] Centralize the definition of temp_file_name(). In GH 3337, I pushed a commit to copy tests/tr1/include/temp_file_name.h to tests/std/include/temp_file_name.hpp, because tests/std can't use tests/tr1/include. What I forgot is that tests/tr1 *can* use tests/std/include, in both the GitHub and MSVC-internal test harnesses. Let's deduplicate these files now. Include `` with angle brackets because it's outside the current directory (this is our usual convention, with only a few exceptions that I'll clean up later). --- tests/tr1/include/temp_file_name.h | 21 --------------------- tests/tr1/tests/cstdio/test.cpp | 3 ++- tests/tr1/tests/cwchar1/test.cpp | 3 ++- tests/tr1/tests/fstream1/test.cpp | 3 ++- tests/tr1/tests/fstream2/test.cpp | 3 ++- 5 files changed, 8 insertions(+), 25 deletions(-) delete mode 100644 tests/tr1/include/temp_file_name.h diff --git a/tests/tr1/include/temp_file_name.h b/tests/tr1/include/temp_file_name.h deleted file mode 100644 index 79991f56f20..00000000000 --- a/tests/tr1/include/temp_file_name.h +++ /dev/null @@ -1,21 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception - -#pragma once - -#include -#include - -[[nodiscard]] inline std::string temp_file_name() { - std::string ret{"temp_file_"}; - std::uniform_int_distribution dist{0, 15}; - std::random_device rd; - - for (int i = 0; i < 64; ++i) { // 64 hexits = 256 bits of entropy - ret.push_back("0123456789ABCDEF"[dist(rd)]); - } - - ret += ".tmp"; - - return ret; -} diff --git a/tests/tr1/tests/cstdio/test.cpp b/tests/tr1/tests/cstdio/test.cpp index e0bacf6c45a..e59d5a783a7 100644 --- a/tests/tr1/tests/cstdio/test.cpp +++ b/tests/tr1/tests/cstdio/test.cpp @@ -5,13 +5,14 @@ #define TEST_NAMEX "" #include "tdefs.h" -#include "temp_file_name.h" #include #include #include #include #include +#include + #undef clearerr // tested in stdio2.c #undef feof #undef ferror diff --git a/tests/tr1/tests/cwchar1/test.cpp b/tests/tr1/tests/cwchar1/test.cpp index d773b7b00a8..b4449cd1e2a 100644 --- a/tests/tr1/tests/cwchar1/test.cpp +++ b/tests/tr1/tests/cwchar1/test.cpp @@ -5,13 +5,14 @@ #define TEST_NAMEX ", part 1" #include "tdefs.h" -#include "temp_file_name.h" #include #include #include #include #include +#include + #pragma warning(disable : 4793) // function compiled as native diff --git a/tests/tr1/tests/fstream1/test.cpp b/tests/tr1/tests/fstream1/test.cpp index af558e99fd6..680bd7e8645 100644 --- a/tests/tr1/tests/fstream1/test.cpp +++ b/tests/tr1/tests/fstream1/test.cpp @@ -5,11 +5,12 @@ #define TEST_NAME ", part 1" #include "tdefs.h" -#include "temp_file_name.h" #include #include #include +#include + void test_main() { // test basic workings of char fstream definitions STD string tn_str = temp_file_name(); const char* tn = tn_str.c_str(); diff --git a/tests/tr1/tests/fstream2/test.cpp b/tests/tr1/tests/fstream2/test.cpp index b0401459493..42efa9c12a9 100644 --- a/tests/tr1/tests/fstream2/test.cpp +++ b/tests/tr1/tests/fstream2/test.cpp @@ -5,11 +5,12 @@ #define TEST_NAME ", part 2" #include "tdefs.h" -#include "temp_file_name.h" #include #include #include +#include + void test_main() { // test basic workings of wide fstream definitions const auto temp_name = temp_file_name(); const char* tn = temp_name.c_str(); From 143366a85e0335d4c08ff7f0b40fb154aa9d4515 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 9 May 2024 14:30:22 -0700 Subject: [PATCH 02/14] Improve temp_file_name(): "msvc_stl_" prefix, 128 bits. The ".tmp" extension is already clear, so let's change the prefix to make the source of these files obvious. In GH 2210, I chose 256 bits of entropy without careful consideration, and it was way too much. 128 bits is plenty; see Wikipedia's birthday problem article for a probability table. --- tests/std/include/temp_file_name.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/std/include/temp_file_name.hpp b/tests/std/include/temp_file_name.hpp index 79991f56f20..45e3e171ebb 100644 --- a/tests/std/include/temp_file_name.hpp +++ b/tests/std/include/temp_file_name.hpp @@ -7,11 +7,11 @@ #include [[nodiscard]] inline std::string temp_file_name() { - std::string ret{"temp_file_"}; + std::string ret{"msvc_stl_"}; std::uniform_int_distribution 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)]); } From ac441a207261d43a1d24aac9b6b7cc3ab78e1d32 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 9 May 2024 14:58:26 -0700 Subject: [PATCH 03/14] Also use 32 hexits / 128 bits in get_test_directory_subname(). --- tests/std/include/test_filesystem_support.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/std/include/test_filesystem_support.hpp b/tests/std/include/test_filesystem_support.hpp index 3aa59865650..b2f8c94fbc7 100644 --- a/tests/std/include/test_filesystem_support.hpp +++ b/tests/std/include/test_filesystem_support.hpp @@ -19,7 +19,7 @@ inline std::string get_test_directory_subname(const char* const testName, const uniform_int_distribution<> dist(0, 15); string subName(testName, testNameLength); 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; } From 4f1ef1b9322b5ac65eb547ae0face0f6b4878689 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 9 May 2024 15:10:05 -0700 Subject: [PATCH 04/14] Rename part 1: `get_test_directory` => `get_experimental_test_directory` --- tests/std/include/test_filesystem_support.hpp | 2 +- tests/std/tests/Dev11_1066931_filesystem_rename_noop/test.cpp | 2 +- tests/std/tests/VSO_0000000_path_stream_parameter/test.cpp | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/std/include/test_filesystem_support.hpp b/tests/std/include/test_filesystem_support.hpp index b2f8c94fbc7..2324a3d1517 100644 --- a/tests/std/include/test_filesystem_support.hpp +++ b/tests/std/include/test_filesystem_support.hpp @@ -23,7 +23,7 @@ inline std::string get_test_directory_subname(const char* const testName, const return subName; } -inline std::experimental::filesystem::path get_test_directory(const char* const 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, strlen(testName)); } diff --git a/tests/std/tests/Dev11_1066931_filesystem_rename_noop/test.cpp b/tests/std/tests/Dev11_1066931_filesystem_rename_noop/test.cpp index 27300609c41..dd727839dbe 100644 --- a/tests/std/tests/Dev11_1066931_filesystem_rename_noop/test.cpp +++ b/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); diff --git a/tests/std/tests/VSO_0000000_path_stream_parameter/test.cpp b/tests/std/tests/VSO_0000000_path_stream_parameter/test.cpp index 53b9f6576f4..3f69fa5c90c 100644 --- a/tests/std/tests/VSO_0000000_path_stream_parameter/test.cpp +++ b/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); From 9afb8c1a4aea64cf1ed8851935aae683dab5d54f Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 9 May 2024 15:11:32 -0700 Subject: [PATCH 05/14] Rename part 2: `get_new_test_directory` => `get_test_directory` --- tests/std/include/test_filesystem_support.hpp | 2 +- tests/std/tests/P0218R1_filesystem/test.cpp | 2 +- tests/std/tests/VSO_0000000_path_stream_parameter/test.cpp | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/std/include/test_filesystem_support.hpp b/tests/std/include/test_filesystem_support.hpp index 2324a3d1517..a3d093df739 100644 --- a/tests/std/include/test_filesystem_support.hpp +++ b/tests/std/include/test_filesystem_support.hpp @@ -31,7 +31,7 @@ inline std::experimental::filesystem::path get_experimental_test_directory(const #if _HAS_CXX17 #include -inline std::filesystem::path get_new_test_directory(std::string_view testName) { +inline std::filesystem::path get_test_directory(std::string_view testName) { return std::filesystem::temp_directory_path() / get_test_directory_subname(testName.data(), testName.size()); } #endif // _HAS_CXX17 diff --git a/tests/std/tests/P0218R1_filesystem/test.cpp b/tests/std/tests/P0218R1_filesystem/test.cpp index 305990b95a4..d3a614f9b7a 100644 --- a/tests/std/tests/P0218R1_filesystem/test.cpp +++ b/tests/std/tests/P0218R1_filesystem/test.cpp @@ -56,7 +56,7 @@ template struct test_temp_directory { error_code ec; 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)) { remove_all(directoryPath, ec); if (ec) { wcerr << L"Warning, couldn't clean up " << directoryPath << L" before test.\n"; diff --git a/tests/std/tests/VSO_0000000_path_stream_parameter/test.cpp b/tests/std/tests/VSO_0000000_path_stream_parameter/test.cpp index 3f69fa5c90c..41910ec0ccf 100644 --- a/tests/std/tests/VSO_0000000_path_stream_parameter/test.cpp +++ b/tests/std/tests/VSO_0000000_path_stream_parameter/test.cpp @@ -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); From d59c6cafc3c7a7c39f7e3d27f7c1d9976daaeb61 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 9 May 2024 15:16:39 -0700 Subject: [PATCH 06/14] Simplify get_test_directory_subname() by templating it. We no longer need `` for strlen(). --- tests/std/include/test_filesystem_support.hpp | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/tests/std/include/test_filesystem_support.hpp b/tests/std/include/test_filesystem_support.hpp index a3d093df739..34c338b960c 100644 --- a/tests/std/include/test_filesystem_support.hpp +++ b/tests/std/include/test_filesystem_support.hpp @@ -6,32 +6,31 @@ #define _SILENCE_EXPERIMENTAL_FILESYSTEM_DEPRECATION_WARNING #include -#include #include #include #include #include #include -inline std::string get_test_directory_subname(const char* const testName, const size_t testNameLength) { +template +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), 32, [&] { return "0123456789ABCDEF"[dist(rd)]; }); return subName; } 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, strlen(testName)); + return std::experimental::filesystem::temp_directory_path() / get_test_directory_subname(testName); } #if _HAS_CXX17 #include inline std::filesystem::path get_test_directory(std::string_view testName) { - return std::filesystem::temp_directory_path() / get_test_directory_subname(testName.data(), testName.size()); + return std::filesystem::temp_directory_path() / get_test_directory_subname(testName); } #endif // _HAS_CXX17 From 86b6a06e1404a09e1193fa2b01bfa7b6e646c59a Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 9 May 2024 15:30:14 -0700 Subject: [PATCH 07/14] Make test_temp_directory resistant to misuse. Mark it as a `[[nodiscard]]` guard type, and make it noncopyable. --- tests/std/tests/P0218R1_filesystem/test.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/std/tests/P0218R1_filesystem/test.cpp b/tests/std/tests/P0218R1_filesystem/test.cpp index d3a614f9b7a..65ed2485161 100644 --- a/tests/std/tests/P0218R1_filesystem/test.cpp +++ b/tests/std/tests/P0218R1_filesystem/test.cpp @@ -53,7 +53,7 @@ template return str.size() >= prefix.size() && Traits::compare(str.data(), prefix.data(), prefix.size()) == 0; } -struct test_temp_directory { +struct [[nodiscard]] test_temp_directory { error_code ec; path directoryPath; explicit test_temp_directory(const string_view testName) : directoryPath(get_test_directory(testName)) { @@ -68,6 +68,9 @@ 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 { remove_all(directoryPath, ec); if (ec) { From 8d33229e7ff0a7503b84cebcb0fbf525a4b8af98 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 9 May 2024 15:35:42 -0700 Subject: [PATCH 08/14] test_temp_directory's ctor/dtor can use local `error_code ec;`. Nobody needed to access this as a public data member after construction. --- tests/std/tests/P0218R1_filesystem/test.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/std/tests/P0218R1_filesystem/test.cpp b/tests/std/tests/P0218R1_filesystem/test.cpp index 65ed2485161..0a6c58bc3a0 100644 --- a/tests/std/tests/P0218R1_filesystem/test.cpp +++ b/tests/std/tests/P0218R1_filesystem/test.cpp @@ -54,9 +54,9 @@ template } struct [[nodiscard]] test_temp_directory { - error_code ec; path directoryPath; 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"; @@ -72,6 +72,7 @@ struct [[nodiscard]] test_temp_directory { 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"; From 5fc79e8fe25d6f9fc353a813ee3be39a87c72a31 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 9 May 2024 15:43:08 -0700 Subject: [PATCH 09/14] Consistently name test_temp_directory, part 1: `recursiveTests` => `tempDir` --- tests/std/tests/P0218R1_filesystem/test.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/std/tests/P0218R1_filesystem/test.cpp b/tests/std/tests/P0218R1_filesystem/test.cpp index 0a6c58bc3a0..20b97d066b6 100644 --- a/tests/std/tests/P0218R1_filesystem/test.cpp +++ b/tests/std/tests/P0218R1_filesystem/test.cpp @@ -1424,8 +1424,8 @@ void test_recursive_directory_iterator() { test_directory_iterator_common_parts("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; @@ -1434,7 +1434,7 @@ 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; } @@ -1442,11 +1442,11 @@ void test_recursive_directory_iterator() { 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); @@ -1465,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{}); } From 835645ab670286c41367ea5c24b2aad47f2e1f6c Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 9 May 2024 15:44:50 -0700 Subject: [PATCH 10/14] Consistently name test_temp_directory, part 2: `followSymlinkTests` => `tempDir` --- tests/std/tests/P0218R1_filesystem/test.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/std/tests/P0218R1_filesystem/test.cpp b/tests/std/tests/P0218R1_filesystem/test.cpp index 20b97d066b6..d7221a1ff0d 100644 --- a/tests/std/tests/P0218R1_filesystem/test.cpp +++ b/tests/std/tests/P0218R1_filesystem/test.cpp @@ -1485,10 +1485,10 @@ void test_recursive_directory_iterator() { // Also test VSO-649431 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) { @@ -1500,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()); From 2a058fef0d2e2957e3d9305f5b18c8a410417cf2 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 9 May 2024 15:54:48 -0700 Subject: [PATCH 11/14] Use consistent names for test directory bases. "recursive_directory_iterator-specific": Separate with a dash, instead of an accursed space. "recursive_directory_iterator-VSO-649431": Separate with a dash, instead of an underscore. "status": We use the name of the filesystem function we're testing, not our outer test function. "create_directories-and-remove_all": Separate with dashes, don't abbreviate. --- tests/std/tests/P0218R1_filesystem/test.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/std/tests/P0218R1_filesystem/test.cpp b/tests/std/tests/P0218R1_filesystem/test.cpp index d7221a1ff0d..9df6155eef8 100644 --- a/tests/std/tests/P0218R1_filesystem/test.cpp +++ b/tests/std/tests/P0218R1_filesystem/test.cpp @@ -1424,7 +1424,7 @@ void test_recursive_directory_iterator() { test_directory_iterator_common_parts("recursive_directory_iterator"sv); { - const test_temp_directory tempDir("recursive_directory_iterator specific"sv); + 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; @@ -1485,7 +1485,7 @@ void test_recursive_directory_iterator() { // Also test VSO-649431 follow_directory_symlinks with a broken symlink causes iteration to break { - const test_temp_directory tempDir("recursive_directory_iterator_VSO-649431"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; @@ -2887,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); @@ -3647,7 +3647,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 From 00b66244ac5b076757fc24fad1dbc44f49afc53e Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 9 May 2024 16:51:33 -0700 Subject: [PATCH 12/14] Directly pass `path`s instead of `.native()`. These calls to create_directories(), create_directory(), and exists() were totally inconsistent for no reason. We don't need to construct temporary identical paths. --- tests/std/tests/P0218R1_filesystem/test.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/std/tests/P0218R1_filesystem/test.cpp b/tests/std/tests/P0218R1_filesystem/test.cpp index 9df6155eef8..61907288f16 100644 --- a/tests/std/tests/P0218R1_filesystem/test.cpp +++ b/tests/std/tests/P0218R1_filesystem/test.cpp @@ -3332,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"); @@ -3349,7 +3349,7 @@ void test_rename() { // 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 @@ -3368,7 +3368,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"); From 0250412b085dbf7fe3c8f29eaaec890915d2fb6d Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 9 May 2024 16:54:48 -0700 Subject: [PATCH 13/14] Improve testing of "Standard rename where target doesn't exist". We should call the ec form (since we're not catching exceptions), verify that ec is good, and verify that the source file no longer exists. --- tests/std/tests/P0218R1_filesystem/test.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/std/tests/P0218R1_filesystem/test.cpp b/tests/std/tests/P0218R1_filesystem/test.cpp index 61907288f16..db88488f77c 100644 --- a/tests/std/tests/P0218R1_filesystem/test.cpp +++ b/tests/std/tests/P0218R1_filesystem/test.cpp @@ -3353,7 +3353,9 @@ void test_rename() { 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"); // Bad cases From b03f29e5c6dfa5ed84a54562eabd21d4bc609711 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 9 May 2024 19:16:37 -0700 Subject: [PATCH 14/14] Skip affected rename() tests for `_MSVC_INTERNAL_TESTING`. --- tests/std/tests/P0218R1_filesystem/test.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/std/tests/P0218R1_filesystem/test.cpp b/tests/std/tests/P0218R1_filesystem/test.cpp index db88488f77c..c7907450e42 100644 --- a/tests/std/tests/P0218R1_filesystem/test.cpp +++ b/tests/std/tests/P0218R1_filesystem/test.cpp @@ -3346,6 +3346,14 @@ 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)); @@ -3357,6 +3365,7 @@ void test_rename() { 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));