From 0ff7d68dcb2e4cca5dc73d80d750ce211d15707d Mon Sep 17 00:00:00 2001 From: Andreia Gaita Date: Mon, 8 Aug 2016 20:36:03 +0200 Subject: [PATCH 1/6] PackageServiceProvider always needs to be set --- src/UnitTests/GitHub.App/Models/RepositoryModelTests.cs | 2 -- src/UnitTests/GitHub.Exports/SimpleRepositoryModelTests.cs | 1 - src/UnitTests/Substitutes.cs | 1 + 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/UnitTests/GitHub.App/Models/RepositoryModelTests.cs b/src/UnitTests/GitHub.App/Models/RepositoryModelTests.cs index 1a7ca4b142..d62cbe0c0a 100644 --- a/src/UnitTests/GitHub.App/Models/RepositoryModelTests.cs +++ b/src/UnitTests/GitHub.App/Models/RepositoryModelTests.cs @@ -60,7 +60,6 @@ public class PathConstructorTests : TempFileBaseClass public void NoRemoteUrl() { var provider = Substitutes.ServiceProvider; - Services.PackageServiceProvider = provider; var gitservice = provider.GetGitService(); var repo = Substitute.For(); var path = Directory.CreateSubdirectory("repo-name"); @@ -73,7 +72,6 @@ public void NoRemoteUrl() public void WithRemoteUrl() { var provider = Substitutes.ServiceProvider; - Services.PackageServiceProvider = provider; var gitservice = provider.GetGitService(); var repo = Substitute.For(); var path = Directory.CreateSubdirectory("repo-name"); diff --git a/src/UnitTests/GitHub.Exports/SimpleRepositoryModelTests.cs b/src/UnitTests/GitHub.Exports/SimpleRepositoryModelTests.cs index ba9a63c901..ae846db688 100644 --- a/src/UnitTests/GitHub.Exports/SimpleRepositoryModelTests.cs +++ b/src/UnitTests/GitHub.Exports/SimpleRepositoryModelTests.cs @@ -13,7 +13,6 @@ public class SimpleRepositoryModelTests : TempFileBaseClass void SetupRepository(string sha) { var provider = Substitutes.ServiceProvider; - Services.PackageServiceProvider = provider; var gitservice = provider.GetGitService(); var repo = Substitute.For(); gitservice.GetRepo(Args.String).Returns(repo); diff --git a/src/UnitTests/Substitutes.cs b/src/UnitTests/Substitutes.cs index e065200b14..c294db6588 100644 --- a/src/UnitTests/Substitutes.cs +++ b/src/UnitTests/Substitutes.cs @@ -106,6 +106,7 @@ public static IServiceProvider GetServiceProvider( cc.ComposeExportedValue(gitservice); ((IComponentModel)cm).DefaultExportProvider.Returns(cc); ret.GetService(typeof(SComponentModel)).Returns(cm); + Services.PackageServiceProvider = ret; var os = OperatingSystem; var vs = IVSServices; From 369f8088d4beb1360cbf30ba87b2f23bae72d6b7 Mon Sep 17 00:00:00 2001 From: Andreia Gaita Date: Mon, 8 Aug 2016 20:36:18 +0200 Subject: [PATCH 2/6] Don't run tests in parallel --- src/UnitTests/Properties/AssemblyInfo.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/UnitTests/Properties/AssemblyInfo.cs b/src/UnitTests/Properties/AssemblyInfo.cs index 59f209e0db..c1f8178d9d 100644 --- a/src/UnitTests/Properties/AssemblyInfo.cs +++ b/src/UnitTests/Properties/AssemblyInfo.cs @@ -1,6 +1,7 @@ using System.Reflection; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; +using Xunit; // General Information about an assembly is controlled through the following // set of attributes. Change these attribute values to modify the information @@ -34,3 +35,4 @@ // [assembly: AssemblyVersion("1.0.*")] [assembly: AssemblyVersion("1.0.0.0")] [assembly: AssemblyFileVersion("1.0.0.0")] +[assembly: CollectionBehavior(CollectionBehavior.CollectionPerAssembly, DisableTestParallelization = true)] \ No newline at end of file From 9c6eb80f8073b55935aedb49c5ea27d8698b988f Mon Sep 17 00:00:00 2001 From: Andreia Gaita Date: Mon, 8 Aug 2016 20:37:37 +0200 Subject: [PATCH 3/6] NullGuard asserts are useless when testing in debug mode --- src/GitHub.App/Services/PullRequestService.cs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/GitHub.App/Services/PullRequestService.cs b/src/GitHub.App/Services/PullRequestService.cs index 5c3180f187..3404940ac3 100644 --- a/src/GitHub.App/Services/PullRequestService.cs +++ b/src/GitHub.App/Services/PullRequestService.cs @@ -4,12 +4,20 @@ namespace GitHub.Services { + [NullGuard.NullGuard(NullGuard.ValidationFlags.None)] [Export(typeof(IPullRequestService))] [PartCreationPolicy(CreationPolicy.Shared)] public class PullRequestService : IPullRequestService { public IObservable CreatePullRequest(IRepositoryHost host, ISimpleRepositoryModel repository, string title, string body, IBranch source, IBranch target) { + Extensions.Guard.ArgumentNotNull(host, nameof(host)); + Extensions.Guard.ArgumentNotNull(repository, nameof(repository)); + Extensions.Guard.ArgumentNotNull(title, nameof(title)); + Extensions.Guard.ArgumentNotNull(body, nameof(body)); + Extensions.Guard.ArgumentNotNull(source, nameof(source)); + Extensions.Guard.ArgumentNotNull(target, nameof(target)); + return host.ModelService.CreatePullRequest(repository, title, body, source, target); } } From 4c6708121af6bdd53bb033d5ae4a9aae7dd5e8d1 Mon Sep 17 00:00:00 2001 From: Andreia Gaita Date: Mon, 8 Aug 2016 20:38:06 +0200 Subject: [PATCH 4/6] Add tests for PR creation service and view model --- .../Services/PullRequestServiceTests.cs | 48 +++++++++++++++++++ .../PullRequestCreationViewModelTests.cs | 36 ++++++++++++++ src/UnitTests/Substitutes.cs | 8 ++++ src/UnitTests/UnitTests.csproj | 2 + 4 files changed, 94 insertions(+) create mode 100644 src/UnitTests/GitHub.App/Services/PullRequestServiceTests.cs create mode 100644 src/UnitTests/GitHub.App/ViewModels/PullRequestCreationViewModelTests.cs diff --git a/src/UnitTests/GitHub.App/Services/PullRequestServiceTests.cs b/src/UnitTests/GitHub.App/Services/PullRequestServiceTests.cs new file mode 100644 index 0000000000..001cb28d95 --- /dev/null +++ b/src/UnitTests/GitHub.App/Services/PullRequestServiceTests.cs @@ -0,0 +1,48 @@ +using System.Reactive.Linq; +using System.Threading.Tasks; +using NSubstitute; +using Xunit; +using UnitTests; +using GitHub.Models; +using System; +using GitHub.Services; + +public class PullRequestServiceTests : TestBaseClass +{ + [Fact] + public async Task CreatePullRequestAllArgsMandatory() + { + var serviceProvider = Substitutes.ServiceProvider; + var service = new PullRequestService(); + + IRepositoryHost host = null; + ISimpleRepositoryModel repository = null; + string title = null; + string body = null; + IBranch source = null; + IBranch target = null; + + Assert.Throws(() => service.CreatePullRequest(host, repository, title, body, source, target)); + + host = serviceProvider.GetRepositoryHosts().GitHubHost; + Assert.Throws(() => service.CreatePullRequest(host, repository, title, body, source, target)); + + repository = new SimpleRepositoryModel("name", new GitHub.Primitives.UriString("http://github.com/github/stuff")); + Assert.Throws(() => service.CreatePullRequest(host, repository, title, body, source, target)); + + title = "a title"; + Assert.Throws(() => service.CreatePullRequest(host, repository, title, body, source, target)); + + body = "a body"; + Assert.Throws(() => service.CreatePullRequest(host, repository, title, body, source, target)); + + source = new BranchModel() { Name = "source" }; + Assert.Throws(() => service.CreatePullRequest(host, repository, title, body, source, target)); + + target = new BranchModel() { Name = "target" }; + var pr = await service.CreatePullRequest(host, repository, title, body, source, target); + + Assert.NotNull(pr); + } + +} diff --git a/src/UnitTests/GitHub.App/ViewModels/PullRequestCreationViewModelTests.cs b/src/UnitTests/GitHub.App/ViewModels/PullRequestCreationViewModelTests.cs new file mode 100644 index 0000000000..2e71bea45e --- /dev/null +++ b/src/UnitTests/GitHub.App/ViewModels/PullRequestCreationViewModelTests.cs @@ -0,0 +1,36 @@ +using System.Reactive.Linq; +using System.Threading.Tasks; +using NSubstitute; +using Xunit; +using UnitTests; +using GitHub.Models; +using System; +using GitHub.Services; +using GitHub.ViewModels; + +public class PullRequestCreationViewModelTests : TempFileBaseClass +{ + [Fact] + public async Task NullDescriptionBecomesEmptyBody() + { + var serviceProvider = Substitutes.ServiceProvider; + var service = new PullRequestService(); + var notifications = Substitute.For(); + + var host = serviceProvider.GetRepositoryHosts().GitHubHost; + var ms = Substitute.For(); + host.ModelService.Returns(ms); + + var repository = new SimpleRepositoryModel("name", new GitHub.Primitives.UriString("http://github.com/github/stuff")); + var title = "a title"; + + var vm = new PullRequestCreationViewModel(host, repository, service, notifications); + vm.SourceBranch = new BranchModel() { Name = "source" }; + vm.TargetBranch = new BranchModel() { Name = "target" }; + vm.PRTitle = title; + + await vm.CreatePullRequest.ExecuteAsync(); + ms.Received().CreatePullRequest(repository, vm.PRTitle, String.Empty, vm.SourceBranch, vm.TargetBranch); + } + +} diff --git a/src/UnitTests/Substitutes.cs b/src/UnitTests/Substitutes.cs index c294db6588..8117b4219e 100644 --- a/src/UnitTests/Substitutes.cs +++ b/src/UnitTests/Substitutes.cs @@ -2,6 +2,7 @@ using GitHub.Authentication; using GitHub.Models; using GitHub.Services; +using GitHub.VisualStudio; using Microsoft.VisualStudio.ComponentModelHost; using NSubstitute; using Rothko; @@ -65,6 +66,7 @@ public static IOperatingSystem OperatingSystem public static IConnectionManager ConnectionManager { get { return Substitute.For(); } } public static ITwoFactorChallengeHandler TwoFactorChallengeHandler { get { return Substitute.For(); } } public static IGistPublishService GistPublishService { get { return Substitute.For(); } } + public static IPullRequestService PullRequestService { get { return Substitute.For(); } } /// /// This returns a service provider with everything mocked except for @@ -127,6 +129,7 @@ public static IServiceProvider GetServiceProvider( ret.GetService(typeof(IAvatarProvider)).Returns(avatarProvider); ret.GetService(typeof(ITwoFactorChallengeHandler)).Returns(TwoFactorChallengeHandler); ret.GetService(typeof(IGistPublishService)).Returns(GistPublishService); + ret.GetService(typeof(IPullRequestService)).Returns(PullRequestService); return ret; } @@ -198,5 +201,10 @@ public static IGistPublishService GetGistPublishService(this IServiceProvider pr { return provider.GetService(typeof(IGistPublishService)) as IGistPublishService; } + + public static IPullRequestService GetPullRequestsService(this IServiceProvider provider) + { + return provider.GetService(typeof(IPullRequestService)) as IPullRequestService; + } } } diff --git a/src/UnitTests/UnitTests.csproj b/src/UnitTests/UnitTests.csproj index 68cc29803a..2441b2c2a0 100644 --- a/src/UnitTests/UnitTests.csproj +++ b/src/UnitTests/UnitTests.csproj @@ -166,11 +166,13 @@ + + From a59dbeb4bdca892b215dd162c3a14fd57b7ab590 Mon Sep 17 00:00:00 2001 From: Andreia Gaita Date: Mon, 8 Aug 2016 20:38:33 +0200 Subject: [PATCH 5/6] Fix crasher, add logging, don't ignore exceptions --- .../PullRequestCreationViewModel.cs | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/src/GitHub.App/ViewModels/PullRequestCreationViewModel.cs b/src/GitHub.App/ViewModels/PullRequestCreationViewModel.cs index fbf5ff17b2..b9f14b3635 100644 --- a/src/GitHub.App/ViewModels/PullRequestCreationViewModel.cs +++ b/src/GitHub.App/ViewModels/PullRequestCreationViewModel.cs @@ -17,6 +17,7 @@ using System.Reactive; using System.Diagnostics.CodeAnalysis; using Octokit; +using NLog; namespace GitHub.ViewModels { @@ -25,6 +26,8 @@ namespace GitHub.ViewModels [SuppressMessage("Microsoft.Design", "CA1001:TypesThatOwnDisposableFieldsShouldBeDisposable")] public class PullRequestCreationViewModel : BaseViewModel, IPullRequestCreationViewModel { + static readonly Logger log = LogManager.GetCurrentClassLogger(); + readonly IRepositoryHost repositoryHost; readonly ISimpleRepositoryModel activeRepo; readonly Subject initializationComplete = new Subject(); @@ -36,7 +39,7 @@ public class PullRequestCreationViewModel : BaseViewModel, IPullRequestCreationV IPullRequestService service, INotificationService notifications) : this(connectionRepositoryHostMap.CurrentRepositoryHost, teservice.ActiveRepo, service, notifications) {} - + public PullRequestCreationViewModel(IRepositoryHost repositoryHost, ISimpleRepositoryModel activeRepo, IPullRequestService service, INotificationService notifications) { @@ -78,18 +81,18 @@ public PullRequestCreationViewModel(IRepositoryHost repositoryHost, ISimpleRepos .Subscribe(x => notifications.ShowError(BranchValidator.ValidationResult.Message)); createPullRequest = ReactiveCommand.CreateAsyncObservable(whenAnyValidationResultChanges, - _ => service.CreatePullRequest(repositoryHost, activeRepo, PRTitle, Description, SourceBranch, TargetBranch) - ); - createPullRequest.ThrownExceptions.Subscribe(ex => - { - if (!ex.IsCriticalException()) - { - //TODO:Will need a uniform solution to HTTP exception message handling - var apiException = ex as ApiValidationException; - var error = apiException?.ApiError?.Errors?.FirstOrDefault(); - notifications.ShowError(error?.Message ?? ex.Message); - } - }); + _ => service + .CreatePullRequest(repositoryHost, activeRepo, PRTitle, Description ?? String.Empty, SourceBranch, TargetBranch) + .Catch(ex => + { + log.Error(ex); + + //TODO:Will need a uniform solution to HTTP exception message handling + var apiException = ex as ApiValidationException; + var error = apiException?.ApiError?.Errors?.FirstOrDefault(); + notifications.ShowError(error?.Message ?? ex.Message); + return Observable.Empty(); + })); } public override void Initialize([AllowNull] ViewWithData data) From bdd9b97f31cdd24b8e99b09bd74d436fa02832b3 Mon Sep 17 00:00:00 2001 From: Andreia Gaita Date: Tue, 9 Aug 2016 13:20:33 +0200 Subject: [PATCH 6/6] Fix code warning in test --- .../GitHub.App/ViewModels/PullRequestCreationViewModelTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/UnitTests/GitHub.App/ViewModels/PullRequestCreationViewModelTests.cs b/src/UnitTests/GitHub.App/ViewModels/PullRequestCreationViewModelTests.cs index 2e71bea45e..5743439489 100644 --- a/src/UnitTests/GitHub.App/ViewModels/PullRequestCreationViewModelTests.cs +++ b/src/UnitTests/GitHub.App/ViewModels/PullRequestCreationViewModelTests.cs @@ -30,7 +30,7 @@ public async Task NullDescriptionBecomesEmptyBody() vm.PRTitle = title; await vm.CreatePullRequest.ExecuteAsync(); - ms.Received().CreatePullRequest(repository, vm.PRTitle, String.Empty, vm.SourceBranch, vm.TargetBranch); + var unused = ms.Received().CreatePullRequest(repository, vm.PRTitle, String.Empty, vm.SourceBranch, vm.TargetBranch); } }