Tests can't run in parallel. #826

nulltoken · 2015-01-02T21:46:50Z

Currently, when you try to run the tests in parallel they run into file contention issues where one of the tests will take out a lock on some file and then another test fails because it can't open the file. Finding an optimal solution would require more knowledge than I have, but my first thought would be to have each test copy the resources it needs before using them, rather than having multiple tests utilize the same set of resources.

nulltoken · 2014-09-14T11:04:53Z

Labeled as "up-for grabs".

nulltoken · 2014-09-16T20:42:55Z

@Zoltu Based on #825, I've quickly hacked something that "looks" like working with both <AllowParallelTestExecution/> and <AllowTestsToRunInParallelWithThemselves/> set to true.

This proposal should avoid the issues that you've encountered. However, the drawback is that the recursively copied Resources/Easy-To-Remember-Fancy-Guid directory never gets deleted. Which is less than a rich experience :/ Any ideas?

Note: This doesn't sandbox (BaseFixture.Clone()) the tests that do not alter the state of the test repositories.

Could you please double check and see if maybe his could get wrapped into a nice PR?

--- a/LibGit2Sharp.Tests/TestHelpers/BaseFixture.cs
+++ b/LibGit2Sharp.Tests/TestHelpers/BaseFixture.cs
@@ -21,7 +21,7 @@ static BaseFixture()
             // Do the set up in the static ctor so it only happens once
             SetUpTestEnvironment();

-            DirectoryHelper.DeleteSubdirectories(Constants.TemporaryReposPath);
+//            DirectoryHelper.DeleteSubdirectories(Constants.TemporaryReposPath);
         }

         public static string BareTestRepoPath { get; private set; }
@@ -49,12 +49,12 @@ private static void SetUpTestEnvironment()

             var source = new DirectoryInfo(@"../../Resources");
             ResourcesDirectory = new DirectoryInfo(string.Format(@"Resources/{0}", Guid.NewGuid()));
-            var parent = new DirectoryInfo(@"Resources");
+            //var parent = new DirectoryInfo(@"Resources");

-            if (parent.Exists)
-            {
-                DirectoryHelper.DeleteSubdirectories(parent.FullName);
-            }
+            //if (parent.Exists)
+            //{
+            //    //DirectoryHelper.DeleteSubdirectories(parent.FullName);
+            //}

             DirectoryHelper.CopyFilesRecursively(source, ResourcesDirectory);

MicahZoltu · 2014-09-17T02:13:07Z

What I have done to solve this sort of problem in the past was have the fixture setup for each test copy a clean version of whatever disk resource it needed to work with into a directory named Guid.NewGuid(). Then each test would work inside of its own copy of the folder (only it knows the Guid.NewGuid path) and delete it when done.

Of course, if you have a lot of tests like this you end up bottlenecked a bit on disk IO. I do not believe there is any way to take an action after all of the tests are finished, especially in an environment like NCrunch where the tests are often times running non-stop, just cycling through them.

Also, depending on the testing framework you can end up with resources not being cleaned up. In my experience, xUnit (2.0) always executes the fixture Dispose whereas nUnit will only execute the test tear down when the test setup was successful.

My concern with your proposed solution would be the fact that depending on the test runner, it may not always be obvious to the end-user that there are uncleaned up resources left around on disk. In the case of NCrunch, it puts all of its stuff somewhere buried in %appdata% (C:\Users\Micah\AppData\Local\NCrunch\252\7\LibGit2Sharp.Tests\bin\Debug\Resources for my latest run, the numbered directories may be different for you). I would be concerned that over time, my NCrunch directory would fill up with these files, though NCrunch does seem to clean them up on resync.

That all being said, it does appear to allow the tests to run reliably and they are much faster.

In a perfect (and largely unrealistic) world, I would recommend mocking the file system calls and running all of the tests against a virtual (in memory) filesystem (no disk IO at all). I'm guessing that would be a lot of work though, and probably not worth it.

nulltoken · 2014-09-17T05:32:19Z

That all being said, it does appear to allow the tests to run reliably and they are much faster.

Great!

My concern with your proposed solution would be the fact that depending on the test runner, it may not always be obvious to the end-user that there are uncleaned up resources left around on disk

I completely agree. How about this?

No more intermediate guid'd copy of the test repos.
The dynamic renaming (dot_git -> .git) is now only performed when sandboxing the test repo.
Each test that was previously directly accessing the repository has now to sandbox it (Done below on the first three test suites).
Sandboxed repositories are removed when Dispose() is called.

