Skip to content
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

Run GitRepoTests in sparse mode #84

Merged
merged 20 commits into from
Aug 27, 2019
Merged

Run GitRepoTests in sparse mode #84

merged 20 commits into from
Aug 27, 2019

Conversation

derrickstolee
Copy link
Contributor

@derrickstolee derrickstolee commented Aug 23, 2019

Resolves #77.

Update the functional tests to initialize a sparse enlistment with scalar sparse --add-stdin and to test lots of Git commands in that mode versus a vanilla Git repo. Uses the SparseMode checks from microsoft/vfsforgit to verify the contents inside the cone match, but ignores all changes outside the cone.

This took a lot longer than anticipated. Here are a few things that happened along the way:

  • The untracked cache doesn't work correctly on Windows. The functional tests were failing due to quickly running status after every call. This was working correctly on Mac, so maybe we re-enable it on a platform-specific basis later. Reverts Use untracked cache by default #86 until we can revisit.

  • The clone verb was causing problems. This is due to some timing concerns, mostly: VFS for Git needs a checkout to construct an index before mounting, but we can't checkout before mounting. Since the sparse verb doesn't prefetch before checkout (it doesn't need everything, but needs some things), we needed to change this order. However, the CloneVerb code is very convoluted. It required a lot of refactoring to split out that checkout code. Hopefully the new code is cleaner and easier to modify.

  • The input for the cone matches is slightly different for Git: we need / as the path separator, but the logic for interpreting those paths in the test code uses the platform-specific separator.

  • The parallel test scripts were causing new issues in the LooseObjectStepTests. Turns out that in Fix LooseObjectStepTests #68, I messed up the base constructor parameters and didn't give that enlistment a local object cache. That meant that other clones had prefetches and loose object downloads causing different numbers in these tests. That's fixed.

  • CloneWithDefaultLocalCacheLocation() was failing on the build machine but not on my laptop. The error appeared to be due to failing to register the mount with the service. Turns out, this test used to use the --no-mount option, but that was removed in CloneVerb: remove --no-mount option #80. Oops! Maybe we'll put that back as an --internal_use_only parameter. OR we can figure out why the service isn't running during that test.

@derrickstolee derrickstolee added this to the Demo milestone Aug 23, 2019
@derrickstolee
Copy link
Contributor Author

This is harder than anticipated. Looks like CloneVerb needs some work because I'm having non-deterministic failures depending on timing of parallel tests. The logic around mounting and checking out the tip file is not quite working all the time. Will dig deep on this.

derrickstolee and others added 13 commits August 26, 2019 13:41
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <stolee@gmail.com>
@derrickstolee derrickstolee changed the title [WIP] Run GitRepoTests in sparse mode Run GitRepoTests in sparse mode Aug 27, 2019
@derrickstolee derrickstolee marked this pull request as ready for review August 27, 2019 10:50
Copy link
Member

@jrbriggs jrbriggs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good. A few nits/questions.

Scalar/CommandLine/CloneVerb.cs Show resolved Hide resolved
Scalar/CommandLine/CloneVerb.cs Show resolved Hide resolved
Scalar/CommandLine/CloneVerb.cs Outdated Show resolved Hide resolved
Scalar/CommandLine/CloneVerb.cs Outdated Show resolved Hide resolved
@wilbaker
Copy link
Member

The untracked cache doesn't work correctly on Windows.
This was working correctly on Mac, so maybe we re-enable it on a platform-specific basis later. Reverts #86 until we can revisit.

How much of a perf hit was this on Mac? I'm wondering if we should just do the platform-specific change now so that it can be on during the demo.

@derrickstolee
Copy link
Contributor Author

The untracked cache doesn't work correctly on Windows.
This was working correctly on Mac, so maybe we re-enable it on a platform-specific basis later. Reverts #86 until we can revisit.

How much of a perf hit was this on Mac? I'm wondering if we should just do the platform-specific change now so that it can be on during the demo.

I want to come back immediately after this PR with the isolated change for the untracked cache. It's a decent hit to the git status perf on Mac, but I want to make sure that change is not included with this massive PR.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Copy link
Member

@wilbaker wilbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with some minor suggestions

Scalar.FunctionalTests/Tools/ProcessHelper.cs Show resolved Hide resolved
Scalar.FunctionalTests/Tools/ScalarProcess.cs Outdated Show resolved Hide resolved
Scalar.FunctionalTests/Tests/GitCommands/GitRepoTests.cs Outdated Show resolved Hide resolved
Scalar/CommandLine/CloneVerb.cs Outdated Show resolved Hide resolved
Scalar/CommandLine/CloneVerb.cs Show resolved Hide resolved
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
@derrickstolee derrickstolee merged commit 38070a1 into microsoft:master Aug 27, 2019
@derrickstolee derrickstolee deleted the sparse branch November 18, 2019 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sparse: Update functional tests to check sparse mode
3 participants