From b42e08e517f2393eb57a0df0e1ffe0ee8d1a63d9 Mon Sep 17 00:00:00 2001 From: Yu Pei Henry Date: Tue, 26 Feb 2019 02:04:07 +0800 Subject: [PATCH 1/5] Fix residual empty directory when clone fails. - Currently an empty directory will be created when trying to clone non-existant remote repo. - This commit adds an action to delete the repo directory if an exception occurs while cloning. --- src/GitHub.App/Services/RepositoryCloneService.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/GitHub.App/Services/RepositoryCloneService.cs b/src/GitHub.App/Services/RepositoryCloneService.cs index f1df35a486..27d73cd63d 100644 --- a/src/GitHub.App/Services/RepositoryCloneService.cs +++ b/src/GitHub.App/Services/RepositoryCloneService.cs @@ -223,6 +223,7 @@ public async Task CloneRepository( } catch (Exception ex) { + operatingSystem.Directory.DeleteDirectory(repositoryPath); log.Error(ex, "Could not clone {CloneUrl} to {Path}", cloneUrl, repositoryPath); throw; } From 9d4235a740b649852138ddad21bc2257d8c791a8 Mon Sep 17 00:00:00 2001 From: Yu Pei Henry Date: Tue, 26 Feb 2019 11:58:41 +0800 Subject: [PATCH 2/5] Add test for empty directory on clone fail. --- .../Services/RepositoryCloneServiceTests.cs | 17 +++++++++++++++++ test/GitHub.App.UnitTests/Substitutes.cs | 1 + 2 files changed, 18 insertions(+) diff --git a/test/GitHub.App.UnitTests/Services/RepositoryCloneServiceTests.cs b/test/GitHub.App.UnitTests/Services/RepositoryCloneServiceTests.cs index d3941f8798..4f1ac4a6d5 100644 --- a/test/GitHub.App.UnitTests/Services/RepositoryCloneServiceTests.cs +++ b/test/GitHub.App.UnitTests/Services/RepositoryCloneServiceTests.cs @@ -122,6 +122,23 @@ await usageTracker.Received(numberOfCalls).IncrementCounter( ((MemberExpression)x.Body).Member.Name == counterName)); } + [Test] + public async Task CleansDirectoryOnCloneFailed() + { + var serviceProvider = Substitutes.GetServiceProvider(); + var operatingSystem = serviceProvider.GetOperatingSystem(); + var vsGitServices = serviceProvider.GetVSGitServices(); + var cloneService = CreateRepositoryCloneService(serviceProvider); + + Assert.ThrowsAsync(async () => { + await cloneService.CloneRepository("https://github.com/failing/url", @"c:\dev\bar"); + }); + + operatingSystem.Directory.Received().CreateDirectory(@"c:\dev\bar"); + operatingSystem.Directory.Received().DeleteDirectory(@"c:\dev\bar"); + await vsGitServices.Received().Clone("https://github.com/failing/url", @"c:\dev\bar", true); + } + static RepositoryCloneService CreateRepositoryCloneService(IGitHubServiceProvider sp) { return new RepositoryCloneService(sp.GetOperatingSystem(), sp.GetVSGitServices(), sp.GetTeamExplorerServices(), diff --git a/test/GitHub.App.UnitTests/Substitutes.cs b/test/GitHub.App.UnitTests/Substitutes.cs index ac707407f8..a0a73802e7 100644 --- a/test/GitHub.App.UnitTests/Substitutes.cs +++ b/test/GitHub.App.UnitTests/Substitutes.cs @@ -40,6 +40,7 @@ public static IVSGitServices IVSGitServices { var ret = Substitute.For(); ret.GetLocalClonePathFromGitProvider().Returns(@"c:\foo\bar"); + ret.Clone("https://github.com/failing/url", @"c:\dev\bar", true).Returns(x => { throw new Exception(); }); return ret; } } From 894875e4ef537fb3ac81a776036b17a6cecd649c Mon Sep 17 00:00:00 2001 From: Yu Pei Henry Date: Wed, 27 Feb 2019 09:52:11 +0800 Subject: [PATCH 3/5] GitHub.App RepositoryCloneService: Move log error. - Delete directory may throw errors, so log.Error should be called first. --- src/GitHub.App/Services/RepositoryCloneService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/GitHub.App/Services/RepositoryCloneService.cs b/src/GitHub.App/Services/RepositoryCloneService.cs index 27d73cd63d..9a27bf7dcd 100644 --- a/src/GitHub.App/Services/RepositoryCloneService.cs +++ b/src/GitHub.App/Services/RepositoryCloneService.cs @@ -223,8 +223,8 @@ public async Task CloneRepository( } catch (Exception ex) { - operatingSystem.Directory.DeleteDirectory(repositoryPath); log.Error(ex, "Could not clone {CloneUrl} to {Path}", cloneUrl, repositoryPath); + operatingSystem.Directory.DeleteDirectory(repositoryPath); throw; } } From c6c14372fdd43025a8c8bebb4b93e1d4b4d14650 Mon Sep 17 00:00:00 2001 From: Yu Pei Henry Date: Wed, 27 Feb 2019 10:14:07 +0800 Subject: [PATCH 4/5] RepositoryCloneServiceTests:Rewrite test for readability. - Assert.ThrowAsync statement have been shortened. - failing url and dev bar have been factored into clonePath and cloneUrl. --- .../Services/RepositoryCloneServiceTests.cs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/test/GitHub.App.UnitTests/Services/RepositoryCloneServiceTests.cs b/test/GitHub.App.UnitTests/Services/RepositoryCloneServiceTests.cs index 4f1ac4a6d5..152d856302 100644 --- a/test/GitHub.App.UnitTests/Services/RepositoryCloneServiceTests.cs +++ b/test/GitHub.App.UnitTests/Services/RepositoryCloneServiceTests.cs @@ -122,21 +122,19 @@ await usageTracker.Received(numberOfCalls).IncrementCounter( ((MemberExpression)x.Body).Member.Name == counterName)); } - [Test] - public async Task CleansDirectoryOnCloneFailed() + [TestCase("https://github.com/failing/url", @"c:\dev\bar")] + public async Task CleansDirectoryOnCloneFailed(string cloneUrl, string clonePath) { var serviceProvider = Substitutes.GetServiceProvider(); var operatingSystem = serviceProvider.GetOperatingSystem(); var vsGitServices = serviceProvider.GetVSGitServices(); var cloneService = CreateRepositoryCloneService(serviceProvider); - Assert.ThrowsAsync(async () => { - await cloneService.CloneRepository("https://github.com/failing/url", @"c:\dev\bar"); - }); + Assert.ThrowsAsync(() => cloneService.CloneRepository(cloneUrl, clonePath)); - operatingSystem.Directory.Received().CreateDirectory(@"c:\dev\bar"); - operatingSystem.Directory.Received().DeleteDirectory(@"c:\dev\bar"); - await vsGitServices.Received().Clone("https://github.com/failing/url", @"c:\dev\bar", true); + operatingSystem.Directory.Received().CreateDirectory(clonePath); + operatingSystem.Directory.Received().DeleteDirectory(clonePath); + await vsGitServices.Received().Clone(cloneUrl, clonePath, true); } static RepositoryCloneService CreateRepositoryCloneService(IGitHubServiceProvider sp) From 72c146d0605489b38104729ebfdda1a0acb528da Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Thu, 28 Feb 2019 16:09:16 +0000 Subject: [PATCH 5/5] Remove dependency on Substitutes class RepositoryCloneServiceTests and Substitutes were getting too coupled. This commit makes RepositoryCloneServiceTests self contained. --- .../Services/RepositoryCloneServiceTests.cs | 76 +++++++++++-------- test/GitHub.App.UnitTests/Substitutes.cs | 34 +-------- 2 files changed, 47 insertions(+), 63 deletions(-) diff --git a/test/GitHub.App.UnitTests/Services/RepositoryCloneServiceTests.cs b/test/GitHub.App.UnitTests/Services/RepositoryCloneServiceTests.cs index 152d856302..28ca979afe 100644 --- 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(); + var vsGitServices = Substitute.For(); + 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(); + var usageTracker = Substitute.For(); + 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(); + var serviceProvider = Substitute.For(); + var teamExplorerServices = Substitute.For(); operatingSystem.Directory.DirectoryExists(repositoryPath).Returns(true); var dte = Substitute.For(); serviceProvider.GetService().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(); + var usageTracker = Substitute.For(); 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(); vsGitServices.GetLocalClonePathFromGitProvider().Returns(defaultPath); - var usageTracker = serviceProvider.GetUsageTracker(); - var cloneService = CreateRepositoryCloneService(serviceProvider); + var usageTracker = Substitute.For(); + var cloneService = CreateRepositoryCloneService(usageTracker: usageTracker, vsGitServices: vsGitServices); await cloneService.CloneRepository("https://github.com/foo/bar", targetPath); var model = UsageModel.Create(Guid.NewGuid()); @@ -125,10 +124,10 @@ await usageTracker.Received(numberOfCalls).IncrementCounter( [TestCase("https://github.com/failing/url", @"c:\dev\bar")] public async Task CleansDirectoryOnCloneFailed(string cloneUrl, string clonePath) { - var serviceProvider = Substitutes.GetServiceProvider(); - var operatingSystem = serviceProvider.GetOperatingSystem(); - var vsGitServices = serviceProvider.GetVSGitServices(); - var cloneService = CreateRepositoryCloneService(serviceProvider); + var operatingSystem = Substitute.For(); + var vsGitServices = Substitute.For(); + vsGitServices.Clone(cloneUrl, clonePath, true).Returns(x => { throw new Exception(); }); + var cloneService = CreateRepositoryCloneService(operatingSystem: operatingSystem, vsGitServices: vsGitServices); Assert.ThrowsAsync(() => cloneService.CloneRepository(cloneUrl, clonePath)); @@ -137,10 +136,21 @@ public async Task CleansDirectoryOnCloneFailed(string cloneUrl, string clonePath await vsGitServices.Received().Clone(cloneUrl, clonePath, true); } - static RepositoryCloneService CreateRepositoryCloneService(IGitHubServiceProvider sp) + static RepositoryCloneService CreateRepositoryCloneService(IOperatingSystem operatingSystem = null, + IVSGitServices vsGitServices = null, IUsageTracker usageTracker = null, + ITeamExplorerServices teamExplorerServices = null, IGitHubServiceProvider serviceProvider = null) { - return new RepositoryCloneService(sp.GetOperatingSystem(), sp.GetVSGitServices(), sp.GetTeamExplorerServices(), - sp.GetGraphQLClientFactory(), sp.GetGitHubContextService(), sp.GetUsageTracker(), sp); + operatingSystem = operatingSystem ?? Substitute.For(); + vsGitServices = vsGitServices ?? Substitute.For(); + usageTracker = usageTracker ?? Substitute.For(); + teamExplorerServices = teamExplorerServices ?? Substitute.For(); + serviceProvider = serviceProvider ?? Substitute.For(); + + operatingSystem.Environment.ExpandEnvironmentVariables(Args.String).Returns(x => x[0]); + + return new RepositoryCloneService(operatingSystem, vsGitServices, teamExplorerServices, + Substitute.For(), Substitute.For(), + usageTracker, serviceProvider); } } } diff --git a/test/GitHub.App.UnitTests/Substitutes.cs b/test/GitHub.App.UnitTests/Substitutes.cs index a0a73802e7..a3b0255e74 100644 --- a/test/GitHub.App.UnitTests/Substitutes.cs +++ b/test/GitHub.App.UnitTests/Substitutes.cs @@ -30,33 +30,8 @@ public static T1 For(params object[] constructorArguments) }, constructorArguments); } - - // public static IGitRepositoriesExt IGitRepositoriesExt { get { return Substitute.For(); } } public static IGitService IGitService { get { return Substitute.For(); } } - public static IVSGitServices IVSGitServices - { - get - { - var ret = Substitute.For(); - ret.GetLocalClonePathFromGitProvider().Returns(@"c:\foo\bar"); - ret.Clone("https://github.com/failing/url", @"c:\dev\bar", true).Returns(x => { throw new Exception(); }); - return ret; - } - } - - public static IOperatingSystem OperatingSystem - { - get - { - var ret = Substitute.For(); - // 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(); } } public static IRepositoryCreationService RepositoryCreationService { get { return Substitute.For(); } } @@ -110,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(), + var clone = cloneService ?? new RepositoryCloneService(Substitute.For(), + Substitute.For(), Substitute.For(), Substitute.For(), Substitute.For(), Substitute.For(), ret); var create = creationService ?? new RepositoryCreationService(clone); @@ -124,8 +98,8 @@ public static IGitHubServiceProvider GetServiceProvider( ret.GetService(typeof(IGitHubContextService)).Returns(Substitute.For()); ret.GetService(typeof(IVSGitExt)).Returns(Substitute.For()); ret.GetService(typeof(IUsageTracker)).Returns(Substitute.For()); - ret.GetService(typeof(IVSGitServices)).Returns(vsgit); - ret.GetService(typeof(IOperatingSystem)).Returns(os); + ret.GetService(typeof(IVSGitServices)).Returns(Substitute.For()); + ret.GetService(typeof(IOperatingSystem)).Returns(Substitute.For()); ret.GetService(typeof(IRepositoryCloneService)).Returns(clone); ret.GetService(typeof(IRepositoryCreationService)).Returns(create); ret.GetService(typeof(IViewViewModelFactory)).Returns(ViewViewModelFactory);