diff --git a/LibGit2Sharp.Tests/ArchiveFixture.cs b/LibGit2Sharp.Tests/ArchiveFixture.cs
index e070db8..c5b09df 100644
--- a/LibGit2Sharp.Tests/ArchiveFixture.cs
+++ b/LibGit2Sharp.Tests/ArchiveFixture.cs
@@ -11,7 +11,8 @@ public class ArchiveFixture : BaseFixture
         [Fact]
         public void CanArchiveATree()
         {
-            using (var repo = new Repository(BareTestRepoPath))
+            string path = CloneBareTestRepo();
+            using (var repo = new Repository(path))
             {
                 var tree = repo.Lookup<Tree>("581f9824ecaf824221bd36edf5430f2739a7c4f5");

@@ -36,7 +37,8 @@ public void CanArchiveATree()
         [Fact]
         public void CanArchiveACommit()
         {
-            using (var repo = new Repository(BareTestRepoPath))
+            string path = CloneBareTestRepo();
+            using (var repo = new Repository(path))
             {
                 var commit = repo.Lookup<Commit>("4c062a6361ae6959e06292c1fa5e2822d9c96345");

@@ -61,7 +63,8 @@ public void CanArchiveACommit()
         [Fact]
         public void ArchivingANullTreeOrCommitThrows()
         {
-            using (var repo = new Repository(BareTestRepoPath))
+            string path = CloneBareTestRepo();
+            using (var repo = new Repository(path))
             {
                 Assert.Throws<ArgumentNullException>(() => repo.ObjectDatabase.Archive((Commit)null, null));
                 Assert.Throws<ArgumentNullException>(() => repo.ObjectDatabase.Archive((Tree)null, null));
diff --git a/LibGit2Sharp.Tests/BlameFixture.cs b/LibGit2Sharp.Tests/BlameFixture.cs
index b0fcf77..bdfa46f 100644
--- a/LibGit2Sharp.Tests/BlameFixture.cs
+++ b/LibGit2Sharp.Tests/BlameFixture.cs
@@ -22,7 +22,8 @@ private static void AssertCorrectHeadBlame(BlameHunkCollection blame)
         [Fact]
         public void CanBlameSimply()
         {
-            using (var repo = new Repository(BareTestRepoPath))
+            string path = CloneBareTestRepo();
+            using (var repo = new Repository(path))
             {
                 AssertCorrectHeadBlame(repo.Blame("README"));
             }
@@ -31,7 +32,8 @@ public void CanBlameSimply()
         [Fact]
         public void CanBlameFromADifferentCommit()
         {
-            using (var repo = new Repository(MergedTestRepoWorkingDirPath))
+            string path = CloneMergedTestRepo();
+            using (var repo = new Repository(path))
             {
                 // File doesn't exist at HEAD
                 Assert.Throws<LibGit2SharpException>(() => repo.Blame("ancestor-only.txt"));
@@ -44,7 +46,8 @@ public void CanBlameFromADifferentCommit()
         [Fact]
         public void ValidatesLimits()
         {
-            using (var repo = new Repository(BareTestRepoPath))
+            string path = CloneBareTestRepo();
+            using (var repo = new Repository(path))
             {
                 var blame = repo.Blame("README");

@@ -56,7 +59,8 @@ public void ValidatesLimits()
         [Fact]
         public void CanBlameFromVariousTypes()
         {
-            using (var repo = new Repository(BareTestRepoPath))
+            string path = CloneBareTestRepo();
+            using (var repo = new Repository(path))
             {
                 AssertCorrectHeadBlame(repo.Blame("README", new BlameOptions {StartingAt = "HEAD" }));
                 AssertCorrectHeadBlame(repo.Blame("README", new BlameOptions {StartingAt = repo.Head }));
@@ -68,7 +72,8 @@ public void CanBlameFromVariousTypes()
         [Fact]
         public void CanStopBlame()
         {
-            using (var repo = new Repository(BareTestRepoPath))
+            string path = CloneBareTestRepo();
+            using (var repo = new Repository(path))
             {
                 // $ git blame .\new.txt
                 // 9fd738e8 (Scott Chacon 2010-05-24 10:19:19 -0700 1) my new file
diff --git a/LibGit2Sharp.Tests/BlobFixture.cs b/LibGit2Sharp.Tests/BlobFixture.cs
index f615aed..e9dc554 100644
--- a/LibGit2Sharp.Tests/BlobFixture.cs
+++ b/LibGit2Sharp.Tests/BlobFixture.cs
@@ -12,7 +12,8 @@ public class BlobFixture : BaseFixture
         [Fact]
         public void CanGetBlobAsText()
         {
-            using (var repo = new Repository(BareTestRepoPath))
+            string path = CloneBareTestRepo();
+            using (var repo = new Repository(path))
             {
                 var blob = repo.Lookup<Blob>("a8233120f6ad708f843d861ce2b7228ec4e3dec6");

@@ -86,7 +87,8 @@ public void CanGetBlobAsTextWithVariousEncodings(string encodingName, int expect
         [Fact]
         public void CanGetBlobSize()
         {
-            using (var repo = new Repository(BareTestRepoPath))
+            string path = CloneBareTestRepo();
+            using (var repo = new Repository(path))
             {
                 var blob = repo.Lookup<Blob>("a8233120f6ad708f843d861ce2b7228ec4e3dec6");
                 Assert.Equal(10, blob.Size);
@@ -106,7 +108,8 @@ public void CanLookUpBlob()
         [Fact]
         public void CanReadBlobStream()
         {
-            using (var repo = new Repository(BareTestRepoPath))
+            string path = CloneBareTestRepo();
+            using (var repo = new Repository(path))
             {
                 var blob = repo.Lookup<Blob>("a8233120f6ad708f843d861ce2b7228ec4e3dec6");

@@ -208,7 +211,8 @@ public void CanStageAFileGeneratedFromABlobContentStream()
         [Fact]
         public void CanTellIfTheBlobContentLooksLikeBinary()
         {
-            using (var repo = new Repository(BareTestRepoPath))
+            string path = CloneBareTestRepo();
+            using (var repo = new Repository(path))
             {
                 var blob = repo.Lookup<Blob>("a8233120f6ad708f843d861ce2b7228ec4e3dec6");
                 Assert.Equal(false, blob.IsBinary);
diff --git a/LibGit2Sharp.Tests/TestHelpers/BaseFixture.cs b/LibGit2Sharp.Tests/TestHelpers/BaseFixture.cs
index 2cab50d..a94f84b 100644
--- a/LibGit2Sharp.Tests/TestHelpers/BaseFixture.cs
+++ b/LibGit2Sharp.Tests/TestHelpers/BaseFixture.cs
@@ -48,26 +48,17 @@ private static void SetUpTestEnvironment()
             IsFileSystemCaseSensitive = IsFileSystemCaseSensitiveInternal();

             var source = new DirectoryInfo(@"../../Resources");
-            ResourcesDirectory = new DirectoryInfo(string.Format(@"Resources/{0}", Guid.NewGuid()));
-            var parent = new DirectoryInfo(@"Resources");
-
-            if (parent.Exists)
-            {
-                DirectoryHelper.DeleteSubdirectories(parent.FullName);
-            }
-
-            DirectoryHelper.CopyFilesRecursively(source, ResourcesDirectory);

             // Setup standard paths to our test repositories
-            BareTestRepoPath = Path.Combine(ResourcesDirectory.FullName, "testrepo.git");
-            StandardTestRepoWorkingDirPath = Path.Combine(ResourcesDirectory.FullName, "testrepo_wd");
+            BareTestRepoPath = Path.Combine(source.FullName, "testrepo.git");
+            StandardTestRepoWorkingDirPath = Path.Combine(source.FullName, "testrepo_wd");
             StandardTestRepoPath = Path.Combine(StandardTestRepoWorkingDirPath, ".git");
-            ShallowTestRepoPath = Path.Combine(ResourcesDirectory.FullName, "shallow.git");
-            MergedTestRepoWorkingDirPath = Path.Combine(ResourcesDirectory.FullName, "mergedrepo_wd");
-            MergeRenamesTestRepoWorkingDirPath = Path.Combine(ResourcesDirectory.FullName, "mergerenames_wd");
-            MergeTestRepoWorkingDirPath = Path.Combine(ResourcesDirectory.FullName, "merge_testrepo_wd");
-            RevertTestRepoWorkingDirPath = Path.Combine(ResourcesDirectory.FullName, "revert_testrepo_wd");
-            SubmoduleTestRepoWorkingDirPath = Path.Combine(ResourcesDirectory.FullName, "submodule_wd");
+            ShallowTestRepoPath = Path.Combine(source.FullName, "shallow.git");
+            MergedTestRepoWorkingDirPath = Path.Combine(source.FullName, "mergedrepo_wd");
+            MergeRenamesTestRepoWorkingDirPath = Path.Combine(source.FullName, "mergerenames_wd");
+            MergeTestRepoWorkingDirPath = Path.Combine(source.FullName, "merge_testrepo_wd");
+            RevertTestRepoWorkingDirPath = Path.Combine(source.FullName, "revert_testrepo_wd");
+            SubmoduleTestRepoWorkingDirPath = Path.Combine(source.FullName, "submodule_wd");
         }

         private static bool IsFileSystemCaseSensitiveInternal()

MicahZoltu · 2014-10-01T15:48:16Z

This approach seems reasonable, though I am hesitant to say much since I am not familiar with the bigger picture.

Another option that is nCrunch specific would be this: http://www.ncrunch.net/documentation/reference_runtime-framework_exclusively-uses-attribute

You can use attributes to teach nCrunch that certain tests shouldn't be run in parallel because they both use the same resource. I'm not a huge fan of this since the solution only works with nCrunch, not any other parallel test runner. However, it would make it so you didn't need to keep copying/deleting directories when testing, making sequential runs faster.

I think the most "right" solution (though also the most work by far) would be to mock out the filesystem calls throughout libgit2 and use some kind of dependency injection system to fulfill them at runtime with either an in-memory mock filesystem or the real thing. Then you could execute all of these tests without going to disk at all. I would be up for discussing this more if you like, just be aware that it is a large undertaking.

frankshearar · 2014-12-31T21:45:54Z

@Zoltu You're talking about intercepting the file system calls that libgit2 makes?

As opposed to pulling an interface out of Proxy and injecting a mock/stub version into Repository in tests?

MicahZoltu · 2015-01-01T18:52:49Z

I mean that anywhere in the codebase where you do something like File.Open, File.ReadAllBytes, etc. you instead do something like iFile.Open or iFile.ReadAllBytes and you would pass around an implementation of IFile (iFile) throughout the application.

IFile would be some interface with an implementation that simply calls the framework File class in production but would be mocked in tests.

shiftkey · 2015-01-02T03:00:58Z

A few thoughts on this thread:

We went down a similar path ages ago with our tests for GitHub for Windows. The overall harness looks like this:

[Fact]
public async Task TestSomethingWithRepository()
{
    using (var fixture = IntegrationTestHelper.GetTestRepo("mergedrepo_wd"))
    {
       // test code here
    }
}

The role of GetTestRepo is to extract the repository resource (it's archived and compressed) to a temporary directory, and then cleanup after the variable is de-scoped. fixture is a POCO contains the file paths on disk for the test to use.

So its not too different to what @nulltoken was working towards earlier, but we have some extra work after the test runs.

We switched over to use xUnit 2.0 betas and enable parallel tests from the command line. Of all the things we had to work through as part of this process, none of them were related to filesystem access in these tests - so I believe you're on the right track here.
Please don't go down the path of mocking out filesystem calls for testing purposes - these are super-important to Git, and will help detect any platform-specific issues that might come up.

@nulltoken @Zoltu I've not had a chance to look at the current state of affairs for the libgit2sharp tests - anything I can do here to help move things along?

Fix #826

nulltoken · 2015-01-02T21:53:48Z

This issue has been transmuted into a proper Pull Request.

I renamed the Clone() test helper method into a more meaningful (at least I hope so) Sandbox() one, then proceeded to isolate every test so it's properly isolated from other runs.

The NCrunch solution settings has been tweaked so that it now allows every test to be run in parallel.

I'd really like a deep review on this as this is the kind of huge change which is a magnet for mistakes and overlooks.

Therzok · 2015-01-03T03:31:51Z

👍

frankshearar · 2015-01-03T23:40:55Z

I did find I needed one extra change:

--- a/LibGit2Sharp.Tests/TestHelpers/BaseFixture.cs
+++ b/LibGit2Sharp.Tests/TestHelpers/BaseFixture.cs
@@ -58,12 +58,6 @@ private static void SetUpTestEnvironment()
             var source = new DirectoryInfo(@"../../Resources");
             var resourcesRelativePath = string.Format(@"Resources/{0}", Guid.NewGuid());
             ResourcesDirectory = new DirectoryInfo(resourcesRelativePath);
-            var parent = new DirectoryInfo(@"Resources");
-
-            if (parent.Exists)
-            {
-                DirectoryHelper.DeleteSubdirectories(parent.FullName);
-            }

             DirectoryHelper.CopyFilesRecursively(source, ResourcesDirectory);

(Sorry about the lack of pretty colours: I can't find out how you did that!)

Without this, the BaseFixture will try to delete all the registered directories, all but one of which won't exist in an NCrunch workspace.

I think one could address this by throwing away the global cleanup in favour of a small helper, something like

    public class Sandbox : IDisposable
    {
        public string Path { get; private set; }

        public Sandbox(string path)
        {
            Path = path;
            // Resource setup: copying stuff etc.
        }

        public void Dispose()
        {
            // Delete the sandbox directory
        }
    }

    public sealed class Temp
    {
        public static Sandbox CleanSandbox()
        {
            return new Sandbox(someGeneratedPath);
        }
    }

Tests then just say

using (Temp.Sandbox()) {
    // Profit!
}

Each test then becomes responsible for resource cleanup, but keeping syntax nice & light.

Therzok · 2015-01-03T23:47:33Z

Strip the leading whitespace and use diff not git-diff. =D

nulltoken · 2015-01-04T15:55:01Z

@frankshearar Better now?

frankshearar · 2015-01-04T20:12:46Z

@nulltoken Thanks! Much better! Works out of the box for NCrunch. And using R#'s test runner, I can see the temp directories being deleted, so it seems like a win all round?

Fix #826

nulltoken · 2015-01-04T20:19:24Z

@frankshearar Thanks for the feedback 😍

I've rebased this. Let's make sure the CIs are still happy and let's get this in!

MicahZoltu mentioned this pull request Sep 13, 2014

Adds support for nCrunch. #825

Merged

nulltoken added the Up for grabs label Sep 14, 2014

nulltoken added a commit that referenced this pull request Jan 2, 2015

Sandbox every test

b1cff1b

Fix #826

nulltoken added this to the v0.21 milestone Jan 3, 2015

nulltoken removed the Up for grabs label Jan 4, 2015

nulltoken added 3 commits January 4, 2015 21:14

Rename Clone() test helper method in Sandbox()

7e57858

Sandbox every test

27fbde6

Fix #826

Prefer GetRandomFileName() over NewGuid() for paths

5971306

nulltoken force-pushed the ntk/ncrunch branch from b7a22f9 to 5971306 Compare January 4, 2015 20:16

nulltoken merged commit 5971306 into vNext Jan 4, 2015

nulltoken deleted the ntk/ncrunch branch January 4, 2015 21:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Tests can't run in parallel. #826

Tests can't run in parallel. #826

Uh oh!

nulltoken commented Jan 2, 2015

Uh oh!

nulltoken commented Sep 14, 2014

Uh oh!

nulltoken commented Sep 16, 2014

Uh oh!

MicahZoltu commented Sep 17, 2014

Uh oh!

nulltoken commented Sep 17, 2014

Uh oh!

MicahZoltu commented Oct 1, 2014

Uh oh!

frankshearar commented Dec 31, 2014

Uh oh!

MicahZoltu commented Jan 1, 2015

Uh oh!

shiftkey commented Jan 2, 2015

Uh oh!

nulltoken commented Jan 2, 2015

Uh oh!

Therzok commented Jan 3, 2015

Uh oh!

frankshearar commented Jan 3, 2015

Uh oh!

Therzok commented Jan 3, 2015

Uh oh!

nulltoken commented Jan 4, 2015

Uh oh!

frankshearar commented Jan 4, 2015

Uh oh!

nulltoken commented Jan 4, 2015

Uh oh!

Uh oh!

Tests can't run in parallel. #826

Tests can't run in parallel. #826

Uh oh!

Conversation

nulltoken commented Jan 2, 2015

Uh oh!

nulltoken commented Sep 14, 2014

Uh oh!

nulltoken commented Sep 16, 2014

Uh oh!

MicahZoltu commented Sep 17, 2014

Uh oh!

nulltoken commented Sep 17, 2014

Uh oh!

MicahZoltu commented Oct 1, 2014

Uh oh!

frankshearar commented Dec 31, 2014

Uh oh!

MicahZoltu commented Jan 1, 2015

Uh oh!

shiftkey commented Jan 2, 2015

Uh oh!

nulltoken commented Jan 2, 2015

Uh oh!

Therzok commented Jan 3, 2015

Uh oh!

frankshearar commented Jan 3, 2015

Uh oh!

Therzok commented Jan 3, 2015

Uh oh!

nulltoken commented Jan 4, 2015

Uh oh!

frankshearar commented Jan 4, 2015

Uh oh!

nulltoken commented Jan 4, 2015

Uh oh!

Uh oh!