-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Improve P0218R1_filesystem
test reliability and temp filename generation
#4665
Merged
StephanTLavavej
merged 14 commits into
microsoft:main
from
StephanTLavavej:frosted-filesystem-flakes
May 21, 2024
Merged
Improve P0218R1_filesystem
test reliability and temp filename generation
#4665
StephanTLavavej
merged 14 commits into
microsoft:main
from
StephanTLavavej:frosted-filesystem-flakes
May 21, 2024
Conversation
This file contains 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
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 `<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).
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.
We no longer need `<cstring>` for strlen().
Mark it as a `[[nodiscard]]` guard type, and make it noncopyable.
Nobody needed to access this as a public data member after construction.
"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.
These calls to create_directories(), create_directory(), and exists() were totally inconsistent for no reason. We don't need to construct temporary identical paths.
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.
I'm speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
barcharcraz
approved these changes
May 20, 2024
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.
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.temp_file_name()
.tests/tr1/include/temp_file_name.h
totests/std/include/temp_file_name.hpp
, becausetests/std
can't usetests/tr1/include
. What I forgot is thattests/tr1
can usetests/std/include
, in both the GitHub and MSVC-internal test harnesses. Let's deduplicate these files now.<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).temp_file_name()
:"msvc_stl_"
prefix, 128 bits.".tmp"
extension is already clear, so let's change the prefix to make the source of these files obvious.tests/tr1
: Fix sporadic failures caused bytmpnam
/_tempnam
#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.get_test_directory_subname()
from 16 hexits / 64 bits to 32 hexits / 128 bits.get_test_directory
=>get_experimental_test_directory
get_new_test_directory
=>get_test_directory
<filesystem>
should be the default assumption.get_test_directory_subname()
by templating it.<cstring>
forstrlen()
.test_temp_directory
resistant to misuse.[[nodiscard]]
guard type and make it noncopyable.test_temp_directory
's ctor/dtor can use localerror_code ec;
.test_temp_directory
, part 1:recursiveTests
=>tempDir
test_temp_directory
, part 2:followSymlinkTests
=>tempDir
"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.path
s instead of.native()
.create_directories()
,create_directory()
, andexists()
were totally inconsistent for no reason. We don't need to construct temporary identical paths.ec
form (since we're not catching exceptions), verify thatec
is good, and verify that the source file no longer exists.rename()
tests for_MSVC_INTERNAL_TESTING
.