Merge pull request #2241 from YuPeiHenry/2126-empty-directory-when-clone-fails

StanleyGoldman · web-flow · commit 0054884d903c · 2019-02-28T11:47:47.000-05:00
Remove empty directory when clone fails
diff --git a/src/GitHub.App/Services/RepositoryCloneService.cs b/src/GitHub.App/Services/RepositoryCloneService.cs
@@ -224,6 +224,7 @@ public async Task CloneRepository(
             catch (Exception ex)
             {
                 log.Error(ex, "Could not clone {CloneUrl} to {Path}", cloneUrl, repositoryPath);
+                operatingSystem.Directory.DeleteDirectory(repositoryPath);
                 throw;
             }
         }
diff --git a/test/GitHub.App.UnitTests/Services/RepositoryCloneServiceTests.cs b/test/GitHub.App.UnitTests/Services/RepositoryCloneServiceTests.cs
@@ -1,24 +1,24 @@
-﻿using System.Reactive.Linq;
+﻿using System;
+using System.Linq.Expressions;
+using System.Reactive.Linq;
 using System.Threading.Tasks;
+using GitHub.Api;
+using GitHub.Models;
+using GitHub.Services;
 using NSubstitute;
 using NUnit.Framework;
-using UnitTests;
-using GitHub.Services;
-using System.Linq.Expressions;
-using System;
-using GitHub.Models;
+using Rothko;
 
 public class RepositoryCloneServiceTests
 {
-    public class TheCloneRepositoryMethod : TestBaseClass
+    public class TheCloneRepositoryMethod
     {
         [Test]
         public async Task ClonesToRepositoryPathAsync()
         {
-            var serviceProvider = Substitutes.GetServiceProvider();
-            var operatingSystem = serviceProvider.GetOperatingSystem();
-            var vsGitServices = serviceProvider.GetVSGitServices();
-            var cloneService = CreateRepositoryCloneService(serviceProvider);
+            var operatingSystem = Substitute.For<IOperatingSystem>();
+            var vsGitServices = Substitute.For<IVSGitServices>();
+            var cloneService = CreateRepositoryCloneService(operatingSystem, vsGitServices);
 
             await cloneService.CloneRepository("https://github.com/foo/bar", @"c:\dev\bar");
 
@@ -34,9 +34,9 @@ public async Task ClonesToRepositoryPathAsync()
         [TestCase("https://enterprise.com/foo/bar", 0, nameof(UsageModel.MeasuresModel.NumberOfGitHubClones))]
         public async Task UpdatesMetricsWhenRepositoryClonedAsync(string cloneUrl, int numberOfCalls, string counterName)
         {
-            var serviceProvider = Substitutes.GetServiceProvider();
-            var usageTracker = serviceProvider.GetUsageTracker();
-            var cloneService = CreateRepositoryCloneService(serviceProvider);
+            var vsGitServices = Substitute.For<IVSGitServices>();
+            var usageTracker = Substitute.For<IUsageTracker>();
+            var cloneService = CreateRepositoryCloneService(vsGitServices: vsGitServices, usageTracker: usageTracker);
 
             await cloneService.CloneRepository(cloneUrl, @"c:\dev\bar");
             var model = UsageModel.Create(Guid.NewGuid());
@@ -58,8 +58,9 @@ public async Task Skip_OpenRepository_When_Already_Open(string repositoryPath, s
         {
             var repositoryUrl = "https://github.com/owner/repo";
             var cloneDialogResult = new CloneDialogResult(repositoryPath, repositoryUrl);
-            var serviceProvider = Substitutes.GetServiceProvider();
-            var operatingSystem = serviceProvider.GetOperatingSystem();
+            var operatingSystem = Substitute.For<IOperatingSystem>();
+            var serviceProvider = Substitute.For<IGitHubServiceProvider>();
+            var teamExplorerServices = Substitute.For<ITeamExplorerServices>();
             operatingSystem.Directory.DirectoryExists(repositoryPath).Returns(true);
             var dte = Substitute.For<EnvDTE.DTE>();
             serviceProvider.GetService<EnvDTE.DTE>().Returns(dte);
@@ -68,11 +69,11 @@ public async Task Skip_OpenRepository_When_Already_Open(string repositoryPath, s
             {
                 operatingSystem.Directory.DirectoryExists(solutionPath).Returns(true);
             }
-            var cloneService = CreateRepositoryCloneService(serviceProvider);
+            var cloneService = CreateRepositoryCloneService(operatingSystem: operatingSystem,
+                teamExplorerServices: teamExplorerServices, serviceProvider: serviceProvider);
 
             await cloneService.CloneOrOpenRepository(cloneDialogResult);
 
-            var teamExplorerServices = serviceProvider.GetTeamExplorerServices();
             teamExplorerServices.Received(openRepository).OpenRepository(repositoryPath);
         }
 
@@ -91,11 +92,10 @@ public async Task UpdatesMetricsWhenCloneOrOpenRepositoryAsync(string cloneUrl,
         {
             var repositoryPath = @"c:\dev\bar";
             var cloneDialogResult = new CloneDialogResult(repositoryPath, cloneUrl);
-            var serviceProvider = Substitutes.GetServiceProvider();
-            var operatingSystem = serviceProvider.GetOperatingSystem();
+            var operatingSystem = Substitute.For<IOperatingSystem>();
+            var usageTracker = Substitute.For<IUsageTracker>();
             operatingSystem.Directory.DirectoryExists(repositoryPath).Returns(dirExists);
-            var usageTracker = serviceProvider.GetUsageTracker();
-            var cloneService = CreateRepositoryCloneService(serviceProvider);
+            var cloneService = CreateRepositoryCloneService(operatingSystem: operatingSystem, usageTracker: usageTracker);
 
             await cloneService.CloneOrOpenRepository(cloneDialogResult);
 
@@ -108,11 +108,10 @@ await usageTracker.Received(numberOfCalls).IncrementCounter(
         [TestCase(@"c:\not_default\repo", @"c:\default", 0, nameof(UsageModel.MeasuresModel.NumberOfClonesToDefaultClonePath))]
         public async Task UpdatesMetricsWhenDefaultClonePath(string targetPath, string defaultPath, int numberOfCalls, string counterName)
         {
-            var serviceProvider = Substitutes.GetServiceProvider();
-            var vsGitServices = serviceProvider.GetVSGitServices();
+            var vsGitServices = Substitute.For<IVSGitServices>();
             vsGitServices.GetLocalClonePathFromGitProvider().Returns(defaultPath);
-            var usageTracker = serviceProvider.GetUsageTracker();
-            var cloneService = CreateRepositoryCloneService(serviceProvider);
+            var usageTracker = Substitute.For<IUsageTracker>();
+            var cloneService = CreateRepositoryCloneService(usageTracker: usageTracker, vsGitServices: vsGitServices);
 
             await cloneService.CloneRepository("https://github.com/foo/bar", targetPath);
             var model = UsageModel.Create(Guid.NewGuid());
@@ -122,10 +121,36 @@ await usageTracker.Received(numberOfCalls).IncrementCounter(
                     ((MemberExpression)x.Body).Member.Name == counterName));
         }
 
-        static RepositoryCloneService CreateRepositoryCloneService(IGitHubServiceProvider sp)
+        [TestCase("https://github.com/failing/url", @"c:\dev\bar")]
+        public async Task CleansDirectoryOnCloneFailed(string cloneUrl, string clonePath)
         {
-            return new RepositoryCloneService(sp.GetOperatingSystem(), sp.GetVSGitServices(), sp.GetTeamExplorerServices(),
-                sp.GetGraphQLClientFactory(), sp.GetGitHubContextService(), sp.GetUsageTracker(), sp);
+            var operatingSystem = Substitute.For<IOperatingSystem>();
+            var vsGitServices = Substitute.For<IVSGitServices>();
+            vsGitServices.Clone(cloneUrl, clonePath, true).Returns(x => { throw new Exception(); });
+            var cloneService = CreateRepositoryCloneService(operatingSystem: operatingSystem, vsGitServices: vsGitServices);
+
+            Assert.ThrowsAsync<Exception>(() => cloneService.CloneRepository(cloneUrl, clonePath));
+
+            operatingSystem.Directory.Received().CreateDirectory(clonePath);
+            operatingSystem.Directory.Received().DeleteDirectory(clonePath);
+            await vsGitServices.Received().Clone(cloneUrl, clonePath, true);
+        }
+
+        static RepositoryCloneService CreateRepositoryCloneService(IOperatingSystem operatingSystem = null,
+            IVSGitServices vsGitServices = null, IUsageTracker usageTracker = null,
+            ITeamExplorerServices teamExplorerServices = null, IGitHubServiceProvider serviceProvider = null)
+        {
+            operatingSystem = operatingSystem ?? Substitute.For<IOperatingSystem>();
+            vsGitServices = vsGitServices ?? Substitute.For<IVSGitServices>();
+            usageTracker = usageTracker ?? Substitute.For<IUsageTracker>();
+            teamExplorerServices = teamExplorerServices ?? Substitute.For<ITeamExplorerServices>();
+            serviceProvider = serviceProvider ?? Substitute.For<IGitHubServiceProvider>();
+
+            operatingSystem.Environment.ExpandEnvironmentVariables(Args.String).Returns(x => x[0]);
+
+            return new RepositoryCloneService(operatingSystem, vsGitServices, teamExplorerServices,
+                Substitute.For<IGraphQLClientFactory>(), Substitute.For<IGitHubContextService>(),
+                usageTracker, serviceProvider);
         }
     }
 }
diff --git a/test/GitHub.App.UnitTests/Substitutes.cs b/test/GitHub.App.UnitTests/Substitutes.cs
@@ -30,32 +30,8 @@ public static T1 For<T1, T2, T3, T4>(params object[] constructorArguments)
             }, constructorArguments);
         }
 
-
-        // public static IGitRepositoriesExt IGitRepositoriesExt { get { return Substitute.For<IGitRepositoriesExt>(); } }
         public static IGitService IGitService { get { return Substitute.For<IGitService>(); } }
 
-        public static IVSGitServices IVSGitServices
-        {
-            get
-            {
-                var ret = Substitute.For<IVSGitServices>();
-                ret.GetLocalClonePathFromGitProvider().Returns(@"c:\foo\bar");
-                return ret;
-            }
-        }
-
-        public static IOperatingSystem OperatingSystem
-        {
-            get
-            {
-                var ret = Substitute.For<IOperatingSystem>();
-                // this expansion happens when the GetLocalClonePathFromGitProvider call is setup by default
-                // see IVSServices property above
-                ret.Environment.ExpandEnvironmentVariables(Args.String).Returns(x => x[0]);
-                return ret;
-            }
-        }
-
         public static IViewViewModelFactory ViewViewModelFactory { get { return Substitute.For<IViewViewModelFactory>(); } }
 
         public static IRepositoryCreationService RepositoryCreationService { get { return Substitute.For<IRepositoryCreationService>(); } }
@@ -109,9 +85,8 @@ public static IGitHubServiceProvider GetServiceProvider(
             ret.GetService(typeof(SComponentModel)).Returns(cm);
             Services.UnitTestServiceProvider = ret;
 
-            var os = OperatingSystem;
-            var vsgit = IVSGitServices;
-            var clone = cloneService ?? new RepositoryCloneService(os, vsgit, Substitute.For<ITeamExplorerServices>(),
+            var clone = cloneService ?? new RepositoryCloneService(Substitute.For<IOperatingSystem>(),
+                Substitute.For<IVSGitServices>(), Substitute.For<ITeamExplorerServices>(),
                 Substitute.For<IGraphQLClientFactory>(), Substitute.For<IGitHubContextService>(),
                 Substitute.For<IUsageTracker>(), ret);
             var create = creationService ?? new RepositoryCreationService(clone);
@@ -123,8 +98,8 @@ public static IGitHubServiceProvider GetServiceProvider(
             ret.GetService(typeof(IGitHubContextService)).Returns(Substitute.For<IGitHubContextService>());
             ret.GetService(typeof(IVSGitExt)).Returns(Substitute.For<IVSGitExt>());
             ret.GetService(typeof(IUsageTracker)).Returns(Substitute.For<IUsageTracker>());
-            ret.GetService(typeof(IVSGitServices)).Returns(vsgit);
-            ret.GetService(typeof(IOperatingSystem)).Returns(os);
+            ret.GetService(typeof(IVSGitServices)).Returns(Substitute.For<IVSGitServices>());
+            ret.GetService(typeof(IOperatingSystem)).Returns(Substitute.For<IOperatingSystem>());
             ret.GetService(typeof(IRepositoryCloneService)).Returns(clone);
             ret.GetService(typeof(IRepositoryCreationService)).Returns(create);
             ret.GetService(typeof(IViewViewModelFactory)).Returns(ViewViewModelFactory);

Original file line number	Diff line number	Diff line change
`@@ -224,6 +224,7 @@ public async Task CloneRepository(`
`224`	`224`	`catch (Exception ex)`
`225`	`225`	`{`
`226`	`226`	`log.Error(ex, "Could not clone {CloneUrl} to {Path}", cloneUrl, repositoryPath);`
	`227`	`+ operatingSystem.Directory.DeleteDirectory(repositoryPath);`
`227`	`228`	`throw;`
`228`	`229`	`}`
`229`	`230`	`}`