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); } } 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) 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.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..5743439489 --- /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(); + var unused = ms.Received().CreatePullRequest(repository, vm.PRTitle, String.Empty, vm.SourceBranch, vm.TargetBranch); + } + +} 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/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 diff --git a/src/UnitTests/Substitutes.cs b/src/UnitTests/Substitutes.cs index e065200b14..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 @@ -106,6 +108,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; @@ -126,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; } @@ -197,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 @@ + +