add Watchman-enabled functional tests to GitHub Actions CI#436
Merged
derrickstolee merged 5 commits intomicrosoft:mainfrom Oct 2, 2020
Merged
add Watchman-enabled functional tests to GitHub Actions CI#436derrickstolee merged 5 commits intomicrosoft:mainfrom
derrickstolee merged 5 commits intomicrosoft:mainfrom
Conversation
b1828ae to
604bbcd
Compare
In commit cbf6f82 in PR microsoft#349 the ValidateGitCommand() helper function was refactored slightly to use a new LinesShouldMatch() function; however, the inputs were accidentally reversed, so here we correct the order of the "expected" and "actual" string arguments passed to LinesShouldMatch().
604bbcd to
47b43ba
Compare
On macOS and Linux platforms, we use rename(2) directly when moving files with the SystemIORunner's MoveFile() method, instead of using .NET Core's File.Move() method. The latter actually implements most file moves using a combination of link(2) followed by unlink(2) on the source file. The consequence of using link() instead of rename() is that on recent versions of macOS, when running with Watchman, file creation events may not be delivered by Watchman to Git when a new hard link within the Git working tree is created if that link points to an existing inode outside of the watched tree. This in turn causes the MoveFileFromOutsideRepoToInsideRepoAndAdd() test in GitCommandsTests to fail intermittently. We can work around the problem by utilizing rename() directly, which also parallels the behaviour of SystemIORunner's RenameDirectory() method, added in commit 0a517fa. See also: facebook/watchman#858 https://github.com/dotnet/runtime/blob/94515f7bd34aabaab5ba6a1316f4a881cbf2370c/src/libraries/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs#L142-L144
We replicate for the Mac version of RunFunctionalTests.sh the changes made in commit da2b813 for the Linux version, which permit manual runs of the functional test suite after rebuilding the binaries by copying them into the directory where the Scalar.FunctionalTests binary was published, unless the --test-scalar-on-path option was given to the script.
47b43ba to
6b86640
Compare
derrickstolee
approved these changes
Oct 1, 2020
Contributor
derrickstolee
left a comment
There was a problem hiding this comment.
Thank you for adding these fixups and getting these builds running!
Contributor
Author
|
Thanks @derrickstolee for the review! It looks like you may need to temporarily disable the required Actions runs so we can merge this, as the PR is waiting on the older no-Watchman Actions jobs and those, of course, aren't running on this PR. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
We add Watchman-enabled runs of the functional test suite on all GitHub Actions runners for all three platforms.
The code changes in this PR are primarily the result of needing to work around some inconsistent behaviour from Watchman when running on macOS Catalina, which is used by the Actions macOS runners. As described in facebook/watchman#858, creating a hard link to an existing inode from inside the watched Git working tree may not be reported to Git as a new file if the inode's other, existing "source" path (as passed to
link(2)) resides outside the watched directory.This problem is tickled by our functional test suite because the
MoveFileFromOutsideRepoToInsideRepoAndAdd()test relies on theSystem.IO.File.Move()method, which on Unix/POSIX systems like macOS, actually performs alink(2)call rather thanrename(2).While the underlying problem is outside of Scalar's control, we can work around it by using
rename(2)directly to move files in theSystemIORunnertest helper class.This particular intermittent but persistent test failure also exposed another, minor issue in the
ValidateGitCommand()utility method, which passed its actual and expected string values to theLinesShouldMatch()method in the reverse order from what was intended, so we swap them here.Debugging these issues also turned up some minor typos in the error messages output from the
run clonecommand when a repository can not be initialized, so those are fixed here also.Finally, to enable the functional test suite to run in CI on macOS, we add to the Mac version of the
RunFunctionalTests.shscript so that like the Linux and Windows versions it copies the Scalar binary into the functional test binary's build location so it can be located by the functional test program.