STL icon indicating copy to clipboard operation
STL copied to clipboard

Improve `P0218R1_filesystem` test reliability and temp filename generation

Open StephanTLavavej opened this issue 1 year ago • 1 comments

Making an exception to our usual rule of "we don't spend a lot of effort on cleaning up tests that are well-behaved, because it's low reward and risks disrupting what they were trying to test". I had to investigate why P0218R1_filesystem was behaving slightly flaky, and I ended up making a bunch of targeted improvements.

  • Centralize the definition of temp_file_name().
    • In #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 <temp_file_name.hpp> 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).
  • 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 #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.
  • Conversely, increase get_test_directory_subname() from 16 hexits / 64 bits to 32 hexits / 128 bits.
  • Rename part 1: get_test_directory => get_experimental_test_directory
  • Rename part 2: get_new_test_directory => get_test_directory
    • Standard <filesystem> should be the default assumption.
  • Simplify get_test_directory_subname() by templating it.
    • We no longer need <cstring> for strlen().
  • Make test_temp_directory resistant to misuse.
    • Mark it as a [[nodiscard]] guard type and make it noncopyable.
  • test_temp_directory's ctor/dtor can use local error_code ec;.
    • Nobody needed to access this as a public data member after construction.
  • Consistently name test_temp_directory, part 1: recursiveTests => tempDir
  • Consistently name test_temp_directory, part 2: followSymlinkTests => tempDir
  • 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.
  • Directly pass paths 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.
  • 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.
  • Skip affected rename() tests for _MSVC_INTERNAL_TESTING.

StephanTLavavej avatar May 10 '24 04:05 StephanTLavavej

I'm speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

StephanTLavavej avatar May 17 '24 19:05 StephanTLavavej