enable FetchStepTests on Mac/Linux and remove MacTODO test categories #426
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
The FetchStepCleansUpStaleFetchLock() functional test currently only succeeds on Windows (and is therefore marked with the MacTODO.TestNeedsToLockFile category) because it checks whether the FetchStep command has released its lock file by testing if the lock file has been deleted. This works with our WindowsFileBasedLock implementation, which opens the lock file with FileOptions.DeleteOnClose, so the file is removed when the lock is released. However, the MacFileBasedLock version leaves the file in place and uses flock(2) and LOCK_EX to acquire and release the lock atomically; there is no simple way to delete the file while still holding the open file descriptor needed for flock(). To make this test succeed on Mac/Linux while still being meaningful, we need to be robust against the lock file existing both at the start of the test and afterwards, while still ensuring that the lock has been released by the FetchStep command. We therefore simply attempt to open the lock file after the fetch command completes, using the FileShare.None attribute, which will fail on all platforms if the lock file has not been released. On Windows this is because WindowsFileBasedLock uses FileShare.Read to open the lock file, so if the lock is still held our attempt with FileShare.None will fail. On macOS (and Linux) this is because our use of flock() in MacFileBasedLock will conflict with the flock() with LOCK_EX which the .NET Core Unix implementation of FileShare.None also uses. See the discussions in microsoft/VFSForGit#213, dotnet/runtime#24432, as well as the .NET Core library implementation in: https://github.com/dotnet/runtime/blob/341b8b22280e297daa7d96b4b8a9fa1d40d28aa9/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Unix.cs#L73-L88 We also rename the test to FetchStepReleasesFetchLockFile() to more accurately reflect what it is now testing.
Having addressed the issues preventing the tests in the MacTODO.TestNeedsToLockFile category from running on macOS, we can now remove this class of categories entirely.
derrickstolee
approved these changes
Sep 8, 2020
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.
The
FetchStepCleansUpStaleFetchLock()functional test currently only succeeds on Windows (and is therefore marked with theMacTODO.TestNeedsToLockFilecategory) because it checks whether theFetchStepcommand has released its lock file by testing if the lock file has been deleted.This works with our
WindowsFileBasedLockimplementation, which opens the lock file withFileOptions.DeleteOnClose, so the file is removed when the lock is released. However, theMacFileBasedLockversion leaves the file in place and usesflock(2)andLOCK_EXto acquire and release the lock atomically; there is no simple way to delete the file while still holding the open file descriptor needed forflock().To make this test succeed on Mac/Linux while still being meaningful, we need to be robust against the lock file existing both at the start of the test and afterwards, while still ensuring that the lock has been released by the
FetchStepcommand.We therefore simply attempt to open the lock file after the fetch command completes, using the
FileShare.Noneattribute, which will fail on all platforms if the lock file has not been released.On Windows this is because
WindowsFileBasedLockusesFileShare.Readto open the lock file, so if the lock is still held our attempt withFileShare.Nonewill fail.On macOS (and Linux) this is because our use of
flock()inMacFileBasedLockwill conflict with theflock()withLOCK_EXwhich the .NET Core Unix implementation ofFileShare.Nonealso uses.See the discussions in microsoft/VFSForGit#213, dotnet/runtime#24432, as well as the .NET Core library implementation.
We also rename the test to
FetchStepReleasesFetchLockFile()to more accurately reflect what it is now testing.And having now addressed the issues preventing the tests in the
MacTODO.TestNeedsToLockFilecategory from running on macOS, we can now remove this class of categories entirely.