-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Improve <filesystem> test coverage
#5749
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tem>`. Standard P0218R1_filesystem includes test_filesystem_support.hpp, and we don't want `<experimental/filesystem>` there. get_test_directory_subname() is the important thing to centralize. Explicitly calling fs::temp_directory_path() makes what we're doing clearer.
This test intended to cover the Experimental and Standard implementations, but never got around to finishing the latter. In the `_HAS_CXX17` region, we called get_test_directory() to get a Standard std::filesystem::path, but then everything else called fs::meow() (i.e. std::experimental::filesystem::meow()) and had to use .native() to adapt the Standard path to the Experimental functions. At one point, these were commented TODO, but it looks like I purged those comments during the cleanup for GitHub, without realizing that the test was unfinished. As Standard filesystem has long been completed, we can fix this test. Now the Experimental and Standard regions properly mirror each other. This also noticed a missing assertion in the Experimental part, that the final fs::remove_all() should succeed. Finally, to defend against further confusion, restrict the Experimental namespace alias to the scope where it's needed.
This test was including both Standard and Experimental filesystem, but exercising only Experimental. Gather `<filesystem>` and the commented-out `<any>`, `<optional>`, `<variant>` into a `_HAS_CXX17` block, to avoid emitting "this does nothing" messages in C++14 mode. Include `<type_traits>` because I'll need `is_same_v`. Rename the existing coverage to experimental_filesystem_test_impl and experimental_filesystem_test. Within `_HAS_CXX17`, add standard_filesystem_test_impl and standard_filesystem_test. Differences: * `using namespace experimental::filesystem;` => `using namespace filesystem;` * Guard the `default_locale` constructions with `is_same_v<CharType, char>` due to N5014 [fs.path.construct]/5: "Mandates: The value type of Source and InputIterator is char." * Now that p3, p4, and p5 are scoped, change following usage to always use p0. We don't need to worry about unused variable warnings (for now). * Drop `p0.concat(*c_str);`. N5014 [fs.path.concat] specifies `concat(const Source& x)` and N5014 [fs.path.req]/2 doesn't permit individual characters to be a Source.
…many_double_dots. This Experimental-only test was defining exec_test_output_path_too_long_behavior() but never calling it. As this involves creating symlinks, it couldn't run for PR/CI checks anyways. We have some Standard test coverage for symlinks that warns (without failing) when symlinks can't be created, making it possible for manual runs to laboriously exercise that code. That isn't worth the effort for the Experimental implementation, which we aren't messing with anymore. This function was the only user of MAX_PATH and `<Windows.h>`, which we can now drop from the test too. Also, cleanup an outdated test comment. VSO-158882 "<filesystem>: canonical() should handle symlinks and other OS behavior" was fixed by MSVC-PR-107664 on 2018-02-23, making the Standard implementation call GetFinalPathNameByHandleW. But this is a test for the Experimental implementation, which never calls GetFinalPathNameByHandleW, so the comment should be cauterized.
In C++17 mode, include Standard `<filesystem>`.
Move the existing code into `namespace test_experimental`, including its Experimental namespace alias.
Rephrase `system_clock::now()` to `fs::file_time_type::clock::now()`, which works across Experimental, Standard 17, and Standard 20.
In C++17 mode, introduce `namespace test_standard`, which exercises the Standard `filesystem` namespace.
Note that we need to expect `filesystem::remove_all("DELME_dir", ec) == 4`. It appears that our Standard implementation is correct to return 4, and Experimental incorrectly returned 3.
…rt 1. Guard the inclusion of `<filesystem>` with `_HAS_CXX17`. Introduce `namespace test_experimental` and scope the Experimental namespace alias within it. Extract `Get_child_dir_name()` as it's used by both the Experimental and partial Standard coverage. Remove an unnecessary newline. Change `chrono::system_clock::now()` to `fs::file_time_type::clock::now()`, which works across Experimental, Standard 17, and Standard 20. Adjust the comment to talk about "the file clock". The partial Standard coverage in test_experimental::Test_VSO_171729_disable_recursion_pending_should_not_be_permanent() will be untangled in the next commit.
…rt 2. Add `namespace test_standard`, exercising the Standard namespace. Transfer the Standard `recursive_directory_iterator` case here, removing it from `namespace test_experimental`. (Apparently they differ slightly in semantics.) Update the citations in Test_VSO_153113_copy_examples. Drop the mention of "This tests copy_options::_Unspecified_recursion_prevention_tag" as that doesn't apply to our current Standard implementation.
|
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
MahmoudGSaleh
approved these changes
Oct 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While looking into my long-awaited removal of
<experimental/filesystem>, I noticed that our test coverage for it was tangled up with Standard<filesystem>. We had a mix of tests that were fully dual-mode (testing both Experimental and Standard), partially dual-mode, confused into believing they were dual-mode but not really, and Experimental-only when they could have been dual-mode. We also had some dead test code.This PR improves our Standard
<filesystem>coverage, bringing it up to parity with the Experimental coverage. It doesn't modify the product code.test_filesystem_support.hpp: Avoid dragging in<experimental/filesystem>.P0218R1_filesystemincludestest_filesystem_support.hpp, and we don't want<experimental/filesystem>there.get_test_directory_subname()is the important thing to centralize. Explicitly callingfs::temp_directory_path()makes what we're doing clearer.VSO_0000000_path_stream_parameter, make it dual-mode._HAS_CXX17region, we calledget_test_directory()to get a Standardstd::filesystem::path, but then everything else calledfs::meow()(i.e.std::experimental::filesystem::meow()) and had to use.native()to adapt the Standard path to the Experimental functions. At one point, these were commented TODO, but it looks like I purged those comments during the cleanup for GitHub, without realizing that the test was unfinished.fs::remove_all()should succeed.VSO_0000000_instantiate_iterators_miscto be dual-mode.<filesystem>and the commented-out<any>,<optional>,<variant>into a_HAS_CXX17block, to avoid emitting "this does nothing" messages in C++14 mode.<type_traits>because I'll needis_same_v.experimental_filesystem_test_implandexperimental_filesystem_test._HAS_CXX17, addstandard_filesystem_test_implandstandard_filesystem_test. Differences:using namespace experimental::filesystem;=>using namespace filesystem;default_localeconstructions withis_same_v<CharType, char>due to WG21-N5014 [fs.path.construct]/5: "Mandates: The value type of Source and InputIterator is char."p3,p4, andp5are scoped, change following usage to always usep0. We don't need to worry about unused variable warnings (for now).p0.concat(*c_str);. WG21-N5014 [fs.path.concat] specifiesconcat(const Source& x)and WG21-N5014 [fs.path.req]/2 doesn't permit individual characters to be aSource.VSO_0121275_filesystem_canonical_should_handle_many_double_dots.exec_test_output_path_too_long_behavior()but never calling it. As this involves creating symlinks, it couldn't run for PR/CI checks anyways. We have some Standard test coverage for symlinks that warns (without failing) when symlinks can't be created, making it possible for manual runs to laboriously exercise that code. That isn't worth the effort for the Experimental implementation, which we aren't messing with anymore.MAX_PATHand<Windows.h>, which we can now drop from the test too.<filesystem>:canonical()should handle symlinks and other OS behavior" was fixed by MSVC-PR-107664 on 2018-02-23, making the Standard implementation callGetFinalPathNameByHandleW. But this is a test for the Experimental implementation, which never callsGetFinalPathNameByHandleW, so the comment should be cauterized.Dev11_1180290_filesystem_error_codeto be dual-mode.<filesystem>.namespace test_experimental, including its Experimental namespace alias.system_clock::now()tofs::file_time_type::clock::now(), which works across Experimental, Standard 17, and Standard 20.namespace test_standard, which exercises the Standardfilesystemnamespace.filesystem::remove_all("DELME_dir", ec) == 4. It appears that our Standard implementation is correct to return 4, and Experimental incorrectly returned 3.Dev11_1066931_filesystem_rename_noopto be fully dual-mode, part 1.<filesystem>with_HAS_CXX17.namespace test_experimentaland scope the Experimental namespace alias within it.Get_child_dir_name()as it's used by both the Experimental and partial Standard coverage.chrono::system_clock::now()tofs::file_time_type::clock::now(), which works across Experimental, Standard 17, and Standard 20. Adjust the comment to talk about "the file clock".test_experimental::Test_VSO_171729_disable_recursion_pending_should_not_be_permanent()will be untangled in the next commit.Dev11_1066931_filesystem_rename_noopto be fully dual-mode, part 2.namespace test_standard, exercising the Standard namespace.recursive_directory_iteratorcase here, removing it fromnamespace test_experimental. (Apparently they differ slightly in semantics.)Test_VSO_153113_copy_examples(). Drop the mention of "This testscopy_options::_Unspecified_recursion_prevention_tag" as that doesn't apply to our current Standard implementation.