Improve <filesystem> test coverage by StephanTLavavej · Pull Request #5749 · microsoft/STL (original) (raw)

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>.
  • 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.
• Fix confusion in VSO_0000000_path_stream_parameter, make it dual-mode.
  • 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.
• Fix VSO_0000000_instantiate_iterators_misc to be dual-mode.
  • 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 WG21-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);. WG21-N5014 [fs.path.concat] specifies concat(const Source& x) and WG21-N5014 [fs.path.req]/2 doesn't permit individual characters to be a Source.
• Remove dead code from VSO_0121275_filesystem_canonical_should_handle_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.
  • (This test appears to be highly specific to the Experimental implementation's limitations, so I'm not attempting to make it dual-mode.)
• Extend Dev11_1180290_filesystem_error_code to be dual-mode.
  • 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.
• Extend Dev11_1066931_filesystem_rename_noop to be fully dual-mode, part 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.
• Extend Dev11_1066931_filesystem_rename_noop to be fully dual-mode, part 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.

…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 ": 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.