From b834336e2057d35bee6029077e8186fa2385222e Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 16 Jan 2018 17:28:40 +0000 Subject: [PATCH 01/64] Add TeamExplorerContext MEF service This service is used to access IGitExt from Visual Studio 2015 and 2017. --- src/GitHub.App/GitHub.App.csproj | 1 + .../Services/TeamExplorerContext.cs | 75 +++++++++++++++++++ src/GitHub.Exports/GitHub.Exports.csproj | 1 + .../Services/ITeamExplorerContext.cs | 12 +++ 4 files changed, 89 insertions(+) create mode 100644 src/GitHub.App/Services/TeamExplorerContext.cs create mode 100644 src/GitHub.Exports/Services/ITeamExplorerContext.cs diff --git a/src/GitHub.App/GitHub.App.csproj b/src/GitHub.App/GitHub.App.csproj index f726e09103..e359cd6022 100644 --- a/src/GitHub.App/GitHub.App.csproj +++ b/src/GitHub.App/GitHub.App.csproj @@ -138,6 +138,7 @@ + diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs new file mode 100644 index 0000000000..7ed1c7c1b2 --- /dev/null +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -0,0 +1,75 @@ +using System; +using System.Linq; +using System.ComponentModel; +using System.ComponentModel.Composition; +using GitHub.Models; +using System.Collections.Generic; +using Microsoft.VisualStudio.Shell; + +namespace GitHub.Services +{ + /// + /// This service uses reflection to access the IGitExt from Visual Studio 2015 and 2017. + /// + [Export(typeof(ITeamExplorerContext))] + [PartCreationPolicy(CreationPolicy.Shared)] + public class TeamExplorerContext : ITeamExplorerContext + { + [ImportingConstructor] + public TeamExplorerContext([Import(typeof(SVsServiceProvider))] IServiceProvider serviceProvider) + { + var gitExtType = Type.GetType("Microsoft.VisualStudio.TeamFoundation.Git.Extensibility.IGitExt, Microsoft.TeamFoundation.Git.Provider", false); + if (gitExtType != null) + { + var gitExt = serviceProvider.GetService(gitExtType); + Refresh(gitExt); + + var notifyPropertyChanged = (INotifyPropertyChanged)gitExt; + notifyPropertyChanged.PropertyChanged += (s, e) => Refresh(gitExt); + } + } + + void Refresh(object gitExt) + { + var newPath = FindActiveRepositoryPath(gitExt); + var oldPath = ActiveRepository?.LocalPath; + if (newPath != oldPath) + { + ActiveRepository = new LocalRepositoryModel(newPath); + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(ActiveRepository))); + } + else + { + StatusChanged?.Invoke(this, EventArgs.Empty); + } + } + + static string FindActiveRepositoryPath(object gitExt) + { + var activeRepositoriesProperty = gitExt.GetType().GetProperty("ActiveRepositories"); + var activeRepositories = (IEnumerable)activeRepositoriesProperty.GetValue(gitExt); + if (activeRepositories == null) + { + return null; + } + + var repo = activeRepositories.FirstOrDefault(); + if (repo == null) + { + return null; + } + + var repositoryPathProperty = repo.GetType().GetProperty("RepositoryPath"); + var repositoryPath = (string)repositoryPathProperty.GetValue(repo); + return repositoryPath; + } + + public ILocalRepositoryModel ActiveRepository + { + get; private set; + } + + public event PropertyChangedEventHandler PropertyChanged; + public event EventHandler StatusChanged; + } +} diff --git a/src/GitHub.Exports/GitHub.Exports.csproj b/src/GitHub.Exports/GitHub.Exports.csproj index dc0c9d224c..4bf3018444 100644 --- a/src/GitHub.Exports/GitHub.Exports.csproj +++ b/src/GitHub.Exports/GitHub.Exports.csproj @@ -154,6 +154,7 @@ + diff --git a/src/GitHub.Exports/Services/ITeamExplorerContext.cs b/src/GitHub.Exports/Services/ITeamExplorerContext.cs new file mode 100644 index 0000000000..7429997e81 --- /dev/null +++ b/src/GitHub.Exports/Services/ITeamExplorerContext.cs @@ -0,0 +1,12 @@ +using System; +using System.ComponentModel; +using GitHub.Models; + +namespace GitHub.Services +{ + public interface ITeamExplorerContext : INotifyPropertyChanged + { + ILocalRepositoryModel ActiveRepository { get; } + event EventHandler StatusChanged; + } +} From 4e9d1ec339229a6d694967ec67bbc552ae68d6f5 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 17 Jan 2018 10:34:44 +0000 Subject: [PATCH 02/64] Harden reflection code and add logging --- .../Services/TeamExplorerContext.cs | 38 ++++++++++++++----- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index 7ed1c7c1b2..836898d899 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -2,9 +2,11 @@ using System.Linq; using System.ComponentModel; using System.ComponentModel.Composition; -using GitHub.Models; using System.Collections.Generic; using Microsoft.VisualStudio.Shell; +using GitHub.Models; +using GitHub.Logging; +using Serilog; namespace GitHub.Services { @@ -15,18 +17,36 @@ namespace GitHub.Services [PartCreationPolicy(CreationPolicy.Shared)] public class TeamExplorerContext : ITeamExplorerContext { + const string GitExtTypeName = "Microsoft.VisualStudio.TeamFoundation.Git.Extensibility.IGitExt, Microsoft.TeamFoundation.Git.Provider"; + static readonly ILogger log = LogManager.ForContext(); + [ImportingConstructor] public TeamExplorerContext([Import(typeof(SVsServiceProvider))] IServiceProvider serviceProvider) { - var gitExtType = Type.GetType("Microsoft.VisualStudio.TeamFoundation.Git.Extensibility.IGitExt, Microsoft.TeamFoundation.Git.Provider", false); - if (gitExtType != null) + var gitExtType = Type.GetType(GitExtTypeName, false); + if (gitExtType == null) + { + log.Error("Couldn't find type {GitExtTypeName}", GitExtTypeName); + return; + } + + var gitExt = serviceProvider.GetService(gitExtType); + if (gitExt == null) { - var gitExt = serviceProvider.GetService(gitExtType); - Refresh(gitExt); + log.Error("Couldn't find service for type {GitExtType}", gitExtType); + return; + } + + Refresh(gitExt); - var notifyPropertyChanged = (INotifyPropertyChanged)gitExt; - notifyPropertyChanged.PropertyChanged += (s, e) => Refresh(gitExt); + var notifyPropertyChanged = gitExt as INotifyPropertyChanged; + if (notifyPropertyChanged == null) + { + log.Error("The service {ServiceObject} doesn't implement {Interface}", gitExt, typeof(INotifyPropertyChanged)); + return; } + + notifyPropertyChanged.PropertyChanged += (s, e) => Refresh(gitExt); } void Refresh(object gitExt) @@ -47,7 +67,7 @@ void Refresh(object gitExt) static string FindActiveRepositoryPath(object gitExt) { var activeRepositoriesProperty = gitExt.GetType().GetProperty("ActiveRepositories"); - var activeRepositories = (IEnumerable)activeRepositoriesProperty.GetValue(gitExt); + var activeRepositories = (IEnumerable)activeRepositoriesProperty?.GetValue(gitExt); if (activeRepositories == null) { return null; @@ -60,7 +80,7 @@ static string FindActiveRepositoryPath(object gitExt) } var repositoryPathProperty = repo.GetType().GetProperty("RepositoryPath"); - var repositoryPath = (string)repositoryPathProperty.GetValue(repo); + var repositoryPath = (string)repositoryPathProperty?.GetValue(repo); return repositoryPath; } From e374c36a4e6afa5499daa3fd4bc098c0e86aec5b Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 17 Jan 2018 11:26:28 +0000 Subject: [PATCH 03/64] Only fire StatusChanged on BranchName/HeadSha change --- .../Services/TeamExplorerContext.cs | 59 ++++++++++++++----- .../Services/PullRequestSessionManager.cs | 3 +- 2 files changed, 46 insertions(+), 16 deletions(-) diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index 836898d899..35af07a50b 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -20,6 +20,10 @@ public class TeamExplorerContext : ITeamExplorerContext const string GitExtTypeName = "Microsoft.VisualStudio.TeamFoundation.Git.Extensibility.IGitExt, Microsoft.TeamFoundation.Git.Provider"; static readonly ILogger log = LogManager.ForContext(); + string repositoryPath; + string branchName; + string headSha; + [ImportingConstructor] public TeamExplorerContext([Import(typeof(SVsServiceProvider))] IServiceProvider serviceProvider) { @@ -51,37 +55,62 @@ public TeamExplorerContext([Import(typeof(SVsServiceProvider))] IServiceProvider void Refresh(object gitExt) { - var newPath = FindActiveRepositoryPath(gitExt); - var oldPath = ActiveRepository?.LocalPath; - if (newPath != oldPath) + string newRepositoryPath; + string newBranchName; + string newHeadSha; + FindActiveRepository(gitExt, out newRepositoryPath, out newBranchName, out newHeadSha); + + log.Information("Refresh ActiveRepository: RepositoryPath={RepositoryPath}, BranchName={BranchName}, HeadSha={HeadSha}", + newRepositoryPath, newBranchName, newHeadSha); + + if (newRepositoryPath != repositoryPath) { - ActiveRepository = new LocalRepositoryModel(newPath); + log.Information("Fire PropertyChanged event for ActiveRepository"); + + repositoryPath = newRepositoryPath; + branchName = newBranchName; + headSha = newHeadSha; + + ActiveRepository = repositoryPath != null ? new LocalRepositoryModel(repositoryPath) : null; PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(ActiveRepository))); } - else + else if (newBranchName != branchName || newHeadSha != headSha) { + log.Information("Fire StatusChanged event for ActiveRepository"); + + branchName = newBranchName; + headSha = newHeadSha; + StatusChanged?.Invoke(this, EventArgs.Empty); } } - static string FindActiveRepositoryPath(object gitExt) + static bool FindActiveRepository(object gitExt, out string repositoryPath, out string branchName, out string headSha) { var activeRepositoriesProperty = gitExt.GetType().GetProperty("ActiveRepositories"); var activeRepositories = (IEnumerable)activeRepositoriesProperty?.GetValue(gitExt); - if (activeRepositories == null) - { - return null; - } - - var repo = activeRepositories.FirstOrDefault(); + var repo = activeRepositories?.FirstOrDefault(); if (repo == null) { - return null; + repositoryPath = null; + branchName = null; + headSha = null; + return false; } var repositoryPathProperty = repo.GetType().GetProperty("RepositoryPath"); - var repositoryPath = (string)repositoryPathProperty?.GetValue(repo); - return repositoryPath; + repositoryPath = (string)repositoryPathProperty?.GetValue(repo); + + var currentBranchProperty = repo.GetType().GetProperty("CurrentBranch"); + var currentBranch = currentBranchProperty?.GetValue(repo); + + var headShaProperty = currentBranch?.GetType().GetProperty("HeadSha"); + headSha = (string)headShaProperty?.GetValue(currentBranch); + + var nameProperty = currentBranch?.GetType().GetProperty("Name"); + branchName = (string)nameProperty?.GetValue(currentBranch); + + return true; } public ILocalRepositoryModel ActiveRepository diff --git a/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs b/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs index 106c67d6cf..5fe1dcdc30 100644 --- a/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs +++ b/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs @@ -53,7 +53,8 @@ public PullRequestSessionManager( IPullRequestSessionService sessionService, IConnectionManager connectionManager, IModelServiceFactory modelServiceFactory, - ITeamExplorerServiceHolder teamExplorerService) + ITeamExplorerServiceHolder teamExplorerService, + ITeamExplorerContext teamExplorerContext) { Guard.ArgumentNotNull(service, nameof(service)); Guard.ArgumentNotNull(sessionService, nameof(sessionService)); From 416ec354ba6c827da0f635cd173f21dced47e5c1 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 17 Jan 2018 14:51:38 +0000 Subject: [PATCH 04/64] Ignore when ActiveRepositories is empty --- src/GitHub.App/Services/TeamExplorerContext.cs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index 35af07a50b..6d2b6e894c 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -63,7 +63,14 @@ void Refresh(object gitExt) log.Information("Refresh ActiveRepository: RepositoryPath={RepositoryPath}, BranchName={BranchName}, HeadSha={HeadSha}", newRepositoryPath, newBranchName, newHeadSha); - if (newRepositoryPath != repositoryPath) + if (newRepositoryPath == null) + { + // Ignore when ActiveRepositories is empty. + // The ActiveRepository can be changed but not unloaded. + // https://github.com/github/VisualStudio/issues/1421 + log.Information("Ignoring null ActiveRepository"); + } + else if (newRepositoryPath != repositoryPath) { log.Information("Fire PropertyChanged event for ActiveRepository"); @@ -85,7 +92,7 @@ void Refresh(object gitExt) } } - static bool FindActiveRepository(object gitExt, out string repositoryPath, out string branchName, out string headSha) + static void FindActiveRepository(object gitExt, out string repositoryPath, out string branchName, out string headSha) { var activeRepositoriesProperty = gitExt.GetType().GetProperty("ActiveRepositories"); var activeRepositories = (IEnumerable)activeRepositoriesProperty?.GetValue(gitExt); @@ -95,7 +102,7 @@ static bool FindActiveRepository(object gitExt, out string repositoryPath, out s repositoryPath = null; branchName = null; headSha = null; - return false; + return; } var repositoryPathProperty = repo.GetType().GetProperty("RepositoryPath"); @@ -109,8 +116,6 @@ static bool FindActiveRepository(object gitExt, out string repositoryPath, out s var nameProperty = currentBranch?.GetType().GetProperty("Name"); branchName = (string)nameProperty?.GetValue(currentBranch); - - return true; } public ILocalRepositoryModel ActiveRepository From 768eca18152e05b56b3e0f38ade95e5f14ee8d4b Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 17 Jan 2018 16:18:55 +0000 Subject: [PATCH 05/64] Don't throw if CloneUrl is null --- src/GitHub.App/Services/PullRequestService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/GitHub.App/Services/PullRequestService.cs b/src/GitHub.App/Services/PullRequestService.cs index b20f65b759..bb611fa11e 100644 --- a/src/GitHub.App/Services/PullRequestService.cs +++ b/src/GitHub.App/Services/PullRequestService.cs @@ -287,7 +287,7 @@ public bool IsPullRequestFromRepository(ILocalRepositoryModel repository, IPullR { if (pullRequest.Head?.RepositoryCloneUrl != null) { - return repository.CloneUrl.ToRepositoryUrl() == pullRequest.Head.RepositoryCloneUrl.ToRepositoryUrl(); + return repository.CloneUrl?.ToRepositoryUrl() == pullRequest.Head.RepositoryCloneUrl.ToRepositoryUrl(); } return false; From 00051bbf70547b77499cd4665866577994091d0c Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 17 Jan 2018 16:24:03 +0000 Subject: [PATCH 06/64] Change PullRequestSessionManager to use ITeamExplorerContext --- .../Services/PullRequestSessionManager.cs | 17 ++- .../GitHub.InlineReviews.UnitTests.csproj | 1 - .../PullRequestSessionManagerTests.cs | 113 +++++++++++------- .../FakeTeamExplorerServiceHolder.cs | 54 --------- 4 files changed, 86 insertions(+), 99 deletions(-) delete mode 100644 test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeTeamExplorerServiceHolder.cs diff --git a/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs b/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs index 5fe1dcdc30..3bb43a7afd 100644 --- a/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs +++ b/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs @@ -53,20 +53,31 @@ public PullRequestSessionManager( IPullRequestSessionService sessionService, IConnectionManager connectionManager, IModelServiceFactory modelServiceFactory, - ITeamExplorerServiceHolder teamExplorerService, ITeamExplorerContext teamExplorerContext) { Guard.ArgumentNotNull(service, nameof(service)); Guard.ArgumentNotNull(sessionService, nameof(sessionService)); Guard.ArgumentNotNull(connectionManager, nameof(connectionManager)); Guard.ArgumentNotNull(modelServiceFactory, nameof(modelServiceFactory)); - Guard.ArgumentNotNull(teamExplorerService, nameof(teamExplorerService)); + Guard.ArgumentNotNull(teamExplorerContext, nameof(teamExplorerContext)); this.service = service; this.sessionService = sessionService; this.connectionManager = connectionManager; this.modelServiceFactory = modelServiceFactory; - teamExplorerService.Subscribe(this, x => RepoChanged(x).Forget()); + + RepoChanged(teamExplorerContext.ActiveRepository).Forget(); + teamExplorerContext.StatusChanged += (s, e) => + { + RepoChanged(teamExplorerContext.ActiveRepository).Forget(); + }; + teamExplorerContext.PropertyChanged += (s, e) => + { + if (e.PropertyName == nameof(teamExplorerContext.ActiveRepository)) + { + RepoChanged(teamExplorerContext.ActiveRepository).Forget(); + } + }; } /// diff --git a/test/GitHub.InlineReviews.UnitTests/GitHub.InlineReviews.UnitTests.csproj b/test/GitHub.InlineReviews.UnitTests/GitHub.InlineReviews.UnitTests.csproj index 1cc2c42efb..471f99879e 100644 --- a/test/GitHub.InlineReviews.UnitTests/GitHub.InlineReviews.UnitTests.csproj +++ b/test/GitHub.InlineReviews.UnitTests/GitHub.InlineReviews.UnitTests.csproj @@ -143,7 +143,6 @@ - diff --git a/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionManagerTests.cs b/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionManagerTests.cs index 2a38ff7806..e31e4acae5 100644 --- a/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionManagerTests.cs +++ b/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionManagerTests.cs @@ -5,7 +5,6 @@ using System.Reactive.Subjects; using System.Text; using System.Threading.Tasks; -using GitHub.Extensions; using GitHub.Factories; using GitHub.InlineReviews.Models; using GitHub.InlineReviews.Services; @@ -50,11 +49,30 @@ public void ReadsPullRequestFromCorrectFork() Substitute.For(), connectionManager, modelFactory, - new FakeTeamExplorerServiceHolder(repositoryModel)); + CreateTeamExplorerContext(repositoryModel)); var modelService = modelFactory.CreateBlocking(connectionManager.Connections[0]); modelService.Received(1).GetPullRequest("fork", "repo", 15); } + + [Fact] + public void LocalRepositoryModelNull() + { + var repositoryModel = null as LocalRepositoryModel; + var service = CreatePullRequestService(); + var connectionManager = CreateConnectionManager(); + var modelFactory = CreateModelServiceFactory(); + var teamExplorerContext = CreateTeamExplorerContext(repositoryModel); + + var target = new PullRequestSessionManager( + service, + Substitute.For(), + connectionManager, + modelFactory, + teamExplorerContext); + + Assert.Null(target.CurrentSession); + } } public class TheCurrentSessionProperty : PullRequestSessionManagerTests @@ -67,7 +85,7 @@ public void CreatesSessionForCurrentBranch() Substitute.For(), CreateConnectionManager(), CreateModelServiceFactory(), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); Assert.NotNull(target.CurrentSession); Assert.True(target.CurrentSession.IsCheckedOut); @@ -84,7 +102,7 @@ public void CurrentSessionIsNullIfNoPullRequestForCurrentBranch() Substitute.For(), CreateConnectionManager(), CreateModelServiceFactory(), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); Assert.Null(target.CurrentSession); } @@ -93,18 +111,18 @@ public void CurrentSessionIsNullIfNoPullRequestForCurrentBranch() public void CurrentSessionChangesWhenBranchChanges() { var service = CreatePullRequestService(); - var teService = new FakeTeamExplorerServiceHolder(CreateRepositoryModel()); + var teamExplorerContext = CreateTeamExplorerContext(CreateRepositoryModel()); var target = new PullRequestSessionManager( service, Substitute.For(), CreateConnectionManager(), CreateModelServiceFactory(), - teService); + teamExplorerContext); var session = target.CurrentSession; service.GetPullRequestForCurrentBranch(null).ReturnsForAnyArgs(Observable.Return(Tuple.Create("foo", 22))); - teService.NotifyActiveRepoChanged(); + teamExplorerContext.StatusChanged += Raise.Event(); Assert.NotSame(session, target.CurrentSession); } @@ -112,17 +130,17 @@ public void CurrentSessionChangesWhenBranchChanges() [Fact] public void CurrentSessionChangesWhenRepoChanged() { - var teService = new FakeTeamExplorerServiceHolder(CreateRepositoryModel()); + var teamExplorerContext = CreateTeamExplorerContext(CreateRepositoryModel()); var target = new PullRequestSessionManager( CreatePullRequestService(), Substitute.For(), CreateConnectionManager(), CreateModelServiceFactory(), - teService); + teamExplorerContext); var session = target.CurrentSession; - teService.ActiveRepo = CreateRepositoryModel("https://github.com/owner/other"); + SetActiveRepository(teamExplorerContext, CreateRepositoryModel("https://github.com/owner/other")); Assert.NotSame(session, target.CurrentSession); } @@ -130,17 +148,17 @@ public void CurrentSessionChangesWhenRepoChanged() [Fact] public void RepoChangedDoesntCreateNewSessionIfNotNecessary() { - var teService = new FakeTeamExplorerServiceHolder(CreateRepositoryModel()); + var teamExplorerContext = CreateTeamExplorerContext(CreateRepositoryModel()); var target = new PullRequestSessionManager( CreatePullRequestService(), Substitute.For(), CreateConnectionManager(), CreateModelServiceFactory(), - teService); + teamExplorerContext); var session = target.CurrentSession; - teService.NotifyActiveRepoChanged(); + teamExplorerContext.StatusChanged += Raise.Event(); Assert.Same(session, target.CurrentSession); } @@ -148,15 +166,15 @@ public void RepoChangedDoesntCreateNewSessionIfNotNecessary() [Fact] public void RepoChangedHandlesNullRepository() { - var teService = new FakeTeamExplorerServiceHolder(CreateRepositoryModel()); + var teamExplorerContext = CreateTeamExplorerContext(CreateRepositoryModel()); var target = new PullRequestSessionManager( CreatePullRequestService(), Substitute.For(), CreateConnectionManager(), CreateModelServiceFactory(), - teService); + teamExplorerContext); - teService.ActiveRepo = null; + SetActiveRepository(teamExplorerContext, null); Assert.Null(target.CurrentSession); } @@ -169,7 +187,7 @@ public void CreatesSessionWithCorrectRepositoryOwner() Substitute.For(), CreateConnectionManager(), CreateModelServiceFactory(), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); Assert.Equal("this-owner", target.CurrentSession.RepositoryOwner); } @@ -189,7 +207,7 @@ public async Task BaseShaIsSet() CreateSessionService(), CreateConnectionManager(), CreateModelServiceFactory(), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); var file = await target.GetLiveFile(FilePath, textView, textView.TextBuffer); Assert.Same("BASESHA", file.BaseSha); @@ -205,7 +223,7 @@ public async Task CommitShaIsSet() CreateSessionService(), CreateConnectionManager(), CreateModelServiceFactory(), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); var file = await target.GetLiveFile(FilePath, textView, textView.TextBuffer); Assert.Same("TIPSHA", file.CommitSha); @@ -221,7 +239,7 @@ public async Task CommitShaIsNullIfModified() CreateSessionService(true), CreateConnectionManager(), CreateModelServiceFactory(), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); var file = await target.GetLiveFile(FilePath, textView, textView.TextBuffer); Assert.Null(file.CommitSha); @@ -249,7 +267,7 @@ public async Task DiffIsSet() sessionService, CreateConnectionManager(), CreateModelServiceFactory(), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); var file = await target.GetLiveFile(FilePath, textView, textView.TextBuffer); Assert.Same(diff, file.Diff); @@ -267,7 +285,7 @@ public async Task InlineCommentThreadsIsSet() sessionService, CreateConnectionManager(), CreateModelServiceFactory(), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); sessionService.BuildCommentThreads( target.CurrentSession.PullRequest, @@ -296,7 +314,7 @@ public async Task CreatesTrackingPointsForThreads() sessionService, CreateConnectionManager(), CreateModelServiceFactory(), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); sessionService.BuildCommentThreads( target.CurrentSession.PullRequest, @@ -315,14 +333,14 @@ public async Task MovingToNoRepositoryShouldNullOutProperties() var textView = CreateTextView(); var sessionService = CreateSessionService(); var threads = new List(); - var teHolder = new FakeTeamExplorerServiceHolder(CreateRepositoryModel()); + var teamExplorerContext = CreateTeamExplorerContext(CreateRepositoryModel()); var target = new PullRequestSessionManager( CreatePullRequestService(), CreateSessionService(), CreateConnectionManager(), CreateModelServiceFactory(), - teHolder); + teamExplorerContext); sessionService.BuildCommentThreads( target.CurrentSession.PullRequest, @@ -338,7 +356,7 @@ public async Task MovingToNoRepositoryShouldNullOutProperties() Assert.NotNull(file.InlineCommentThreads); Assert.NotNull(file.TrackingPoints); - teHolder.ActiveRepo = null; + SetActiveRepository(teamExplorerContext, null); Assert.Null(file.BaseSha); Assert.Null(file.CommitSha); @@ -366,7 +384,7 @@ public async Task ModifyingBufferMarksThreadsAsStaleAndSignalsRebuild() sessionService, CreateConnectionManager(), CreateModelServiceFactory(), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); sessionService.BuildCommentThreads( target.CurrentSession.PullRequest, @@ -409,7 +427,7 @@ public async Task RebuildSignalUpdatesCommitSha() sessionService, CreateConnectionManager(), CreateModelServiceFactory(), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); var file = (PullRequestSessionLiveFile)await target.GetLiveFile(FilePath, textView, textView.TextBuffer); Assert.Same("TIPSHA", file.CommitSha); @@ -430,7 +448,7 @@ public async Task ClosingTextViewDisposesFile() CreateSessionService(), CreateConnectionManager(), CreateModelServiceFactory(), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); var file = (PullRequestSessionLiveFile)await target.GetLiveFile(FilePath, textView, textView.TextBuffer); var compositeDisposable = file.ToDispose as CompositeDisposable; @@ -474,7 +492,7 @@ Line 2 CreateRealSessionService(diff: diffService), CreateConnectionManager(), CreateModelServiceFactory(pullRequest), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); var file = (PullRequestSessionLiveFile)await target.GetLiveFile(FilePath, textView, textView.TextBuffer); Assert.Equal(1, file.InlineCommentThreads.Count); @@ -520,7 +538,7 @@ Line 2 CreateRealSessionService(diff: diffService), CreateConnectionManager(), CreateModelServiceFactory(pullRequest), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); var file = (PullRequestSessionLiveFile)await target.GetLiveFile(FilePath, textView, textView.TextBuffer); Assert.Equal(1, file.InlineCommentThreads.Count); @@ -580,7 +598,7 @@ Line 2 CreateRealSessionService(diff: diffService), CreateConnectionManager(), CreateModelServiceFactory(pullRequest), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); var file = (PullRequestSessionLiveFile)await target.GetLiveFile(FilePath, textView, textView.TextBuffer); Assert.Equal("Original Comment", file.InlineCommentThreads[0].Comments[0].Body); @@ -635,7 +653,7 @@ Line 2 CreateRealSessionService(diff: diffService), CreateConnectionManager(), CreateModelServiceFactory(pullRequest), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); var file = (PullRequestSessionLiveFile)await target.GetLiveFile(FilePath, textView, textView.TextBuffer); Assert.Equal(1, file.InlineCommentThreads[0].Comments.Count); @@ -666,7 +684,7 @@ public async Task CommitShaIsUpdatedOnTextChange() sessionService, CreateConnectionManager(), CreateModelServiceFactory(), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); var file = await target.GetLiveFile(FilePath, textView, textView.TextBuffer); Assert.Equal("TIPSHA", file.CommitSha); @@ -695,7 +713,7 @@ public async Task UpdatingCurrentSessionPullRequestTriggersLinesChanged() sessionService, CreateConnectionManager(), CreateModelServiceFactory(), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); var file = await target.GetLiveFile(FilePath, textView, textView.TextBuffer); var raised = false; var pullRequest = target.CurrentSession.PullRequest; @@ -807,7 +825,7 @@ public async Task GetSessionReturnsAndUpdatesCurrentSessionIfNumbersMatch() Substitute.For(), CreateConnectionManager(), CreateModelServiceFactory(), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); var newModel = CreatePullRequestModel(CurrentBranchPullRequestNumber); var result = await target.GetSession(newModel); @@ -824,7 +842,7 @@ public async Task GetSessionReturnsNewSessionForPullRequestWithDifferentNumber() Substitute.For(), CreateConnectionManager(), CreateModelServiceFactory(), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); var newModel = CreatePullRequestModel(NotCurrentBranchPullRequestNumber); var result = await target.GetSession(newModel); @@ -842,7 +860,7 @@ public async Task GetSessionReturnsNewSessionForPullRequestWithDifferentBaseOwne Substitute.For(), CreateConnectionManager(), CreateModelServiceFactory(), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); var newModel = CreatePullRequestModel(CurrentBranchPullRequestNumber, "https://github.com/fork/repo"); var result = await target.GetSession(newModel); @@ -860,7 +878,7 @@ public async Task GetSessionReturnsSameSessionEachTime() Substitute.For(), CreateConnectionManager(), CreateModelServiceFactory(), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); var newModel = CreatePullRequestModel(NotCurrentBranchPullRequestNumber); var result1 = await target.GetSession(newModel); @@ -879,7 +897,7 @@ public async Task SessionCanBeCollected() Substitute.For(), CreateConnectionManager(), CreateModelServiceFactory(), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); Func run = async () => { @@ -912,7 +930,7 @@ public async Task GetSessionUpdatesCurrentSessionIfCurrentBranchIsPullRequestBut Substitute.For(), CreateConnectionManager(), CreateModelServiceFactory(), - new FakeTeamExplorerServiceHolder(CreateRepositoryModel())); + CreateTeamExplorerContext(CreateRepositoryModel())); Assert.Null(target.CurrentSession); @@ -983,5 +1001,18 @@ ILocalRepositoryModel CreateRepositoryModel(string cloneUrl = OwnerCloneUrl) result.Owner.Returns(uriString.Owner); return result; } + + static ITeamExplorerContext CreateTeamExplorerContext(ILocalRepositoryModel repo) + { + var teamExplorerContext = Substitute.For(); + teamExplorerContext.ActiveRepository.Returns(repo); + return teamExplorerContext; + } + + static void SetActiveRepository(ITeamExplorerContext teamExplorerContext, ILocalRepositoryModel localRepositoryModel) + { + teamExplorerContext.ActiveRepository.Returns(localRepositoryModel); + teamExplorerContext.StatusChanged += Raise.Event(); + } } } diff --git a/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeTeamExplorerServiceHolder.cs b/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeTeamExplorerServiceHolder.cs deleted file mode 100644 index e6f5d1b2a9..0000000000 --- a/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeTeamExplorerServiceHolder.cs +++ /dev/null @@ -1,54 +0,0 @@ -using System; -using GitHub.Models; -using GitHub.Services; - -namespace GitHub.InlineReviews.UnitTests.TestDoubles -{ - class FakeTeamExplorerServiceHolder : ITeamExplorerServiceHolder - { - ILocalRepositoryModel activeRepo; - Action listener; - - public FakeTeamExplorerServiceHolder() - { - } - - public FakeTeamExplorerServiceHolder(ILocalRepositoryModel repo) - { - ActiveRepo = repo; - } - - public ILocalRepositoryModel ActiveRepo - { - get { return activeRepo; } - set { activeRepo = value; NotifyActiveRepoChanged(); } - } - - public IGitAwareItem HomeSection { get; set; } - - public IServiceProvider ServiceProvider { get; set; } - - public void ClearServiceProvider(IServiceProvider provider) - { - } - - public void NotifyActiveRepoChanged() - { - listener?.Invoke(activeRepo); - } - - public void Refresh() - { - } - - public void Subscribe(object who, Action handler) - { - listener = handler; - handler(ActiveRepo); - } - - public void Unsubscribe(object who) - { - } - } -} From f06ff667d0b780c55992818c6e48bca973e6370e Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 17 Jan 2018 16:30:18 +0000 Subject: [PATCH 07/64] Change PR session if Number or Owner changes --- src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs b/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs index 3bb43a7afd..d72779c3c6 100644 --- a/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs +++ b/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs @@ -212,7 +212,7 @@ async Task RepoChanged(ILocalRepositoryModel localRepositoryModel) { var pr = await service.GetPullRequestForCurrentBranch(localRepositoryModel).FirstOrDefaultAsync(); - if (pr?.Item1 != (CurrentSession?.PullRequest.Base.RepositoryCloneUrl.Owner) && + if (pr?.Item1 != (CurrentSession?.PullRequest.Base.RepositoryCloneUrl.Owner) || pr?.Item2 != (CurrentSession?.PullRequest.Number)) { var pullRequest = await GetPullRequestForTip(modelService, localRepositoryModel); From 8079f1720623785c39dd5e64ce4373491dab4fe4 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 17 Jan 2018 18:47:08 +0000 Subject: [PATCH 08/64] GetSession doesn't accept a null pullRequest --- src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs b/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs index d72779c3c6..358a91edd8 100644 --- a/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs +++ b/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs @@ -156,6 +156,8 @@ public string GetRelativePath(ITextBuffer buffer) /// public async Task GetSession(IPullRequestModel pullRequest) { + Guard.ArgumentNotNull(pullRequest, nameof(pullRequest)); + if (await service.EnsureLocalBranchesAreMarkedAsPullRequests(repository, pullRequest)) { // The branch for the PR was not previously marked with the PR number in the git From d31d9a425112b8fd4194ba1aa53d13f34f4a108a Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Thu, 18 Jan 2018 12:25:59 +0000 Subject: [PATCH 09/64] Set ActiveRepository to null when solution isn't in Git ActiveRepository should only be set to null when moving to a solution that isn't in Git. Avoid issue with multiple loads and unloads. --- src/GitHub.App/GitHub.App.csproj | 3 +++ .../Services/TeamExplorerContext.cs | 21 ++++++++++++++----- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/GitHub.App/GitHub.App.csproj b/src/GitHub.App/GitHub.App.csproj index e359cd6022..9c10b26b2b 100644 --- a/src/GitHub.App/GitHub.App.csproj +++ b/src/GitHub.App/GitHub.App.csproj @@ -50,6 +50,9 @@ + + False + ..\..\packages\LibGit2Sharp.0.23.1\lib\net40\LibGit2Sharp.dll True diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index 6d2b6e894c..bf3e89721e 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -7,6 +7,7 @@ using GitHub.Models; using GitHub.Logging; using Serilog; +using EnvDTE; namespace GitHub.Services { @@ -20,6 +21,9 @@ public class TeamExplorerContext : ITeamExplorerContext const string GitExtTypeName = "Microsoft.VisualStudio.TeamFoundation.Git.Extensibility.IGitExt, Microsoft.TeamFoundation.Git.Provider"; static readonly ILogger log = LogManager.ForContext(); + readonly DTE dte; + + string solutionPath; string repositoryPath; string branchName; string headSha; @@ -41,6 +45,12 @@ public TeamExplorerContext([Import(typeof(SVsServiceProvider))] IServiceProvider return; } + dte = (DTE)serviceProvider.GetService(typeof(DTE)); + if (dte == null) + { + log.Error("Couldn't find service for type {DteType}", typeof(DTE)); + } + Refresh(gitExt); var notifyPropertyChanged = gitExt as INotifyPropertyChanged; @@ -59,14 +69,14 @@ void Refresh(object gitExt) string newBranchName; string newHeadSha; FindActiveRepository(gitExt, out newRepositoryPath, out newBranchName, out newHeadSha); + var newSolutionPath = dte?.Solution?.FullName; - log.Information("Refresh ActiveRepository: RepositoryPath={RepositoryPath}, BranchName={BranchName}, HeadSha={HeadSha}", - newRepositoryPath, newBranchName, newHeadSha); + log.Information("Refresh ActiveRepository: RepositoryPath={RepositoryPath}, BranchName={BranchName}, HeadSha={HeadSha}, SolutionPath={SolutionPath}", + newRepositoryPath, newBranchName, newHeadSha, newSolutionPath); - if (newRepositoryPath == null) + if (newRepositoryPath == null && newSolutionPath == solutionPath) { - // Ignore when ActiveRepositories is empty. - // The ActiveRepository can be changed but not unloaded. + // Ignore when ActiveRepositories is empty and solution hasn't changed. // https://github.com/github/VisualStudio/issues/1421 log.Information("Ignoring null ActiveRepository"); } @@ -74,6 +84,7 @@ void Refresh(object gitExt) { log.Information("Fire PropertyChanged event for ActiveRepository"); + solutionPath = newSolutionPath; repositoryPath = newRepositoryPath; branchName = newBranchName; headSha = newHeadSha; From 6fbfb0a930906d2846f5a4188778778af2e22b1d Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Thu, 18 Jan 2018 13:13:00 +0000 Subject: [PATCH 10/64] Remove IVSGitExt hack from PullRequestDetailViewModel Converted to use IPullRequestSessionManager property change event. --- .../GitHubPane/PullRequestDetailViewModel.cs | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs b/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs index af9424ac37..34e96ebc47 100644 --- a/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs +++ b/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs @@ -4,6 +4,7 @@ using System.IO; using System.Linq; using System.Reactive; +using System.ComponentModel; using System.Reactive.Disposables; using System.Reactive.Linq; using System.Reactive.Threading.Tasks; @@ -34,7 +35,6 @@ public sealed class PullRequestDetailViewModel : PanePageViewModelBase, IPullReq readonly IPullRequestService pullRequestsService; readonly IPullRequestSessionManager sessionManager; readonly IUsageTracker usageTracker; - readonly IVSGitExt vsGitExt; IModelService modelService; IPullRequestModel model; string sourceBranchDisplayName; @@ -78,7 +78,6 @@ public PullRequestDetailViewModel( this.sessionManager = sessionManager; this.modelServiceFactory = modelServiceFactory; this.usageTracker = usageTracker; - this.vsGitExt = vsGitExt; Checkout = ReactiveCommand.CreateAsyncObservable( this.WhenAnyValue(x => x.CheckoutState) @@ -106,7 +105,7 @@ public PullRequestDetailViewModel( DiffFile = ReactiveCommand.Create(); DiffFileWithWorkingDirectory = ReactiveCommand.Create(this.WhenAnyValue(x => x.IsCheckedOut)); OpenFileInWorkingDirectory = ReactiveCommand.Create(this.WhenAnyValue(x => x.IsCheckedOut)); - ViewFile = ReactiveCommand.Create(); + ViewFile = ReactiveCommand.Create(); } /// @@ -322,7 +321,8 @@ public async Task InitializeAsync( Number = number; WebUrl = LocalRepository.CloneUrl.ToRepositoryUrl().Append("pull/" + number); modelService = await modelServiceFactory.CreateAsync(connection); - vsGitExt.ActiveRepositoriesChanged += ActiveRepositoriesChanged; + sessionManager.PropertyChanged += ActiveRepositoriesChanged; + await Refresh(); } finally @@ -340,7 +340,7 @@ public async Task Load(IPullRequestModel pullRequest) try { var firstLoad = (Model == null); - Model = pullRequest; + Model = pullRequest; Session = await sessionManager.GetSession(pullRequest); Title = Resources.PullRequestNavigationItemText + " #" + pullRequest.Number; @@ -514,22 +514,24 @@ protected override void Dispose(bool disposing) if (disposing) { - vsGitExt.ActiveRepositoriesChanged -= ActiveRepositoriesChanged; + sessionManager.PropertyChanged -= ActiveRepositoriesChanged; } } - async void ActiveRepositoriesChanged() + void ActiveRepositoriesChanged(object sender, PropertyChangedEventArgs e) { try { - if (active) + if (e.PropertyName == nameof(sessionManager.CurrentSession)) { - await ThreadingHelper.SwitchToMainThreadAsync(); - await Refresh(); - } - else - { - refreshOnActivate = true; + if (active) + { + Refresh().Forget(); + } + else + { + refreshOnActivate = true; + } } } catch (Exception ex) From 7c431717f9a8af31135ed9a7e06a2dbc6fb668ba Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Thu, 18 Jan 2018 15:30:27 +0000 Subject: [PATCH 11/64] Add tests for TeamExplorerContext Create a unit test project for GitHub.App. --- GitHubVS.sln | 13 +++ .../Services/TeamExplorerContext.cs | 43 ++++++++-- .../GitHub.App.UnitTests.csproj | 85 ++++++++++++++++++ .../Properties/AssemblyInfo.cs | 36 ++++++++ .../Services/TeamExplorerContextTests.cs | 86 +++++++++++++++++++ test/GitHub.App.UnitTests/packages.config | 7 ++ 6 files changed, 263 insertions(+), 7 deletions(-) create mode 100644 test/GitHub.App.UnitTests/GitHub.App.UnitTests.csproj create mode 100644 test/GitHub.App.UnitTests/Properties/AssemblyInfo.cs create mode 100644 test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs create mode 100644 test/GitHub.App.UnitTests/packages.config diff --git a/GitHubVS.sln b/GitHubVS.sln index 73705d4690..eda82c5a1e 100644 --- a/GitHubVS.sln +++ b/GitHubVS.sln @@ -104,6 +104,8 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "GitHub.InlineReviews.UnitTe EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "GitHub.Logging", "src\GitHub.Logging\GitHub.Logging.csproj", "{8D73575A-A89F-47CC-B153-B47DD06837F0}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "GitHub.App.UnitTests", "test\GitHub.App.UnitTests\GitHub.App.UnitTests.csproj", "{134DC1D8-6D5E-4780-895D-ECCE729DE05B}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -403,6 +405,16 @@ Global {8D73575A-A89F-47CC-B153-B47DD06837F0}.Release|Any CPU.Build.0 = Release|Any CPU {8D73575A-A89F-47CC-B153-B47DD06837F0}.ReleaseWithoutVsix|Any CPU.ActiveCfg = Release|Any CPU {8D73575A-A89F-47CC-B153-B47DD06837F0}.ReleaseWithoutVsix|Any CPU.Build.0 = Release|Any CPU + {134DC1D8-6D5E-4780-895D-ECCE729DE05B}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {134DC1D8-6D5E-4780-895D-ECCE729DE05B}.Debug|Any CPU.Build.0 = Debug|Any CPU + {134DC1D8-6D5E-4780-895D-ECCE729DE05B}.DebugCodeAnalysis|Any CPU.ActiveCfg = Debug|Any CPU + {134DC1D8-6D5E-4780-895D-ECCE729DE05B}.DebugCodeAnalysis|Any CPU.Build.0 = Debug|Any CPU + {134DC1D8-6D5E-4780-895D-ECCE729DE05B}.DebugWithoutVsix|Any CPU.ActiveCfg = Debug|Any CPU + {134DC1D8-6D5E-4780-895D-ECCE729DE05B}.DebugWithoutVsix|Any CPU.Build.0 = Debug|Any CPU + {134DC1D8-6D5E-4780-895D-ECCE729DE05B}.Release|Any CPU.ActiveCfg = Release|Any CPU + {134DC1D8-6D5E-4780-895D-ECCE729DE05B}.Release|Any CPU.Build.0 = Release|Any CPU + {134DC1D8-6D5E-4780-895D-ECCE729DE05B}.ReleaseWithoutVsix|Any CPU.ActiveCfg = Release|Any CPU + {134DC1D8-6D5E-4780-895D-ECCE729DE05B}.ReleaseWithoutVsix|Any CPU.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE @@ -426,5 +438,6 @@ Global {7B835A7D-CF94-45E8-B191-96F5A4FE26A8} = {8A7DA2E7-262B-4581-807A-1C45CE79CDFD} {110B206F-8554-4B51-BF86-94DAA32F5E26} = {8A7DA2E7-262B-4581-807A-1C45CE79CDFD} {17EB676B-BB91-48B5-AA59-C67695C647C2} = {8A7DA2E7-262B-4581-807A-1C45CE79CDFD} + {134DC1D8-6D5E-4780-895D-ECCE729DE05B} = {8A7DA2E7-262B-4581-807A-1C45CE79CDFD} EndGlobalSection EndGlobal diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index bf3e89721e..949fafc41c 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -6,6 +6,7 @@ using Microsoft.VisualStudio.Shell; using GitHub.Models; using GitHub.Logging; +using GitHub.Primitives; using Serilog; using EnvDTE; @@ -27,16 +28,17 @@ public class TeamExplorerContext : ITeamExplorerContext string repositoryPath; string branchName; string headSha; + bool testing; [ImportingConstructor] public TeamExplorerContext([Import(typeof(SVsServiceProvider))] IServiceProvider serviceProvider) + : this(serviceProvider, FindGitExtType(), false) { - var gitExtType = Type.GetType(GitExtTypeName, false); - if (gitExtType == null) - { - log.Error("Couldn't find type {GitExtTypeName}", GitExtTypeName); - return; - } + } + + public TeamExplorerContext([Import(typeof(SVsServiceProvider))] IServiceProvider serviceProvider, Type gitExtType, bool testing) + { + this.testing = testing; var gitExt = serviceProvider.GetService(gitExtType); if (gitExt == null) @@ -63,6 +65,17 @@ public TeamExplorerContext([Import(typeof(SVsServiceProvider))] IServiceProvider notifyPropertyChanged.PropertyChanged += (s, e) => Refresh(gitExt); } + static Type FindGitExtType() + { + var gitExtType = Type.GetType(GitExtTypeName, false); + if (gitExtType == null) + { + log.Error("Couldn't find type {GitExtTypeName}", GitExtTypeName); + } + + return gitExtType; + } + void Refresh(object gitExt) { string newRepositoryPath; @@ -89,7 +102,7 @@ void Refresh(object gitExt) branchName = newBranchName; headSha = newHeadSha; - ActiveRepository = repositoryPath != null ? new LocalRepositoryModel(repositoryPath) : null; + ActiveRepository = CreateRepository(repositoryPath); PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(ActiveRepository))); } else if (newBranchName != branchName || newHeadSha != headSha) @@ -103,6 +116,22 @@ void Refresh(object gitExt) } } + ILocalRepositoryModel CreateRepository(string repositoryPath) + { + if (repositoryPath == null) + { + return null; + } + + if (testing) + { + // HACK: This avoids calling GitService.GitServiceHelper. + return new LocalRepositoryModel("testing", new UriString("github.com/testing/testing"), repositoryPath); + } + + return new LocalRepositoryModel(repositoryPath); + } + static void FindActiveRepository(object gitExt, out string repositoryPath, out string branchName, out string headSha) { var activeRepositoriesProperty = gitExt.GetType().GetProperty("ActiveRepositories"); diff --git a/test/GitHub.App.UnitTests/GitHub.App.UnitTests.csproj b/test/GitHub.App.UnitTests/GitHub.App.UnitTests.csproj new file mode 100644 index 0000000000..38a053b1fe --- /dev/null +++ b/test/GitHub.App.UnitTests/GitHub.App.UnitTests.csproj @@ -0,0 +1,85 @@ + + + + + Debug + AnyCPU + {134DC1D8-6D5E-4780-895D-ECCE729DE05B} + Library + Properties + GitHub.App.UnitTests + GitHub.App.UnitTests + v4.6.1 + 512 + + + + true + full + false + bin\Debug\ + DEBUG;TRACE + prompt + 4 + + + pdbonly + true + bin\Release\ + TRACE + prompt + 4 + + + + ..\..\packages\Castle.Core.4.2.0\lib\net45\Castle.Core.dll + True + + + ..\..\packages\NSubstitute.3.1.0\lib\net46\NSubstitute.dll + True + + + ..\..\packages\NUnit.3.6.1\lib\net45\nunit.framework.dll + True + + + + + + ..\..\packages\System.Threading.Tasks.Extensions.4.3.0\lib\portable-net45+win8+wp8+wpa81\System.Threading.Tasks.Extensions.dll + True + + + + + + + + + + + + + + + + + + {1a1da411-8d1f-4578-80a6-04576bea2dc5} + GitHub.App + + + {9AEA02DB-02B5-409C-B0CA-115D05331A6B} + GitHub.Exports + + + + + \ No newline at end of file diff --git a/test/GitHub.App.UnitTests/Properties/AssemblyInfo.cs b/test/GitHub.App.UnitTests/Properties/AssemblyInfo.cs new file mode 100644 index 0000000000..96b11856bc --- /dev/null +++ b/test/GitHub.App.UnitTests/Properties/AssemblyInfo.cs @@ -0,0 +1,36 @@ +using System.Reflection; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; + +// General Information about an assembly is controlled through the following +// set of attributes. Change these attribute values to modify the information +// associated with an assembly. +[assembly: AssemblyTitle("GitHub.App.UnitTests")] +[assembly: AssemblyDescription("")] +[assembly: AssemblyConfiguration("")] +[assembly: AssemblyCompany("")] +[assembly: AssemblyProduct("GitHub.App.UnitTests")] +[assembly: AssemblyCopyright("Copyright © 2018")] +[assembly: AssemblyTrademark("")] +[assembly: AssemblyCulture("")] + +// Setting ComVisible to false makes the types in this assembly not visible +// to COM components. If you need to access a type in this assembly from +// COM, set the ComVisible attribute to true on that type. +[assembly: ComVisible(false)] + +// The following GUID is for the ID of the typelib if this project is exposed to COM +[assembly: Guid("134dc1d8-6d5e-4780-895d-ecce729de05b")] + +// Version information for an assembly consists of the following four values: +// +// Major Version +// Minor Version +// Build Number +// Revision +// +// You can specify all the values or you can default the Build and Revision Numbers +// by using the '*' as shown below: +// [assembly: AssemblyVersion("1.0.*")] +[assembly: AssemblyVersion("1.0.0.0")] +[assembly: AssemblyFileVersion("1.0.0.0")] diff --git a/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs b/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs new file mode 100644 index 0000000000..fbffe1d1d5 --- /dev/null +++ b/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs @@ -0,0 +1,86 @@ +using System; +using System.Collections.Generic; +using GitHub.Services; +using NUnit.Framework; +using NSubstitute; +using System.ComponentModel; +using System.IO; + +namespace GitHub.App.UnitTests.Services +{ + public class TeamExplorerContextTests + { + public class TheActiveRepositoryProperty + { + [Test] + public void NoActiveRepository() + { + var gitExt = new FakeGitExt(); + var target = CreateTeamExplorerContext(gitExt); + + var repo = target.ActiveRepository; + + Assert.That(repo, Is.Null); + } + + [Test] + public void SetActiveRepository() + { + var gitExt = new FakeGitExt(); + var repositoryPath = Directory.GetCurrentDirectory(); + var repoInfo = new GitRepositoryInfo(repositoryPath, null); + gitExt.SetActiveRepository(repoInfo); + var target = CreateTeamExplorerContext(gitExt); + + var repo = target.ActiveRepository; + + Assert.That(repo.LocalPath, Is.EqualTo(repositoryPath)); + } + } + + static TeamExplorerContext CreateTeamExplorerContext(FakeGitExt gitExt) + { + var gitExtType = typeof(FakeGitExt); + var sp = Substitute.For(); + sp.GetService(gitExtType).Returns(gitExt); + return new TeamExplorerContext(sp, gitExtType, true); + } + + class FakeGitExt : INotifyPropertyChanged + { + public void SetActiveRepository(GitRepositoryInfo repo) + { + ActiveRepositories = new[] { repo }; + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(ActiveRepositories))); + } + + public IReadOnlyList ActiveRepositories { get; private set; } + + public event PropertyChangedEventHandler PropertyChanged; + } + + class GitRepositoryInfo + { + public GitRepositoryInfo(string repositoryPath, GitBranchInfo currentBranch) + { + RepositoryPath = repositoryPath; + CurrentBranch = currentBranch; + } + + public string RepositoryPath { get; } + public GitBranchInfo CurrentBranch { get; } + } + + class GitBranchInfo + { + public GitBranchInfo(string name, string headSha) + { + Name = name; + HeadSha = headSha; + } + + public string Name { get; } + public string HeadSha { get; } + } + } +} diff --git a/test/GitHub.App.UnitTests/packages.config b/test/GitHub.App.UnitTests/packages.config new file mode 100644 index 0000000000..cda880517d --- /dev/null +++ b/test/GitHub.App.UnitTests/packages.config @@ -0,0 +1,7 @@ + + + + + + + \ No newline at end of file From 4b671537a7750be02a0a13564bc14cb11ff91a53 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Thu, 18 Jan 2018 16:06:25 +0000 Subject: [PATCH 12/64] Add tests for TeamExplorerContext.PropertyChanged event --- .../GitHub.App.UnitTests.csproj | 3 + .../Services/TeamExplorerContextTests.cs | 100 +++++++++++++++++- 2 files changed, 99 insertions(+), 4 deletions(-) diff --git a/test/GitHub.App.UnitTests/GitHub.App.UnitTests.csproj b/test/GitHub.App.UnitTests/GitHub.App.UnitTests.csproj index 38a053b1fe..75c871c541 100644 --- a/test/GitHub.App.UnitTests/GitHub.App.UnitTests.csproj +++ b/test/GitHub.App.UnitTests/GitHub.App.UnitTests.csproj @@ -35,6 +35,9 @@ ..\..\packages\Castle.Core.4.2.0\lib\net45\Castle.Core.dll True + + False + ..\..\packages\NSubstitute.3.1.0\lib\net46\NSubstitute.dll True diff --git a/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs b/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs index fbffe1d1d5..08af725fde 100644 --- a/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs +++ b/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs @@ -1,10 +1,11 @@ using System; +using System.IO; +using System.ComponentModel; using System.Collections.Generic; using GitHub.Services; using NUnit.Framework; using NSubstitute; -using System.ComponentModel; -using System.IO; +using EnvDTE; namespace GitHub.App.UnitTests.Services { @@ -38,11 +39,102 @@ public void SetActiveRepository() } } - static TeamExplorerContext CreateTeamExplorerContext(FakeGitExt gitExt) + public class ThePropertyChangedEvent + { + [Test] + public void SetActiveRepository() + { + var gitExt = new FakeGitExt(); + var repositoryPath = Directory.GetCurrentDirectory(); + var repoInfo = new GitRepositoryInfo(repositoryPath, null); + var target = CreateTeamExplorerContext(gitExt); + var eventWasRaised = false; + target.PropertyChanged += (s, e) => eventWasRaised = e.PropertyName == nameof(target.ActiveRepository); + + gitExt.SetActiveRepository(repoInfo); + + Assert.That(eventWasRaised, Is.True); + } + + [Test] + public void SetTwicePropertyChangedFiresOnce() + { + var gitExt = new FakeGitExt(); + var repositoryPath = Directory.GetCurrentDirectory(); + var repoInfo = new GitRepositoryInfo(repositoryPath, null); + var target = CreateTeamExplorerContext(gitExt); + var eventWasRaisedCount = 0; + target.PropertyChanged += (s, e) => eventWasRaisedCount++; + + gitExt.SetActiveRepository(repoInfo); + gitExt.SetActiveRepository(repoInfo); + + Assert.That(1, Is.EqualTo(1)); + } + + [Test] + public void ChangeActiveRepository() + { + var gitExt = new FakeGitExt(); + var repositoryPath = Directory.GetCurrentDirectory(); + var repoInfo = new GitRepositoryInfo(repositoryPath, null); + var repositoryPath2 = Path.GetTempPath(); + var repoInfo2 = new GitRepositoryInfo(repositoryPath2, null); + var target = CreateTeamExplorerContext(gitExt); + gitExt.SetActiveRepository(repoInfo); + var eventWasRaised = false; + target.PropertyChanged += (s, e) => eventWasRaised = e.PropertyName == nameof(target.ActiveRepository); + + gitExt.SetActiveRepository(repoInfo2); + + Assert.That(eventWasRaised, Is.True); + } + + [Test] + public void ClearActiveRepository_NoEventWhenNoSolutionChange() + { + var gitExt = new FakeGitExt(); + var repositoryPath = Directory.GetCurrentDirectory(); + var repoInfo = new GitRepositoryInfo(repositoryPath, null); + var target = CreateTeamExplorerContext(gitExt); + gitExt.SetActiveRepository(repoInfo); + var eventWasRaised = false; + target.PropertyChanged += (s, e) => eventWasRaised = e.PropertyName == nameof(target.ActiveRepository); + + gitExt.SetActiveRepository(null); + + Assert.That(eventWasRaised, Is.False); + Assert.That(target.ActiveRepository.LocalPath, Is.EqualTo(repositoryPath)); + } + + [Test] + public void ClearActiveRepository_FireWhenSolutionChanged() + { + var gitExt = new FakeGitExt(); + var repositoryPath = Directory.GetCurrentDirectory(); + var repoInfo = new GitRepositoryInfo(repositoryPath, null); + var dte = Substitute.For(); + var target = CreateTeamExplorerContext(gitExt, dte); + dte.Solution.FullName.Returns("Solution1"); + gitExt.SetActiveRepository(repoInfo); + var eventWasRaised = false; + target.PropertyChanged += (s, e) => eventWasRaised = e.PropertyName == nameof(target.ActiveRepository); + + dte.Solution.FullName.Returns("Solution2"); + gitExt.SetActiveRepository(null); + + Assert.That(eventWasRaised, Is.True); + Assert.That(target.ActiveRepository, Is.Null); + } + } + + static TeamExplorerContext CreateTeamExplorerContext(FakeGitExt gitExt, DTE dte = null) { var gitExtType = typeof(FakeGitExt); + dte = dte ?? Substitute.For(); var sp = Substitute.For(); sp.GetService(gitExtType).Returns(gitExt); + sp.GetService(typeof(DTE)).Returns(dte); return new TeamExplorerContext(sp, gitExtType, true); } @@ -50,7 +142,7 @@ class FakeGitExt : INotifyPropertyChanged { public void SetActiveRepository(GitRepositoryInfo repo) { - ActiveRepositories = new[] { repo }; + ActiveRepositories = repo != null ? new[] { repo } : new GitRepositoryInfo[0]; PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(ActiveRepositories))); } From cb8c1b60548d77e1cf68235912c34f70b27c64ba Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Thu, 18 Jan 2018 16:26:43 +0000 Subject: [PATCH 13/64] Add tests for TeamExplorerContext.StatusChanged event --- .../Services/TeamExplorerContextTests.cs | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs b/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs index 08af725fde..a7122bfef1 100644 --- a/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs +++ b/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs @@ -128,6 +128,33 @@ public void ClearActiveRepository_FireWhenSolutionChanged() } } + public class TheStatusChangedEvent + { + [TestCase(false, "name1", "sha1", "name1", "sha1", false)] + [TestCase(false, "name1", "sha1", "name2", "sha1", true)] + [TestCase(false, "name1", "sha1", "name1", "sha2", true)] + [TestCase(false, "name1", "sha1", "name2", "sha2", true)] + [TestCase(true, "name1", "sha1", "name1", "sha1", false)] + [TestCase(true, "name1", "sha1", "name2", "sha2", false)] + public void SameActiveRepository_ExpectWasRaised(bool changePath, string name1, string sha1, string name2, string sha2, bool expectWasRaised) + { + var gitExt = new FakeGitExt(); + var repositoryPaths = new[] { Directory.GetCurrentDirectory(), Path.GetTempPath() }; + var path1 = Directory.GetCurrentDirectory(); + var path2 = changePath ? Path.GetTempPath() : path1; + var repoInfo1 = new GitRepositoryInfo(path1, new GitBranchInfo(name1, sha1)); + var repoInfo2 = new GitRepositoryInfo(path2, new GitBranchInfo(name2, sha2)); + var target = CreateTeamExplorerContext(gitExt); + var eventWasRaised = false; + target.StatusChanged += (s, e) => eventWasRaised = true; + + gitExt.SetActiveRepository(repoInfo1); + gitExt.SetActiveRepository(repoInfo2); + + Assert.That(eventWasRaised, Is.EqualTo(expectWasRaised)); + } + } + static TeamExplorerContext CreateTeamExplorerContext(FakeGitExt gitExt, DTE dte = null) { var gitExtType = typeof(FakeGitExt); From 034d09bccfefafbefe65f96f94312d773227c7a1 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Thu, 18 Jan 2018 17:28:40 +0000 Subject: [PATCH 14/64] Change GitHubPaneViewModel to use ITeamExplorerContext This fixes an issue where a PR is checked out causing the PR list to reset. --- .../GitHubPane/GitHubPaneViewModel.cs | 32 +++++----- .../GitHubPane/GitHubPaneViewModelTests.cs | 58 +++++++++---------- 2 files changed, 44 insertions(+), 46 deletions(-) diff --git a/src/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModel.cs b/src/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModel.cs index c0a921573a..834a9215cc 100644 --- a/src/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModel.cs +++ b/src/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModel.cs @@ -15,6 +15,7 @@ using GitHub.Primitives; using GitHub.Services; using GitHub.VisualStudio; +using GitHub.Helpers; using ReactiveUI; using OleMenuCommand = Microsoft.VisualStudio.Shell.OleMenuCommand; @@ -32,7 +33,7 @@ public sealed class GitHubPaneViewModel : ViewModelBase, IGitHubPaneViewModel, I readonly IViewViewModelFactory viewModelFactory; readonly ISimpleApiClientFactory apiClientFactory; readonly IConnectionManager connectionManager; - readonly ITeamExplorerServiceHolder teServiceHolder; + readonly ITeamExplorerContext teamExplorerContext; readonly IVisualStudioBrowser browser; readonly IUsageTracker usageTracker; readonly INavigationViewModel navigator; @@ -46,7 +47,6 @@ public sealed class GitHubPaneViewModel : ViewModelBase, IGitHubPaneViewModel, I readonly ReactiveCommand refresh; readonly ReactiveCommand showPullRequests; readonly ReactiveCommand openInBrowser; - bool initialized; IViewModel content; ILocalRepositoryModel localRepository; string searchQuery; @@ -56,7 +56,7 @@ public GitHubPaneViewModel( IViewViewModelFactory viewModelFactory, ISimpleApiClientFactory apiClientFactory, IConnectionManager connectionManager, - ITeamExplorerServiceHolder teServiceHolder, + ITeamExplorerContext teamExplorerContext, IVisualStudioBrowser browser, IUsageTracker usageTracker, INavigationViewModel navigator, @@ -67,7 +67,7 @@ public GitHubPaneViewModel( Guard.ArgumentNotNull(viewModelFactory, nameof(viewModelFactory)); Guard.ArgumentNotNull(apiClientFactory, nameof(apiClientFactory)); Guard.ArgumentNotNull(connectionManager, nameof(connectionManager)); - Guard.ArgumentNotNull(teServiceHolder, nameof(teServiceHolder)); + Guard.ArgumentNotNull(teamExplorerContext, nameof(teamExplorerContext)); Guard.ArgumentNotNull(browser, nameof(browser)); Guard.ArgumentNotNull(usageTracker, nameof(usageTracker)); Guard.ArgumentNotNull(navigator, nameof(navigator)); @@ -78,7 +78,7 @@ public GitHubPaneViewModel( this.viewModelFactory = viewModelFactory; this.apiClientFactory = apiClientFactory; this.connectionManager = connectionManager; - this.teServiceHolder = teServiceHolder; + this.teamExplorerContext = teamExplorerContext; this.browser = browser; this.usageTracker = usageTracker; this.navigator = navigator; @@ -199,8 +199,15 @@ public void Dispose() /// public async Task InitializeAsync(IServiceProvider paneServiceProvider) { - await UpdateContent(teServiceHolder.ActiveRepo); - teServiceHolder.Subscribe(this, x => UpdateContentIfRepositoryChanged(x).Forget()); + await UpdateContent(teamExplorerContext.ActiveRepository); + teamExplorerContext.PropertyChanged += async (s, e) => + { + if (e.PropertyName == nameof(teamExplorerContext.ActiveRepository)) + { + await ThreadingHelper.SwitchToMainThreadAsync(); + await UpdateContent(teamExplorerContext.ActiveRepository); + } + }; connectionManager.Connections.CollectionChanged += (_, __) => UpdateContent(LocalRepository).Forget(); BindNavigatorCommand(paneServiceProvider, PkgCmdIDList.pullRequestCommand, showPullRequests); @@ -274,7 +281,7 @@ public Task ShowCreatePullRequest() /// public Task ShowPullRequests() { - return NavigateTo(x => x.InitializeAsync(LocalRepository, Connection)); + return NavigateTo(x => x.InitializeAsync(LocalRepository, Connection)); } /// @@ -344,7 +351,6 @@ async Task NavigateTo(Func initialize, Func IsValidRepository(ISimpleApiClient client) { try diff --git a/test/UnitTests/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModelTests.cs b/test/UnitTests/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModelTests.cs index 116bf080bf..0456ceae9a 100644 --- a/test/UnitTests/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModelTests.cs +++ b/test/UnitTests/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModelTests.cs @@ -23,9 +23,9 @@ public class TheInitializeMethod [Fact] public async Task NotAGitRepositoryShownWhenNoRepository() { - var te = Substitute.For(); - te.ActiveRepo.Returns(default(ILocalRepositoryModel)); - var target = CreateTarget(teServiceHolder: te); + var te = Substitute.For(); + te.ActiveRepository.Returns(default(ILocalRepositoryModel)); + var target = CreateTarget(teamExplorerContext: te); await Initialize(target); @@ -36,8 +36,8 @@ public async Task NotAGitRepositoryShownWhenNoRepository() public async Task NotAGitHubRepositoryShownWhenRepositoryCloneUrlIsNull() { var repo = Substitute.For(); - var te = CreateTeamExplorerServiceHolder(null); - var target = CreateTarget(teServiceHolder: te); + var te = CreateTeamExplorerContext(null); + var target = CreateTarget(teamExplorerContext: te); await Initialize(target); @@ -47,8 +47,8 @@ public async Task NotAGitHubRepositoryShownWhenRepositoryCloneUrlIsNull() [Fact] public async Task NotAGitHubRepositoryShownWhenRepositoryIsNotAGitHubInstance() { - var te = CreateTeamExplorerServiceHolder("https://some.site/foo/bar"); - var target = CreateTarget(teServiceHolder: te); + var te = CreateTeamExplorerContext("https://some.site/foo/bar"); + var target = CreateTarget(teamExplorerContext: te); await Initialize(target); @@ -58,8 +58,8 @@ public async Task NotAGitHubRepositoryShownWhenRepositoryIsNotAGitHubInstance() [Fact] public async Task NotAGitHubRepositoryShownWhenRepositoryIsADeletedGitHubRepo() { - var te = CreateTeamExplorerServiceHolder("https://github.com/invalid/repo"); - var target = CreateTarget(teServiceHolder: te); + var te = CreateTeamExplorerContext("https://github.com/invalid/repo"); + var target = CreateTarget(teamExplorerContext: te); await Initialize(target); @@ -69,9 +69,9 @@ public async Task NotAGitHubRepositoryShownWhenRepositoryIsADeletedGitHubRepo() [Fact] public async Task LoggedOutShownWhenNotLoggedInToGitHub() { - var te = CreateTeamExplorerServiceHolder(ValidGitHubRepo); + var te = CreateTeamExplorerContext(ValidGitHubRepo); var cm = CreateConnectionManager("https://enterprise.com"); - var target = CreateTarget(teServiceHolder: te, connectionManager: cm); + var target = CreateTarget(teamExplorerContext: te, connectionManager: cm); await Initialize(target); @@ -81,9 +81,9 @@ public async Task LoggedOutShownWhenNotLoggedInToGitHub() [Fact] public async Task LoggedOutShownWhenNotLoggedInToEnterprise() { - var te = CreateTeamExplorerServiceHolder(ValidEnterpriseRepo); + var te = CreateTeamExplorerContext(ValidEnterpriseRepo); var cm = CreateConnectionManager("https://github.com"); - var target = CreateTarget(teServiceHolder: te, connectionManager: cm); + var target = CreateTarget(teamExplorerContext: te, connectionManager: cm); await Initialize(target); @@ -93,9 +93,9 @@ public async Task LoggedOutShownWhenNotLoggedInToEnterprise() [Fact] public async Task NavigatorShownWhenRepositoryIsAGitHubRepo() { - var te = CreateTeamExplorerServiceHolder(ValidGitHubRepo); + var te = CreateTeamExplorerContext(ValidGitHubRepo); var cm = CreateConnectionManager("https://github.com"); - var target = CreateTarget(teServiceHolder: te, connectionManager: cm); + var target = CreateTarget(teamExplorerContext: te, connectionManager: cm); await Initialize(target); @@ -105,9 +105,9 @@ public async Task NavigatorShownWhenRepositoryIsAGitHubRepo() [Fact] public async Task NavigatorShownWhenRepositoryIsAnEnterpriseRepo() { - var te = CreateTeamExplorerServiceHolder(ValidEnterpriseRepo); + var te = CreateTeamExplorerContext(ValidEnterpriseRepo); var cm = CreateConnectionManager("https://enterprise.com"); - var target = CreateTarget(teServiceHolder: te, connectionManager: cm); + var target = CreateTarget(teamExplorerContext: te, connectionManager: cm); await Initialize(target); @@ -117,9 +117,9 @@ public async Task NavigatorShownWhenRepositoryIsAnEnterpriseRepo() [Fact] public async Task NavigatorShownWhenUserLogsIn() { - var te = CreateTeamExplorerServiceHolder(ValidGitHubRepo); + var te = CreateTeamExplorerContext(ValidGitHubRepo); var cm = CreateConnectionManager(); - var target = CreateTarget(teServiceHolder: te, connectionManager: cm); + var target = CreateTarget(teamExplorerContext: te, connectionManager: cm); await Initialize(target); @@ -136,12 +136,12 @@ public class TheShowPullRequestsMethod [Fact] public async Task HasNoEffectWhenUserLoggedOut() { - var te = CreateTeamExplorerServiceHolder(ValidGitHubRepo); + var te = CreateTeamExplorerContext(ValidGitHubRepo); var viewModelFactory = Substitute.For(); var target = CreateTarget( viewModelFactory: viewModelFactory, connectionManager: CreateConnectionManager(), - teServiceHolder: te); + teamExplorerContext: te); await Initialize(target); Assert.IsAssignableFrom(target.Content); @@ -154,11 +154,11 @@ public async Task HasNoEffectWhenUserLoggedOut() [Fact] public async Task HasNoEffectWhenAlreadyCurrentPage() { - var te = CreateTeamExplorerServiceHolder(ValidGitHubRepo); + var te = CreateTeamExplorerContext(ValidGitHubRepo); var cm = CreateConnectionManager(ValidGitHubRepo); var nav = new NavigationViewModel(); var target = CreateTarget( - teServiceHolder: te, + teamExplorerContext: te, connectionManager: cm, navigator: nav); @@ -176,7 +176,7 @@ static GitHubPaneViewModel CreateTarget( IViewViewModelFactory viewModelFactory = null, ISimpleApiClientFactory apiClientFactory = null, IConnectionManager connectionManager = null, - ITeamExplorerServiceHolder teServiceHolder = null, + ITeamExplorerContext teamExplorerContext = null, IVisualStudioBrowser browser = null, IUsageTracker usageTracker = null, INavigationViewModel navigator = null, @@ -186,7 +186,7 @@ static GitHubPaneViewModel CreateTarget( { viewModelFactory = viewModelFactory ?? Substitute.For(); connectionManager = connectionManager ?? Substitute.For(); - teServiceHolder = teServiceHolder ?? Substitute.For(); + teamExplorerContext = teamExplorerContext ?? Substitute.For(); browser = browser ?? Substitute.For(); usageTracker = usageTracker ?? Substitute.For(); loggedOut = loggedOut ?? Substitute.For(); @@ -219,7 +219,7 @@ static GitHubPaneViewModel CreateTarget( viewModelFactory, apiClientFactory, connectionManager, - teServiceHolder, + teamExplorerContext, browser, usageTracker, navigator, @@ -264,13 +264,13 @@ static INavigationViewModel CreateNavigator() return result; } - static ITeamExplorerServiceHolder CreateTeamExplorerServiceHolder(string repositoryCloneUrl) + static ITeamExplorerContext CreateTeamExplorerContext(string repositoryCloneUrl) { var repository = Substitute.For(); repository.CloneUrl.Returns(new UriString(repositoryCloneUrl)); - var result = Substitute.For(); - result.ActiveRepo.Returns(repository); + var result = Substitute.For(); + result.ActiveRepository.Returns(repository); return result; } From 03a41e7052319450165aaf8f29fa2b68c206b2a7 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 19 Jan 2018 09:55:26 +0000 Subject: [PATCH 15/64] Remove PullRequestDetailViewModel's dependency on IVSGitExt --- .../ViewModels/GitHubPane/PullRequestDetailViewModel.cs | 3 +-- .../ViewModels/GitHubPane/PullRequestDetailViewModelTests.cs | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs b/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs index 34e96ebc47..c270d918b5 100644 --- a/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs +++ b/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs @@ -66,8 +66,7 @@ public PullRequestDetailViewModel( IPullRequestService pullRequestsService, IPullRequestSessionManager sessionManager, IModelServiceFactory modelServiceFactory, - IUsageTracker usageTracker, - IVSGitExt vsGitExt) + IUsageTracker usageTracker) { Guard.ArgumentNotNull(pullRequestsService, nameof(pullRequestsService)); Guard.ArgumentNotNull(sessionManager, nameof(sessionManager)); diff --git a/test/UnitTests/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModelTests.cs b/test/UnitTests/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModelTests.cs index d48819ceef..6732ea3b1b 100644 --- a/test/UnitTests/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModelTests.cs +++ b/test/UnitTests/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModelTests.cs @@ -554,8 +554,7 @@ static Tuple CreateTargetAndSer pullRequestService, sessionManager ?? Substitute.For(), Substitute.For(), - Substitute.For(), - Substitute.For()); + Substitute.For()); vm.InitializeAsync(repository, Substitute.For(), "owner", "repo", 1).Wait(); return Tuple.Create(vm, pullRequestService); From 360ba9ab20877434e588cdd949440a00dbdd098e Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 19 Jan 2018 09:55:41 +0000 Subject: [PATCH 16/64] Fix CA error --- src/GitHub.App/Services/TeamExplorerContext.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index 949fafc41c..63255c907c 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -116,9 +116,9 @@ void Refresh(object gitExt) } } - ILocalRepositoryModel CreateRepository(string repositoryPath) + ILocalRepositoryModel CreateRepository(string path) { - if (repositoryPath == null) + if (path == null) { return null; } @@ -126,10 +126,10 @@ ILocalRepositoryModel CreateRepository(string repositoryPath) if (testing) { // HACK: This avoids calling GitService.GitServiceHelper. - return new LocalRepositoryModel("testing", new UriString("github.com/testing/testing"), repositoryPath); + return new LocalRepositoryModel("testing", new UriString("github.com/testing/testing"), path); } - return new LocalRepositoryModel(repositoryPath); + return new LocalRepositoryModel(path); } static void FindActiveRepository(object gitExt, out string repositoryPath, out string branchName, out string headSha) From c8dd2f6ec3fe0eefff65f9c407629bde8e4ffb21 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 19 Jan 2018 10:36:32 +0000 Subject: [PATCH 17/64] Add some docs for ITeamExplorerContext --- src/GitHub.Exports/Services/ITeamExplorerContext.cs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/GitHub.Exports/Services/ITeamExplorerContext.cs b/src/GitHub.Exports/Services/ITeamExplorerContext.cs index 7429997e81..a75e611bb2 100644 --- a/src/GitHub.Exports/Services/ITeamExplorerContext.cs +++ b/src/GitHub.Exports/Services/ITeamExplorerContext.cs @@ -4,9 +4,22 @@ namespace GitHub.Services { + /// + /// Responsible for watching the active repository in Team Explorer. + /// A PropertyChanged event is fired when moving to a new repository. + /// A StatusChanged event is fired when the CurrentBranch or HeadSha changes. + /// public interface ITeamExplorerContext : INotifyPropertyChanged { + /// + /// The active Git repository in Team Explorer. + /// This will be null if no repository is active. + /// ILocalRepositoryModel ActiveRepository { get; } + + /// + /// Fired when the CurrentBranch or HeadSha changes. + /// event EventHandler StatusChanged; } } From b11958536da8b672e1db12aaed510d3d78c9c11c Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 19 Jan 2018 10:55:11 +0000 Subject: [PATCH 18/64] Don't log when unit testing --- src/GitHub.App/Services/TeamExplorerContext.cs | 11 +++++++---- test/GitHub.App.UnitTests/GitHub.App.UnitTests.csproj | 8 +++++++- .../Services/TeamExplorerContextTests.cs | 4 +++- test/GitHub.App.UnitTests/packages.config | 1 + 4 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index 63255c907c..1aba960a54 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -20,8 +20,8 @@ namespace GitHub.Services public class TeamExplorerContext : ITeamExplorerContext { const string GitExtTypeName = "Microsoft.VisualStudio.TeamFoundation.Git.Extensibility.IGitExt, Microsoft.TeamFoundation.Git.Provider"; - static readonly ILogger log = LogManager.ForContext(); + readonly ILogger log; readonly DTE dte; string solutionPath; @@ -32,14 +32,17 @@ public class TeamExplorerContext : ITeamExplorerContext [ImportingConstructor] public TeamExplorerContext([Import(typeof(SVsServiceProvider))] IServiceProvider serviceProvider) - : this(serviceProvider, FindGitExtType(), false) + : this(serviceProvider, LogManager.ForContext(), null, false) { } - public TeamExplorerContext([Import(typeof(SVsServiceProvider))] IServiceProvider serviceProvider, Type gitExtType, bool testing) + public TeamExplorerContext([Import(typeof(SVsServiceProvider))] IServiceProvider serviceProvider, ILogger log, + Type gitExtType, bool testing) { + this.log = log; this.testing = testing; + gitExtType = gitExtType ?? FindGitExtType(); var gitExt = serviceProvider.GetService(gitExtType); if (gitExt == null) { @@ -65,7 +68,7 @@ public TeamExplorerContext([Import(typeof(SVsServiceProvider))] IServiceProvider notifyPropertyChanged.PropertyChanged += (s, e) => Refresh(gitExt); } - static Type FindGitExtType() + Type FindGitExtType() { var gitExtType = Type.GetType(GitExtTypeName, false); if (gitExtType == null) diff --git a/test/GitHub.App.UnitTests/GitHub.App.UnitTests.csproj b/test/GitHub.App.UnitTests/GitHub.App.UnitTests.csproj index 75c871c541..e08d9b203f 100644 --- a/test/GitHub.App.UnitTests/GitHub.App.UnitTests.csproj +++ b/test/GitHub.App.UnitTests/GitHub.App.UnitTests.csproj @@ -46,6 +46,10 @@ ..\..\packages\NUnit.3.6.1\lib\net45\nunit.framework.dll True + + ..\..\packages\Serilog.2.5.0\lib\net46\Serilog.dll + True + @@ -65,7 +69,9 @@ - + + Designer + diff --git a/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs b/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs index a7122bfef1..0664d07ff1 100644 --- a/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs +++ b/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs @@ -6,6 +6,7 @@ using NUnit.Framework; using NSubstitute; using EnvDTE; +using Serilog; namespace GitHub.App.UnitTests.Services { @@ -162,7 +163,8 @@ static TeamExplorerContext CreateTeamExplorerContext(FakeGitExt gitExt, DTE dte var sp = Substitute.For(); sp.GetService(gitExtType).Returns(gitExt); sp.GetService(typeof(DTE)).Returns(dte); - return new TeamExplorerContext(sp, gitExtType, true); + var log = Substitute.For(); + return new TeamExplorerContext(sp, log, gitExtType, true); } class FakeGitExt : INotifyPropertyChanged diff --git a/test/GitHub.App.UnitTests/packages.config b/test/GitHub.App.UnitTests/packages.config index cda880517d..4e0448638b 100644 --- a/test/GitHub.App.UnitTests/packages.config +++ b/test/GitHub.App.UnitTests/packages.config @@ -3,5 +3,6 @@ + \ No newline at end of file From 834f39d5409070180dfe842d6e630162f1b1c157 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 19 Jan 2018 11:01:34 +0000 Subject: [PATCH 19/64] Log exceptions in Refresh event Reduce logging verbosity. --- .../Services/TeamExplorerContext.cs | 68 ++++++++++--------- 1 file changed, 36 insertions(+), 32 deletions(-) diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index 1aba960a54..5afaa193de 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -81,41 +81,45 @@ Type FindGitExtType() void Refresh(object gitExt) { - string newRepositoryPath; - string newBranchName; - string newHeadSha; - FindActiveRepository(gitExt, out newRepositoryPath, out newBranchName, out newHeadSha); - var newSolutionPath = dte?.Solution?.FullName; - - log.Information("Refresh ActiveRepository: RepositoryPath={RepositoryPath}, BranchName={BranchName}, HeadSha={HeadSha}, SolutionPath={SolutionPath}", - newRepositoryPath, newBranchName, newHeadSha, newSolutionPath); - - if (newRepositoryPath == null && newSolutionPath == solutionPath) - { - // Ignore when ActiveRepositories is empty and solution hasn't changed. - // https://github.com/github/VisualStudio/issues/1421 - log.Information("Ignoring null ActiveRepository"); - } - else if (newRepositoryPath != repositoryPath) + try { - log.Information("Fire PropertyChanged event for ActiveRepository"); - - solutionPath = newSolutionPath; - repositoryPath = newRepositoryPath; - branchName = newBranchName; - headSha = newHeadSha; - - ActiveRepository = CreateRepository(repositoryPath); - PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(ActiveRepository))); + string newRepositoryPath; + string newBranchName; + string newHeadSha; + FindActiveRepository(gitExt, out newRepositoryPath, out newBranchName, out newHeadSha); + var newSolutionPath = dte?.Solution?.FullName; + + if (newRepositoryPath == null && newSolutionPath == solutionPath) + { + // Ignore when ActiveRepositories is empty and solution hasn't changed. + // https://github.com/github/VisualStudio/issues/1421 + log.Information("Ignoring no ActiveRepository when solution hasn't changed"); + } + else if (newRepositoryPath != repositoryPath) + { + log.Information("Fire PropertyChanged event for ActiveRepository"); + + solutionPath = newSolutionPath; + repositoryPath = newRepositoryPath; + branchName = newBranchName; + headSha = newHeadSha; + + ActiveRepository = CreateRepository(repositoryPath); + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(ActiveRepository))); + } + else if (newBranchName != branchName || newHeadSha != headSha) + { + log.Information("Fire StatusChanged event for ActiveRepository"); + + branchName = newBranchName; + headSha = newHeadSha; + + StatusChanged?.Invoke(this, EventArgs.Empty); + } } - else if (newBranchName != branchName || newHeadSha != headSha) + catch (Exception e) { - log.Information("Fire StatusChanged event for ActiveRepository"); - - branchName = newBranchName; - headSha = newHeadSha; - - StatusChanged?.Invoke(this, EventArgs.Empty); + log.Error(e, "Refreshing active repository"); } } From 18eba88d2d6ac5ef19dd8319c71a61e63ddbb5ee Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 19 Jan 2018 12:32:29 +0000 Subject: [PATCH 20/64] Use IGitHubServiceProvider instead of IServiceProvider --- src/GitHub.App/Services/TeamExplorerContext.cs | 6 +++--- .../Services/TeamExplorerContextTests.cs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index 5afaa193de..5ab3330e33 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -31,12 +31,12 @@ public class TeamExplorerContext : ITeamExplorerContext bool testing; [ImportingConstructor] - public TeamExplorerContext([Import(typeof(SVsServiceProvider))] IServiceProvider serviceProvider) + public TeamExplorerContext(IGitHubServiceProvider serviceProvider) : this(serviceProvider, LogManager.ForContext(), null, false) { } - public TeamExplorerContext([Import(typeof(SVsServiceProvider))] IServiceProvider serviceProvider, ILogger log, + public TeamExplorerContext(IGitHubServiceProvider serviceProvider, ILogger log, Type gitExtType, bool testing) { this.log = log; @@ -50,7 +50,7 @@ public TeamExplorerContext([Import(typeof(SVsServiceProvider))] IServiceProvider return; } - dte = (DTE)serviceProvider.GetService(typeof(DTE)); + dte = serviceProvider.TryGetService(); if (dte == null) { log.Error("Couldn't find service for type {DteType}", typeof(DTE)); diff --git a/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs b/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs index 0664d07ff1..055a409109 100644 --- a/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs +++ b/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs @@ -160,9 +160,9 @@ static TeamExplorerContext CreateTeamExplorerContext(FakeGitExt gitExt, DTE dte { var gitExtType = typeof(FakeGitExt); dte = dte ?? Substitute.For(); - var sp = Substitute.For(); + var sp = Substitute.For(); sp.GetService(gitExtType).Returns(gitExt); - sp.GetService(typeof(DTE)).Returns(dte); + sp.TryGetService().Returns(dte); var log = Substitute.For(); return new TeamExplorerContext(sp, log, gitExtType, true); } From be1b5b44048274e678463819f7efea8cf1c06877 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 19 Jan 2018 12:49:35 +0000 Subject: [PATCH 21/64] Use Splat.ModeDetector.InUnitTestRunner to detect runner --- src/GitHub.App/Services/TeamExplorerContext.cs | 10 +++------- test/GitHub.App.UnitTests/GitHub.App.UnitTests.csproj | 4 ++++ .../Services/TeamExplorerContextTests.cs | 8 +++++++- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index 5ab3330e33..127c676ded 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -3,7 +3,6 @@ using System.ComponentModel; using System.ComponentModel.Composition; using System.Collections.Generic; -using Microsoft.VisualStudio.Shell; using GitHub.Models; using GitHub.Logging; using GitHub.Primitives; @@ -28,19 +27,16 @@ public class TeamExplorerContext : ITeamExplorerContext string repositoryPath; string branchName; string headSha; - bool testing; [ImportingConstructor] public TeamExplorerContext(IGitHubServiceProvider serviceProvider) - : this(serviceProvider, LogManager.ForContext(), null, false) + : this(serviceProvider, LogManager.ForContext(), null) { } - public TeamExplorerContext(IGitHubServiceProvider serviceProvider, ILogger log, - Type gitExtType, bool testing) + public TeamExplorerContext(IGitHubServiceProvider serviceProvider, ILogger log, Type gitExtType) { this.log = log; - this.testing = testing; gitExtType = gitExtType ?? FindGitExtType(); var gitExt = serviceProvider.GetService(gitExtType); @@ -130,7 +126,7 @@ ILocalRepositoryModel CreateRepository(string path) return null; } - if (testing) + if (Splat.ModeDetector.InUnitTestRunner()) { // HACK: This avoids calling GitService.GitServiceHelper. return new LocalRepositoryModel("testing", new UriString("github.com/testing/testing"), path); diff --git a/test/GitHub.App.UnitTests/GitHub.App.UnitTests.csproj b/test/GitHub.App.UnitTests/GitHub.App.UnitTests.csproj index e08d9b203f..300bd103ce 100644 --- a/test/GitHub.App.UnitTests/GitHub.App.UnitTests.csproj +++ b/test/GitHub.App.UnitTests/GitHub.App.UnitTests.csproj @@ -82,6 +82,10 @@ {9AEA02DB-02B5-409C-B0CA-115D05331A6B} GitHub.Exports + + {252ce1c2-027a-4445-a3c2-e4d6c80a935a} + Splat-Net45 + - \ No newline at end of file diff --git a/test/GitHub.App.UnitTests/Properties/AssemblyInfo.cs b/test/GitHub.App.UnitTests/Properties/AssemblyInfo.cs deleted file mode 100644 index 96b11856bc..0000000000 --- a/test/GitHub.App.UnitTests/Properties/AssemblyInfo.cs +++ /dev/null @@ -1,36 +0,0 @@ -using System.Reflection; -using System.Runtime.CompilerServices; -using System.Runtime.InteropServices; - -// General Information about an assembly is controlled through the following -// set of attributes. Change these attribute values to modify the information -// associated with an assembly. -[assembly: AssemblyTitle("GitHub.App.UnitTests")] -[assembly: AssemblyDescription("")] -[assembly: AssemblyConfiguration("")] -[assembly: AssemblyCompany("")] -[assembly: AssemblyProduct("GitHub.App.UnitTests")] -[assembly: AssemblyCopyright("Copyright © 2018")] -[assembly: AssemblyTrademark("")] -[assembly: AssemblyCulture("")] - -// Setting ComVisible to false makes the types in this assembly not visible -// to COM components. If you need to access a type in this assembly from -// COM, set the ComVisible attribute to true on that type. -[assembly: ComVisible(false)] - -// The following GUID is for the ID of the typelib if this project is exposed to COM -[assembly: Guid("134dc1d8-6d5e-4780-895d-ecce729de05b")] - -// Version information for an assembly consists of the following four values: -// -// Major Version -// Minor Version -// Build Number -// Revision -// -// You can specify all the values or you can default the Build and Revision Numbers -// by using the '*' as shown below: -// [assembly: AssemblyVersion("1.0.*")] -[assembly: AssemblyVersion("1.0.0.0")] -[assembly: AssemblyFileVersion("1.0.0.0")] diff --git a/test/GitHub.App.UnitTests/packages.config b/test/GitHub.App.UnitTests/packages.config deleted file mode 100644 index 4e0448638b..0000000000 --- a/test/GitHub.App.UnitTests/packages.config +++ /dev/null @@ -1,8 +0,0 @@ - - - - - - - - \ No newline at end of file diff --git a/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs b/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs similarity index 100% rename from test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs rename to test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs diff --git a/test/UnitTests/UnitTests.csproj b/test/UnitTests/UnitTests.csproj index f656756030..98fd53727b 100644 --- a/test/UnitTests/UnitTests.csproj +++ b/test/UnitTests/UnitTests.csproj @@ -170,6 +170,10 @@ + + ..\..\packages\Serilog.2.5.0\lib\net46\Serilog.dll + True + @@ -234,6 +238,7 @@ + diff --git a/test/UnitTests/packages.config b/test/UnitTests/packages.config index 86b5a83f30..67b4372aab 100644 --- a/test/UnitTests/packages.config +++ b/test/UnitTests/packages.config @@ -41,4 +41,5 @@ + \ No newline at end of file From 9706afdd0371f2382e05b813202f41fe8d0b3cae Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Mon, 22 Jan 2018 17:34:05 +0000 Subject: [PATCH 28/64] Fire StatusChanged even when BranchName/HeadSha hasn't changed --- .../Services/TeamExplorerContext.cs | 49 ++++++++++------ .../Services/TeamExplorerContextTests.cs | 57 ++++++++++++++++++- 2 files changed, 86 insertions(+), 20 deletions(-) diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index 127c676ded..d26c169db8 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -27,6 +27,7 @@ public class TeamExplorerContext : ITeamExplorerContext string repositoryPath; string branchName; string headSha; + bool ignoreUnload; [ImportingConstructor] public TeamExplorerContext(IGitHubServiceProvider serviceProvider) @@ -90,27 +91,39 @@ void Refresh(object gitExt) // Ignore when ActiveRepositories is empty and solution hasn't changed. // https://github.com/github/VisualStudio/issues/1421 log.Information("Ignoring no ActiveRepository when solution hasn't changed"); + ignoreUnload = true; } - else if (newRepositoryPath != repositoryPath) + else { - log.Information("Fire PropertyChanged event for ActiveRepository"); - + if (newRepositoryPath != repositoryPath) + { + log.Information("Fire PropertyChanged event for ActiveRepository"); + repositoryPath = newRepositoryPath; + branchName = newBranchName; + headSha = newHeadSha; + ActiveRepository = CreateRepository(repositoryPath); + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(ActiveRepository))); + } + else if (newBranchName != branchName) + { + log.Information("Fire StatusChanged event when BranchName changes for ActiveRepository"); + branchName = newBranchName; + StatusChanged?.Invoke(this, EventArgs.Empty); + } + else if (newHeadSha != headSha) + { + log.Information("Fire StatusChanged event when HeadSha changes for ActiveRepository"); + headSha = newHeadSha; + StatusChanged?.Invoke(this, EventArgs.Empty); + } + else if (!ignoreUnload) + { + log.Information("Fire StatusChanged event for ActiveRepository"); + StatusChanged?.Invoke(this, EventArgs.Empty); + } + + ignoreUnload = false; solutionPath = newSolutionPath; - repositoryPath = newRepositoryPath; - branchName = newBranchName; - headSha = newHeadSha; - - ActiveRepository = CreateRepository(repositoryPath); - PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(ActiveRepository))); - } - else if (newBranchName != branchName || newHeadSha != headSha) - { - log.Information("Fire StatusChanged event for ActiveRepository"); - - branchName = newBranchName; - headSha = newHeadSha; - - StatusChanged?.Invoke(this, EventArgs.Empty); } } catch (Exception e) diff --git a/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs b/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs index 527d568097..068f295794 100644 --- a/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs +++ b/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs @@ -80,7 +80,7 @@ public void SetTwicePropertyChangedFiresOnce() } [Test] - public void ChangeActiveRepository() + public void ChangeActiveRepository_NoSolutionChange() { var gitExt = new FakeGitExt(); var repositoryPath = Directory.GetCurrentDirectory(); @@ -133,11 +133,30 @@ public void ClearActiveRepository_FireWhenSolutionChanged() Assert.That(eventWasRaised, Is.True); Assert.That(target.ActiveRepository, Is.Null); } + + [Test] + public void NoActiveRepositoryChange_SolutionChanges() + { + var gitExt = new FakeGitExt(); + var repositoryPath = Directory.GetCurrentDirectory(); + var repoInfo = new GitRepositoryInfo(repositoryPath, null); + var dte = Substitute.For(); + var target = CreateTeamExplorerContext(gitExt, dte); + dte.Solution.FullName.Returns(""); + gitExt.SetActiveRepository(repoInfo); + var eventWasRaised = false; + target.PropertyChanged += (s, e) => eventWasRaised = e.PropertyName == nameof(target.ActiveRepository); + + dte.Solution.FullName.Returns("Solution"); + gitExt.SetActiveRepository(repoInfo); + + Assert.That(eventWasRaised, Is.False); + } } public class TheStatusChangedEvent { - [TestCase(false, "name1", "sha1", "name1", "sha1", false)] + [TestCase(false, "name1", "sha1", "name1", "sha1", true)] [TestCase(false, "name1", "sha1", "name2", "sha1", true)] [TestCase(false, "name1", "sha1", "name1", "sha2", true)] [TestCase(false, "name1", "sha1", "name2", "sha2", true)] @@ -160,6 +179,40 @@ public void SameActiveRepository_ExpectWasRaised(bool changePath, string name1, Assert.That(eventWasRaised, Is.EqualTo(expectWasRaised)); } + + [Test] + public void SolutionUnloadedAndReloaded_DontFireStatusChanged() + { + var gitExt = new FakeGitExt(); + var path = Directory.GetCurrentDirectory(); + var repoInfo1 = new GitRepositoryInfo(path, new GitBranchInfo("name", "sha")); + var repoInfo2 = new GitRepositoryInfo(null, new GitBranchInfo(null, null)); + var target = CreateTeamExplorerContext(gitExt); + gitExt.SetActiveRepository(repoInfo1); + gitExt.SetActiveRepository(repoInfo2); + + var eventWasRaised = false; + target.StatusChanged += (s, e) => eventWasRaised = true; + gitExt.SetActiveRepository(repoInfo1); + + Assert.That(eventWasRaised, Is.False); + } + + [Test] + public void NameAndShaSameAfterPush_FireStatusChanged() + { + var gitExt = new FakeGitExt(); + var path = Directory.GetCurrentDirectory(); + var repoInfo = new GitRepositoryInfo(path, new GitBranchInfo("name", "sha")); + var target = CreateTeamExplorerContext(gitExt); + gitExt.SetActiveRepository(repoInfo); + + var eventWasRaised = false; + target.StatusChanged += (s, e) => eventWasRaised = true; + gitExt.SetActiveRepository(repoInfo); + + Assert.That(eventWasRaised, Is.True); + } } static TeamExplorerContext CreateTeamExplorerContext(FakeGitExt gitExt, DTE dte = null) From 384519a42c2e3bb5707f138f487a0123ce92a1c7 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Mon, 22 Jan 2018 18:06:36 +0000 Subject: [PATCH 29/64] Use TeamExplorerContext.StatusChanged to refresh PR detail view TeamExplorerContext.StatusChanged doesn't update when the HeadSha or tracking branch changes. We need to use TeamExplorerContext.StatusChanged instead. --- .../GitHubPane/PullRequestDetailViewModel.cs | 39 +++++++++++-------- .../PullRequestDetailViewModelTests.cs | 3 +- 2 files changed, 24 insertions(+), 18 deletions(-) diff --git a/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs b/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs index 1d66bab45b..773ad33690 100644 --- a/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs +++ b/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs @@ -35,6 +35,7 @@ public sealed class PullRequestDetailViewModel : PanePageViewModelBase, IPullReq readonly IPullRequestService pullRequestsService; readonly IPullRequestSessionManager sessionManager; readonly IUsageTracker usageTracker; + readonly ITeamExplorerContext teamExplorerContext; IModelService modelService; IPullRequestModel model; string sourceBranchDisplayName; @@ -51,7 +52,6 @@ public sealed class PullRequestDetailViewModel : PanePageViewModelBase, IPullReq bool active; bool refreshOnActivate; Uri webUrl; - IDisposable subscription; /// /// Initializes a new instance of the class. @@ -61,23 +61,26 @@ public sealed class PullRequestDetailViewModel : PanePageViewModelBase, IPullReq /// The pull requests service. /// The pull request session manager. /// The usage tracker. - /// The Visual Studio git service. + /// The context for tracking repo changes [ImportingConstructor] public PullRequestDetailViewModel( IPullRequestService pullRequestsService, IPullRequestSessionManager sessionManager, IModelServiceFactory modelServiceFactory, - IUsageTracker usageTracker) + IUsageTracker usageTracker, + ITeamExplorerContext teamExplorerContext) { Guard.ArgumentNotNull(pullRequestsService, nameof(pullRequestsService)); Guard.ArgumentNotNull(sessionManager, nameof(sessionManager)); Guard.ArgumentNotNull(modelServiceFactory, nameof(modelServiceFactory)); Guard.ArgumentNotNull(usageTracker, nameof(usageTracker)); + Guard.ArgumentNotNull(teamExplorerContext, nameof(teamExplorerContext)); this.pullRequestsService = pullRequestsService; this.sessionManager = sessionManager; this.modelServiceFactory = modelServiceFactory; this.usageTracker = usageTracker; + this.teamExplorerContext = teamExplorerContext; Checkout = ReactiveCommand.CreateAsyncObservable( this.WhenAnyValue(x => x.CheckoutState) @@ -323,19 +326,7 @@ public async Task InitializeAsync( modelService = await modelServiceFactory.CreateAsync(connection); await Refresh(); - subscription = sessionManager.WhenAnyValue(x => x.CurrentSession) - .Skip(1) - .Subscribe(y => - { - if (active) - { - Refresh().Forget(); - } - else - { - refreshOnActivate = true; - } - }); + teamExplorerContext.StatusChanged += RefreshLater; } finally { @@ -343,6 +334,18 @@ public async Task InitializeAsync( } } + void RefreshLater(object sender, EventArgs e) + { + if (active) + { + Refresh().Forget(); + } + else + { + refreshOnActivate = true; + } + } + /// /// Loads the view model from octokit models. /// @@ -453,6 +456,8 @@ public override async Task Refresh() { try { + await ThreadingHelper.SwitchToMainThreadAsync(); + Error = null; OperationError = null; IsBusy = true; @@ -526,7 +531,7 @@ protected override void Dispose(bool disposing) if (disposing) { - subscription.Dispose(); + teamExplorerContext.StatusChanged -= RefreshLater; } } diff --git a/test/UnitTests/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModelTests.cs b/test/UnitTests/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModelTests.cs index c674de567f..c5e3d71eeb 100644 --- a/test/UnitTests/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModelTests.cs +++ b/test/UnitTests/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModelTests.cs @@ -554,7 +554,8 @@ static Tuple CreateTargetAndSer pullRequestService, sessionManager ?? Substitute.For(), Substitute.For(), - Substitute.For()); + Substitute.For(), + Substitute.For()); vm.InitializeAsync(repository, Substitute.For(), "owner", "repo", 1).Wait(); return Tuple.Create(vm, pullRequestService); From 23af5ec6f32bced507e8feb662a1deaf629fd6eb Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Jan 2018 09:32:19 +0000 Subject: [PATCH 30/64] Fire StatusChanged when tracked branch changes We only want to fire StatusChanged when BranchName, HeadSha or TrackedSha changes. --- .../Services/TeamExplorerContext.cs | 36 +++++++++++++------ .../Services/TeamExplorerContextTests.cs | 18 +--------- 2 files changed, 26 insertions(+), 28 deletions(-) diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index d26c169db8..fe8a7f5bcc 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -8,6 +8,7 @@ using GitHub.Primitives; using Serilog; using EnvDTE; +using LibGit2Sharp; namespace GitHub.Services { @@ -27,7 +28,7 @@ public class TeamExplorerContext : ITeamExplorerContext string repositoryPath; string branchName; string headSha; - bool ignoreUnload; + string trackedSha; [ImportingConstructor] public TeamExplorerContext(IGitHubServiceProvider serviceProvider) @@ -85,45 +86,43 @@ void Refresh(object gitExt) string newHeadSha; FindActiveRepository(gitExt, out newRepositoryPath, out newBranchName, out newHeadSha); var newSolutionPath = dte?.Solution?.FullName; + var newTrackedSha = newRepositoryPath != null ? FindTrackedSha(newRepositoryPath) : null; if (newRepositoryPath == null && newSolutionPath == solutionPath) { // Ignore when ActiveRepositories is empty and solution hasn't changed. // https://github.com/github/VisualStudio/issues/1421 log.Information("Ignoring no ActiveRepository when solution hasn't changed"); - ignoreUnload = true; } else { if (newRepositoryPath != repositoryPath) { log.Information("Fire PropertyChanged event for ActiveRepository"); - repositoryPath = newRepositoryPath; - branchName = newBranchName; - headSha = newHeadSha; - ActiveRepository = CreateRepository(repositoryPath); + ActiveRepository = CreateRepository(newRepositoryPath); PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(ActiveRepository))); } else if (newBranchName != branchName) { log.Information("Fire StatusChanged event when BranchName changes for ActiveRepository"); - branchName = newBranchName; StatusChanged?.Invoke(this, EventArgs.Empty); } else if (newHeadSha != headSha) { log.Information("Fire StatusChanged event when HeadSha changes for ActiveRepository"); - headSha = newHeadSha; StatusChanged?.Invoke(this, EventArgs.Empty); } - else if (!ignoreUnload) + else if (newTrackedSha != trackedSha) { - log.Information("Fire StatusChanged event for ActiveRepository"); + log.Information("Fire StatusChanged event when TrackedSha changes for ActiveRepository"); StatusChanged?.Invoke(this, EventArgs.Empty); } - ignoreUnload = false; + repositoryPath = newRepositoryPath; + branchName = newBranchName; + headSha = newHeadSha; solutionPath = newSolutionPath; + trackedSha = newTrackedSha; } } catch (Exception e) @@ -132,6 +131,21 @@ void Refresh(object gitExt) } } + static string FindTrackedSha(string repositoryPath) + { + try + { + using (var repo = new Repository(repositoryPath)) + { + return repo.Head.TrackedBranch?.Tip.Sha; + } + } + catch (RepositoryNotFoundException) + { + return null; + } + } + ILocalRepositoryModel CreateRepository(string path) { if (path == null) diff --git a/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs b/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs index 068f295794..b77047052b 100644 --- a/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs +++ b/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs @@ -156,7 +156,7 @@ public void NoActiveRepositoryChange_SolutionChanges() public class TheStatusChangedEvent { - [TestCase(false, "name1", "sha1", "name1", "sha1", true)] + [TestCase(false, "name1", "sha1", "name1", "sha1", false)] [TestCase(false, "name1", "sha1", "name2", "sha1", true)] [TestCase(false, "name1", "sha1", "name1", "sha2", true)] [TestCase(false, "name1", "sha1", "name2", "sha2", true)] @@ -197,22 +197,6 @@ public void SolutionUnloadedAndReloaded_DontFireStatusChanged() Assert.That(eventWasRaised, Is.False); } - - [Test] - public void NameAndShaSameAfterPush_FireStatusChanged() - { - var gitExt = new FakeGitExt(); - var path = Directory.GetCurrentDirectory(); - var repoInfo = new GitRepositoryInfo(path, new GitBranchInfo("name", "sha")); - var target = CreateTeamExplorerContext(gitExt); - gitExt.SetActiveRepository(repoInfo); - - var eventWasRaised = false; - target.StatusChanged += (s, e) => eventWasRaised = true; - gitExt.SetActiveRepository(repoInfo); - - Assert.That(eventWasRaised, Is.True); - } } static TeamExplorerContext CreateTeamExplorerContext(FakeGitExt gitExt, DTE dte = null) From ed4bf0100e92d68c0351eeed9e665f7dcb60ee38 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Jan 2018 09:58:33 +0000 Subject: [PATCH 31/64] Code cleanup and comments --- .../Services/TeamExplorerContext.cs | 32 +++++++------------ .../Services/TeamExplorerContextTests.cs | 2 +- 2 files changed, 13 insertions(+), 21 deletions(-) diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index fe8a7f5bcc..a479df527b 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -32,15 +32,23 @@ public class TeamExplorerContext : ITeamExplorerContext [ImportingConstructor] public TeamExplorerContext(IGitHubServiceProvider serviceProvider) - : this(serviceProvider, LogManager.ForContext(), null) + : this(serviceProvider, LogManager.ForContext(), GitExtTypeName) { } - public TeamExplorerContext(IGitHubServiceProvider serviceProvider, ILogger log, Type gitExtType) + public TeamExplorerContext(IGitHubServiceProvider serviceProvider, ILogger log, string gitExtTypeName) { this.log = log; - gitExtType = gitExtType ?? FindGitExtType(); + // Visual Studio 2015 and 2017 use different versions of the Microsoft.TeamFoundation.Git.Provider assembly. + // There are no binding redirections between them, but the package that includes them defines a ProvideBindingPath + // attrubute. This means the required IGitExt type can be found using an unqualified assembly name (GitExtTypeName). + var gitExtType = Type.GetType(gitExtTypeName, false); + if (gitExtType == null) + { + log.Error("Couldn't find type {GitExtTypeName}", gitExtTypeName); + } + var gitExt = serviceProvider.GetService(gitExtType); if (gitExt == null) { @@ -66,17 +74,6 @@ public TeamExplorerContext(IGitHubServiceProvider serviceProvider, ILogger log, notifyPropertyChanged.PropertyChanged += (s, e) => Refresh(gitExt); } - Type FindGitExtType() - { - var gitExtType = Type.GetType(GitExtTypeName, false); - if (gitExtType == null) - { - log.Error("Couldn't find type {GitExtTypeName}", GitExtTypeName); - } - - return gitExtType; - } - void Refresh(object gitExt) { try @@ -99,7 +96,7 @@ void Refresh(object gitExt) if (newRepositoryPath != repositoryPath) { log.Information("Fire PropertyChanged event for ActiveRepository"); - ActiveRepository = CreateRepository(newRepositoryPath); + ActiveRepository = newRepositoryPath != null ? CreateRepository(newRepositoryPath) : null; PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(ActiveRepository))); } else if (newBranchName != branchName) @@ -148,11 +145,6 @@ static string FindTrackedSha(string repositoryPath) ILocalRepositoryModel CreateRepository(string path) { - if (path == null) - { - return null; - } - if (Splat.ModeDetector.InUnitTestRunner()) { // HACK: This avoids calling GitService.GitServiceHelper. diff --git a/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs b/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs index b77047052b..4585ac7550 100644 --- a/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs +++ b/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs @@ -207,7 +207,7 @@ static TeamExplorerContext CreateTeamExplorerContext(FakeGitExt gitExt, DTE dte sp.GetService(gitExtType).Returns(gitExt); sp.TryGetService().Returns(dte); var log = Substitute.For(); - return new TeamExplorerContext(sp, log, gitExtType); + return new TeamExplorerContext(sp, log, gitExtType.AssemblyQualifiedName); } class FakeGitExt : INotifyPropertyChanged From a2c3fe1448113b66ee00d6e99fb11096ae1500e3 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Jan 2018 10:32:50 +0000 Subject: [PATCH 32/64] Simplify testing by using a nested interface --- .../Services/TeamExplorerContext.cs | 57 +++++++++---------- .../Services/TeamExplorerContextTests.cs | 20 +++++-- 2 files changed, 40 insertions(+), 37 deletions(-) diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index a479df527b..433c9b51f4 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -5,7 +5,6 @@ using System.Collections.Generic; using GitHub.Models; using GitHub.Logging; -using GitHub.Primitives; using Serilog; using EnvDTE; using LibGit2Sharp; @@ -30,15 +29,18 @@ public class TeamExplorerContext : ITeamExplorerContext string headSha; string trackedSha; + readonly IService service = new Service(); + [ImportingConstructor] public TeamExplorerContext(IGitHubServiceProvider serviceProvider) - : this(serviceProvider, LogManager.ForContext(), GitExtTypeName) + : this(LogManager.ForContext(), new Service(), GitExtTypeName, serviceProvider) { } - public TeamExplorerContext(IGitHubServiceProvider serviceProvider, ILogger log, string gitExtTypeName) + public TeamExplorerContext(ILogger log, IService service, string gitExtTypeName, IGitHubServiceProvider serviceProvider) { this.log = log; + this.service = service; // Visual Studio 2015 and 2017 use different versions of the Microsoft.TeamFoundation.Git.Provider assembly. // There are no binding redirections between them, but the package that includes them defines a ProvideBindingPath @@ -74,6 +76,25 @@ public TeamExplorerContext(IGitHubServiceProvider serviceProvider, ILogger log, notifyPropertyChanged.PropertyChanged += (s, e) => Refresh(gitExt); } + public interface IService + { + string FindTrackedSha(string repositoryPath); + ILocalRepositoryModel CreateRepository(string path); + } + + class Service : IService + { + public string FindTrackedSha(string repositoryPath) + { + using (var repo = new Repository(repositoryPath)) + { + return repo.Head.TrackedBranch?.Tip.Sha; + } + } + + public ILocalRepositoryModel CreateRepository(string path) => new LocalRepositoryModel(path); + } + void Refresh(object gitExt) { try @@ -83,7 +104,7 @@ void Refresh(object gitExt) string newHeadSha; FindActiveRepository(gitExt, out newRepositoryPath, out newBranchName, out newHeadSha); var newSolutionPath = dte?.Solution?.FullName; - var newTrackedSha = newRepositoryPath != null ? FindTrackedSha(newRepositoryPath) : null; + var newTrackedSha = newRepositoryPath != null ? service.FindTrackedSha(newRepositoryPath) : null; if (newRepositoryPath == null && newSolutionPath == solutionPath) { @@ -96,7 +117,7 @@ void Refresh(object gitExt) if (newRepositoryPath != repositoryPath) { log.Information("Fire PropertyChanged event for ActiveRepository"); - ActiveRepository = newRepositoryPath != null ? CreateRepository(newRepositoryPath) : null; + ActiveRepository = newRepositoryPath != null ? service.CreateRepository(newRepositoryPath) : null; PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(ActiveRepository))); } else if (newBranchName != branchName) @@ -128,32 +149,6 @@ void Refresh(object gitExt) } } - static string FindTrackedSha(string repositoryPath) - { - try - { - using (var repo = new Repository(repositoryPath)) - { - return repo.Head.TrackedBranch?.Tip.Sha; - } - } - catch (RepositoryNotFoundException) - { - return null; - } - } - - ILocalRepositoryModel CreateRepository(string path) - { - if (Splat.ModeDetector.InUnitTestRunner()) - { - // HACK: This avoids calling GitService.GitServiceHelper. - return new LocalRepositoryModel("testing", new UriString("github.com/testing/testing"), path); - } - - return new LocalRepositoryModel(path); - } - static void FindActiveRepository(object gitExt, out string repositoryPath, out string branchName, out string headSha) { var activeRepositoriesProperty = gitExt.GetType().GetProperty("ActiveRepositories"); diff --git a/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs b/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs index 4585ac7550..e8be077a1f 100644 --- a/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs +++ b/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs @@ -7,6 +7,7 @@ using NSubstitute; using EnvDTE; using Serilog; +using GitHub.Models; namespace GitHub.App.UnitTests.Services { @@ -38,11 +39,14 @@ public void SetActiveRepository() var repositoryPath = Directory.GetCurrentDirectory(); var repoInfo = new GitRepositoryInfo(repositoryPath, null); gitExt.SetActiveRepository(repoInfo); - var target = CreateTeamExplorerContext(gitExt); + var service = Substitute.For(); + var expectRepo = Substitute.For(); + service.CreateRepository(repositoryPath).Returns(expectRepo); + var target = CreateTeamExplorerContext(gitExt, service: service); var repo = target.ActiveRepository; - Assert.That(repo.LocalPath, Is.EqualTo(repositoryPath)); + Assert.That(repo, Is.EqualTo(expectRepo)); } } @@ -103,7 +107,10 @@ public void ClearActiveRepository_NoEventWhenNoSolutionChange() var gitExt = new FakeGitExt(); var repositoryPath = Directory.GetCurrentDirectory(); var repoInfo = new GitRepositoryInfo(repositoryPath, null); - var target = CreateTeamExplorerContext(gitExt); + var service = Substitute.For(); + var expectRepo = Substitute.For(); + service.CreateRepository(repositoryPath).Returns(expectRepo); + var target = CreateTeamExplorerContext(gitExt, service: service); gitExt.SetActiveRepository(repoInfo); var eventWasRaised = false; target.PropertyChanged += (s, e) => eventWasRaised = e.PropertyName == nameof(target.ActiveRepository); @@ -111,7 +118,7 @@ public void ClearActiveRepository_NoEventWhenNoSolutionChange() gitExt.SetActiveRepository(null); Assert.That(eventWasRaised, Is.False); - Assert.That(target.ActiveRepository.LocalPath, Is.EqualTo(repositoryPath)); + Assert.That(target.ActiveRepository, Is.EqualTo(expectRepo)); } [Test] @@ -199,15 +206,16 @@ public void SolutionUnloadedAndReloaded_DontFireStatusChanged() } } - static TeamExplorerContext CreateTeamExplorerContext(FakeGitExt gitExt, DTE dte = null) + static TeamExplorerContext CreateTeamExplorerContext(FakeGitExt gitExt, DTE dte = null, TeamExplorerContext.IService service = null) { var gitExtType = typeof(FakeGitExt); dte = dte ?? Substitute.For(); + service = service ?? Substitute.For(); var sp = Substitute.For(); sp.GetService(gitExtType).Returns(gitExt); sp.TryGetService().Returns(dte); var log = Substitute.For(); - return new TeamExplorerContext(sp, log, gitExtType.AssemblyQualifiedName); + return new TeamExplorerContext(log, service, gitExtType.AssemblyQualifiedName, sp); } class FakeGitExt : INotifyPropertyChanged From 850f0893dbca2e20bc4b8c795a330061c327dd31 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Jan 2018 11:00:10 +0000 Subject: [PATCH 33/64] Add test for when only TrackedSha changes --- .../Services/TeamExplorerContextTests.cs | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs b/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs index e8be077a1f..d699e6f38e 100644 --- a/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs +++ b/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs @@ -187,6 +187,26 @@ public void SameActiveRepository_ExpectWasRaised(bool changePath, string name1, Assert.That(eventWasRaised, Is.EqualTo(expectWasRaised)); } + [TestCase("trackedSha", "trackedSha", false)] + [TestCase("trackedSha1", "trackedSha2", true)] + public void TrackedShaChanges_CheckWasRaised(string trackedSha1, string trackedSha2, bool expectWasRaised) + { + var gitExt = new FakeGitExt(); + var repositoryPaths = new[] { Directory.GetCurrentDirectory(), Path.GetTempPath() }; + var repoPath = Directory.GetCurrentDirectory(); + var repoInfo = new GitRepositoryInfo(repoPath, new GitBranchInfo("name", "sha")); + var service = Substitute.For(); + service.FindTrackedSha(Arg.Any()).Returns(trackedSha1, trackedSha2); + var target = CreateTeamExplorerContext(gitExt, service: service); + gitExt.SetActiveRepository(repoInfo); + var eventWasRaised = false; + target.StatusChanged += (s, e) => eventWasRaised = true; + + gitExt.SetActiveRepository(repoInfo); + + Assert.That(eventWasRaised, Is.EqualTo(expectWasRaised)); + } + [Test] public void SolutionUnloadedAndReloaded_DontFireStatusChanged() { From bfba18646539a55b4c77fe14be1724c68ab4a71e Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Jan 2018 11:30:48 +0000 Subject: [PATCH 34/64] Use class instead of out params --- .../Services/TeamExplorerContext.cs | 44 ++++++++++++------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index 433c9b51f4..64e3c5e941 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -99,14 +99,10 @@ void Refresh(object gitExt) { try { - string newRepositoryPath; - string newBranchName; - string newHeadSha; - FindActiveRepository(gitExt, out newRepositoryPath, out newBranchName, out newHeadSha); + var repo = FindActiveRepository(gitExt); var newSolutionPath = dte?.Solution?.FullName; - var newTrackedSha = newRepositoryPath != null ? service.FindTrackedSha(newRepositoryPath) : null; - if (newRepositoryPath == null && newSolutionPath == solutionPath) + if (repo == null && newSolutionPath == solutionPath) { // Ignore when ActiveRepositories is empty and solution hasn't changed. // https://github.com/github/VisualStudio/issues/1421 @@ -114,10 +110,15 @@ void Refresh(object gitExt) } else { + var newRepositoryPath = repo?.RepositoryPath; + var newBranchName = repo?.BranchName; + var newHeadSha = repo?.HeadSha; + var newTrackedSha = newRepositoryPath != null ? service.FindTrackedSha(newRepositoryPath) : null; + if (newRepositoryPath != repositoryPath) { log.Information("Fire PropertyChanged event for ActiveRepository"); - ActiveRepository = newRepositoryPath != null ? service.CreateRepository(newRepositoryPath) : null; + ActiveRepository = repo != null ? service.CreateRepository(repo.RepositoryPath) : null; PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(ActiveRepository))); } else if (newBranchName != branchName) @@ -149,30 +150,43 @@ void Refresh(object gitExt) } } - static void FindActiveRepository(object gitExt, out string repositoryPath, out string branchName, out string headSha) + static RepositoryInfo FindActiveRepository(object gitExt) { var activeRepositoriesProperty = gitExt.GetType().GetProperty("ActiveRepositories"); var activeRepositories = (IEnumerable)activeRepositoriesProperty?.GetValue(gitExt); var repo = activeRepositories?.FirstOrDefault(); if (repo == null) { - repositoryPath = null; - branchName = null; - headSha = null; - return; + return null; } var repositoryPathProperty = repo.GetType().GetProperty("RepositoryPath"); - repositoryPath = (string)repositoryPathProperty?.GetValue(repo); + var repositoryPath = (string)repositoryPathProperty?.GetValue(repo); var currentBranchProperty = repo.GetType().GetProperty("CurrentBranch"); var currentBranch = currentBranchProperty?.GetValue(repo); var headShaProperty = currentBranch?.GetType().GetProperty("HeadSha"); - headSha = (string)headShaProperty?.GetValue(currentBranch); + var headSha = (string)headShaProperty?.GetValue(currentBranch); var nameProperty = currentBranch?.GetType().GetProperty("Name"); - branchName = (string)nameProperty?.GetValue(currentBranch); + var branchName = (string)nameProperty?.GetValue(currentBranch); + + return new RepositoryInfo(repositoryPath, branchName, headSha); + } + + class RepositoryInfo + { + public RepositoryInfo(string repositoryPath, string branchName, string headSha) + { + RepositoryPath = repositoryPath; + BranchName = branchName; + HeadSha = headSha; + } + + public string RepositoryPath { get; } + public string BranchName { get; } + public string HeadSha { get; } } public ILocalRepositoryModel ActiveRepository From 067e320790daf8eb370154f58a7e84013c6a912e Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Jan 2018 12:36:56 +0000 Subject: [PATCH 35/64] Add xmldoc comments for TeamExplorerContext --- .../Services/TeamExplorerContext.cs | 23 ++++++++++++++++++- .../Services/ITeamExplorerContext.cs | 2 +- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index 64e3c5e941..5ed475da15 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -12,8 +12,16 @@ namespace GitHub.Services { /// - /// This service uses reflection to access the IGitExt from Visual Studio 2015 and 2017. + /// This implementation listenes to IGitExt for ActiveRepositories property change events and fires + /// and events when appropriate. /// + /// + /// A is fired when a solution is opened in a new repository (or not in a repository). + /// A event is only fired when the current branch, head SHA or tracked SHA changes (not + /// on every IGitExt property change event). contains the active repository or null + /// if a solution is opened that isn't in a repository. No events are fired when the same solution is unloaded then + /// reloaded (e.g. when a .sln file is touched). + /// [Export(typeof(ITeamExplorerContext))] [PartCreationPolicy(CreationPolicy.Shared)] public class TeamExplorerContext : ITeamExplorerContext @@ -76,6 +84,9 @@ public TeamExplorerContext(ILogger log, IService service, string gitExtTypeName, notifyPropertyChanged.PropertyChanged += (s, e) => Refresh(gitExt); } + /// + /// Used for unit testing. + /// public interface IService { string FindTrackedSha(string repositoryPath); @@ -189,12 +200,22 @@ public RepositoryInfo(string repositoryPath, string branchName, string headSha) public string HeadSha { get; } } + /// + /// The active repository or null if not in a repository. + /// public ILocalRepositoryModel ActiveRepository { get; private set; } + /// + /// Fired when a solution is opened in a new repository (or that isn't in a repository). + /// public event PropertyChangedEventHandler PropertyChanged; + + /// + /// Fired when the current branch, head SHA or tracked SHA changes. + /// public event EventHandler StatusChanged; } } diff --git a/src/GitHub.Exports/Services/ITeamExplorerContext.cs b/src/GitHub.Exports/Services/ITeamExplorerContext.cs index 47f3cc46d3..d90c64e867 100644 --- a/src/GitHub.Exports/Services/ITeamExplorerContext.cs +++ b/src/GitHub.Exports/Services/ITeamExplorerContext.cs @@ -9,7 +9,7 @@ namespace GitHub.Services /// /// /// A event is fired when moving to a new repository. - /// A event is fired when the CurrentBranch or HeadSha changes. + /// A event is fired when the current branch, head SHA or tracked SHA changes. /// public interface ITeamExplorerContext : INotifyPropertyChanged { From 218ba5a36d883b0e18f4512ce445e565f26e805c Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Jan 2018 15:05:04 +0000 Subject: [PATCH 36/64] Rename RefreshLater to RefreshIfActive --- .../ViewModels/GitHubPane/PullRequestDetailViewModel.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs b/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs index 773ad33690..c521c2227f 100644 --- a/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs +++ b/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs @@ -326,7 +326,7 @@ public async Task InitializeAsync( modelService = await modelServiceFactory.CreateAsync(connection); await Refresh(); - teamExplorerContext.StatusChanged += RefreshLater; + teamExplorerContext.StatusChanged += RefreshIfActive; } finally { @@ -334,7 +334,7 @@ public async Task InitializeAsync( } } - void RefreshLater(object sender, EventArgs e) + void RefreshIfActive(object sender, EventArgs e) { if (active) { @@ -531,7 +531,7 @@ protected override void Dispose(bool disposing) if (disposing) { - teamExplorerContext.StatusChanged -= RefreshLater; + teamExplorerContext.StatusChanged -= RefreshIfActive; } } From fd3077e30973d77fe601a5a5466e93bcb629e960 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Jan 2018 15:17:07 +0000 Subject: [PATCH 37/64] Consistently use CreateTeamExplorerContext --- .../GitHubPane/GitHubPaneViewModelTests.cs | 59 ++++++++++--------- 1 file changed, 31 insertions(+), 28 deletions(-) diff --git a/test/UnitTests/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModelTests.cs b/test/UnitTests/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModelTests.cs index e252fb9e75..3ebc41458b 100644 --- a/test/UnitTests/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModelTests.cs +++ b/test/UnitTests/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModelTests.cs @@ -23,26 +23,26 @@ public class TheInitializeMethod [Test] public async Task NotAGitRepositoryShownWhenNoRepository() { - var te = Substitute.For(); + var te = CreateTeamExplorerContext(); te.ActiveRepository.Returns(default(ILocalRepositoryModel)); var target = CreateTarget(teamExplorerContext: te); await Initialize(target); - Assert.That(target.Content, Is.InstanceOf()); - } + Assert.That(target.Content, Is.InstanceOf()); + } [Test] public async Task NotAGitHubRepositoryShownWhenRepositoryCloneUrlIsNull() { var repo = Substitute.For(); - var te = CreateTeamExplorerContext(null); + var te = CreateTeamExplorerContext(); var target = CreateTarget(teamExplorerContext: te); await Initialize(target); - Assert.That(target.Content, Is.InstanceOf()); - } + Assert.That(target.Content, Is.InstanceOf()); + } [Test] public async Task NotAGitHubRepositoryShownWhenRepositoryIsNotAGitHubInstance() @@ -52,8 +52,8 @@ public async Task NotAGitHubRepositoryShownWhenRepositoryIsNotAGitHubInstance() await Initialize(target); - Assert.That(target.Content, Is.InstanceOf()); - } + Assert.That(target.Content, Is.InstanceOf()); + } [Test] public async Task NotAGitHubRepositoryShownWhenRepositoryIsADeletedGitHubRepo() @@ -63,8 +63,8 @@ public async Task NotAGitHubRepositoryShownWhenRepositoryIsADeletedGitHubRepo() await Initialize(target); - Assert.That(target.Content, Is.InstanceOf()); - } + Assert.That(target.Content, Is.InstanceOf()); + } [Test] public async Task LoggedOutShownWhenNotLoggedInToGitHub() @@ -75,8 +75,8 @@ public async Task LoggedOutShownWhenNotLoggedInToGitHub() await Initialize(target); - Assert.That(target.Content, Is.InstanceOf()); - } + Assert.That(target.Content, Is.InstanceOf()); + } [Test] public async Task LoggedOutShownWhenNotLoggedInToEnterprise() @@ -87,8 +87,8 @@ public async Task LoggedOutShownWhenNotLoggedInToEnterprise() await Initialize(target); - Assert.That(target.Content, Is.InstanceOf()); - } + Assert.That(target.Content, Is.InstanceOf()); + } [Test] public async Task NavigatorShownWhenRepositoryIsAGitHubRepo() @@ -99,8 +99,8 @@ public async Task NavigatorShownWhenRepositoryIsAGitHubRepo() await Initialize(target); - Assert.That(target.Content, Is.InstanceOf()); - } + Assert.That(target.Content, Is.InstanceOf()); + } [Test] public async Task NavigatorShownWhenRepositoryIsAnEnterpriseRepo() @@ -111,8 +111,8 @@ public async Task NavigatorShownWhenRepositoryIsAnEnterpriseRepo() await Initialize(target); - Assert.That(target.Content, Is.InstanceOf()); - } + Assert.That(target.Content, Is.InstanceOf()); + } [Test] public async Task NavigatorShownWhenUserLogsIn() @@ -123,12 +123,12 @@ public async Task NavigatorShownWhenUserLogsIn() await Initialize(target); - Assert.That(target.Content, Is.InstanceOf()); + Assert.That(target.Content, Is.InstanceOf()); - AddConnection(cm, "https://github.com"); + AddConnection(cm, "https://github.com"); - Assert.That(target.Content, Is.InstanceOf()); - } + Assert.That(target.Content, Is.InstanceOf()); + } } public class TheShowPullRequestsMethod @@ -144,9 +144,9 @@ public async Task HasNoEffectWhenUserLoggedOut() teamExplorerContext: te); await Initialize(target); - Assert.That(target.Content, Is.InstanceOf()); + Assert.That(target.Content, Is.InstanceOf()); - await target.ShowPullRequests(); + await target.ShowPullRequests(); viewModelFactory.DidNotReceive().CreateViewModel(); } @@ -164,9 +164,9 @@ public async Task HasNoEffectWhenAlreadyCurrentPage() await Initialize(target); Assert.That(nav, Is.SameAs(target.Content)); - Assert.That(nav.Content, Is.InstanceOf()); + Assert.That(nav.Content, Is.InstanceOf()); - await target.ShowPullRequests(); + await target.ShowPullRequests(); Assert.That(1, Is.EqualTo(nav.History.Count)); } @@ -264,10 +264,13 @@ static INavigationViewModel CreateNavigator() return result; } - static ITeamExplorerContext CreateTeamExplorerContext(string repositoryCloneUrl) + static ITeamExplorerContext CreateTeamExplorerContext(string repositoryCloneUrl = null) { var repository = Substitute.For(); - repository.CloneUrl.Returns(new UriString(repositoryCloneUrl)); + if (repositoryCloneUrl != null) + { + repository.CloneUrl.Returns(new UriString(repositoryCloneUrl)); + } var result = Substitute.For(); result.ActiveRepository.Returns(repository); From 67a7c16983296753b9ab102bd471638112010bbe Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Jan 2018 16:48:25 +0000 Subject: [PATCH 38/64] Fire property changed event from ActiveRepository setter --- src/GitHub.App/Services/TeamExplorerContext.cs | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index 5ed475da15..bf2d39630b 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -37,6 +37,8 @@ public class TeamExplorerContext : ITeamExplorerContext string headSha; string trackedSha; + ILocalRepositoryModel repositoryModel; + readonly IService service = new Service(); [ImportingConstructor] @@ -130,7 +132,6 @@ void Refresh(object gitExt) { log.Information("Fire PropertyChanged event for ActiveRepository"); ActiveRepository = repo != null ? service.CreateRepository(repo.RepositoryPath) : null; - PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(ActiveRepository))); } else if (newBranchName != branchName) { @@ -205,7 +206,19 @@ public RepositoryInfo(string repositoryPath, string branchName, string headSha) /// public ILocalRepositoryModel ActiveRepository { - get; private set; + get + { + return repositoryModel; + } + + private set + { + if (value != repositoryModel) + { + repositoryModel = value; + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(ActiveRepository))); + } + } } /// From 687db8827df8e6c188c0f937b4360c531e570ae5 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Jan 2018 19:02:52 +0000 Subject: [PATCH 39/64] Encapsulate reflection code in GitExt and RepositoryInfo --- .../Services/TeamExplorerContext.cs | 56 +++++++++---------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index bf2d39630b..6c22e4c73f 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -30,6 +30,8 @@ public class TeamExplorerContext : ITeamExplorerContext readonly ILogger log; readonly DTE dte; + readonly IService service; + readonly GitExt gitExt; string solutionPath; string repositoryPath; @@ -39,7 +41,6 @@ public class TeamExplorerContext : ITeamExplorerContext ILocalRepositoryModel repositoryModel; - readonly IService service = new Service(); [ImportingConstructor] public TeamExplorerContext(IGitHubServiceProvider serviceProvider) @@ -68,13 +69,15 @@ public TeamExplorerContext(ILogger log, IService service, string gitExtTypeName, return; } + this.gitExt = new GitExt(gitExt); + dte = serviceProvider.TryGetService(); if (dte == null) { log.Error("Couldn't find service for type {DteType}", typeof(DTE)); } - Refresh(gitExt); + Refresh(); var notifyPropertyChanged = gitExt as INotifyPropertyChanged; if (notifyPropertyChanged == null) @@ -83,7 +86,7 @@ public TeamExplorerContext(ILogger log, IService service, string gitExtTypeName, return; } - notifyPropertyChanged.PropertyChanged += (s, e) => Refresh(gitExt); + notifyPropertyChanged.PropertyChanged += (s, e) => Refresh(); } /// @@ -108,11 +111,11 @@ public string FindTrackedSha(string repositoryPath) public ILocalRepositoryModel CreateRepository(string path) => new LocalRepositoryModel(path); } - void Refresh(object gitExt) + void Refresh() { try { - var repo = FindActiveRepository(gitExt); + var repo = gitExt.ActiveRepository; var newSolutionPath = dte?.Solution?.FullName; if (repo == null && newSolutionPath == solutionPath) @@ -162,38 +165,35 @@ void Refresh(object gitExt) } } - static RepositoryInfo FindActiveRepository(object gitExt) + class GitExt { - var activeRepositoriesProperty = gitExt.GetType().GetProperty("ActiveRepositories"); - var activeRepositories = (IEnumerable)activeRepositoriesProperty?.GetValue(gitExt); - var repo = activeRepositories?.FirstOrDefault(); - if (repo == null) + readonly object gitExt; + + internal GitExt(object gitExt) { - return null; + this.gitExt = gitExt; } - var repositoryPathProperty = repo.GetType().GetProperty("RepositoryPath"); - var repositoryPath = (string)repositoryPathProperty?.GetValue(repo); - - var currentBranchProperty = repo.GetType().GetProperty("CurrentBranch"); - var currentBranch = currentBranchProperty?.GetValue(repo); - - var headShaProperty = currentBranch?.GetType().GetProperty("HeadSha"); - var headSha = (string)headShaProperty?.GetValue(currentBranch); - - var nameProperty = currentBranch?.GetType().GetProperty("Name"); - var branchName = (string)nameProperty?.GetValue(currentBranch); - - return new RepositoryInfo(repositoryPath, branchName, headSha); + internal RepositoryInfo ActiveRepository + { + get + { + var activeRepositories = (IEnumerable)gitExt.GetType().GetProperty("ActiveRepositories")?.GetValue(gitExt); + var repo = activeRepositories?.FirstOrDefault(); + return repo != null ? new RepositoryInfo(repo) : null; + } + } } class RepositoryInfo { - public RepositoryInfo(string repositoryPath, string branchName, string headSha) + internal RepositoryInfo(object repo) { - RepositoryPath = repositoryPath; - BranchName = branchName; - HeadSha = headSha; + // Only called a handful times per session so not caching reflection calls. + var currentBranch = repo.GetType().GetProperty("CurrentBranch")?.GetValue(repo); + RepositoryPath = (string)repo.GetType().GetProperty("RepositoryPath")?.GetValue(repo); + BranchName = (string)currentBranch?.GetType().GetProperty("Name")?.GetValue(currentBranch); + HeadSha = (string)currentBranch?.GetType().GetProperty("HeadSha")?.GetValue(currentBranch); } public string RepositoryPath { get; } From f2678a1adb32f298024dfd005e36c22726c5dd06 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Jan 2018 20:55:30 +0000 Subject: [PATCH 40/64] Make teamExplorerContext default to ValidGitHubRepo --- .../GitHubPane/GitHubPaneViewModelTests.cs | 29 ++++++------------- 1 file changed, 9 insertions(+), 20 deletions(-) diff --git a/test/UnitTests/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModelTests.cs b/test/UnitTests/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModelTests.cs index 3ebc41458b..974e35f5a5 100644 --- a/test/UnitTests/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModelTests.cs +++ b/test/UnitTests/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModelTests.cs @@ -23,8 +23,8 @@ public class TheInitializeMethod [Test] public async Task NotAGitRepositoryShownWhenNoRepository() { - var te = CreateTeamExplorerContext(); - te.ActiveRepository.Returns(default(ILocalRepositoryModel)); + var te = Substitute.For(); + te.ActiveRepository.Returns(null as ILocalRepositoryModel); var target = CreateTarget(teamExplorerContext: te); await Initialize(target); @@ -35,8 +35,7 @@ public async Task NotAGitRepositoryShownWhenNoRepository() [Test] public async Task NotAGitHubRepositoryShownWhenRepositoryCloneUrlIsNull() { - var repo = Substitute.For(); - var te = CreateTeamExplorerContext(); + var te = CreateTeamExplorerContext(null); var target = CreateTarget(teamExplorerContext: te); await Initialize(target); @@ -93,9 +92,8 @@ public async Task LoggedOutShownWhenNotLoggedInToEnterprise() [Test] public async Task NavigatorShownWhenRepositoryIsAGitHubRepo() { - var te = CreateTeamExplorerContext(ValidGitHubRepo); var cm = CreateConnectionManager("https://github.com"); - var target = CreateTarget(teamExplorerContext: te, connectionManager: cm); + var target = CreateTarget(connectionManager: cm); await Initialize(target); @@ -117,9 +115,8 @@ public async Task NavigatorShownWhenRepositoryIsAnEnterpriseRepo() [Test] public async Task NavigatorShownWhenUserLogsIn() { - var te = CreateTeamExplorerContext(ValidGitHubRepo); var cm = CreateConnectionManager(); - var target = CreateTarget(teamExplorerContext: te, connectionManager: cm); + var target = CreateTarget(connectionManager: cm); await Initialize(target); @@ -136,12 +133,10 @@ public class TheShowPullRequestsMethod [Test] public async Task HasNoEffectWhenUserLoggedOut() { - var te = CreateTeamExplorerContext(ValidGitHubRepo); var viewModelFactory = Substitute.For(); var target = CreateTarget( viewModelFactory: viewModelFactory, - connectionManager: CreateConnectionManager(), - teamExplorerContext: te); + connectionManager: CreateConnectionManager()); await Initialize(target); Assert.That(target.Content, Is.InstanceOf()); @@ -154,11 +149,9 @@ public async Task HasNoEffectWhenUserLoggedOut() [Test] public async Task HasNoEffectWhenAlreadyCurrentPage() { - var te = CreateTeamExplorerContext(ValidGitHubRepo); var cm = CreateConnectionManager(ValidGitHubRepo); var nav = new NavigationViewModel(); var target = CreateTarget( - teamExplorerContext: te, connectionManager: cm, navigator: nav); @@ -186,7 +179,7 @@ static GitHubPaneViewModel CreateTarget( { viewModelFactory = viewModelFactory ?? Substitute.For(); connectionManager = connectionManager ?? Substitute.For(); - teamExplorerContext = teamExplorerContext ?? Substitute.For(); + teamExplorerContext = teamExplorerContext ?? CreateTeamExplorerContext(ValidGitHubRepo); browser = browser ?? Substitute.For(); usageTracker = usageTracker ?? Substitute.For(); loggedOut = loggedOut ?? Substitute.For(); @@ -264,14 +257,10 @@ static INavigationViewModel CreateNavigator() return result; } - static ITeamExplorerContext CreateTeamExplorerContext(string repositoryCloneUrl = null) + static ITeamExplorerContext CreateTeamExplorerContext(string repositoryCloneUrl) { var repository = Substitute.For(); - if (repositoryCloneUrl != null) - { - repository.CloneUrl.Returns(new UriString(repositoryCloneUrl)); - } - + repository.CloneUrl.Returns(new UriString(repositoryCloneUrl)); var result = Substitute.For(); result.ActiveRepository.Returns(repository); return result; From 5499f258561500a9ff0d945ce11c2d709f8e155e Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 24 Jan 2018 17:48:51 +0000 Subject: [PATCH 41/64] Use IVSGitExt rather than access IGitExt directly Add `Sha` and `TrackedSha` properties to `IBranch`. Remove nested `IService` interface. --- .../Services/TeamExplorerContext.cs | 116 ++----------- src/GitHub.Exports/Models/BranchModel.cs | 5 +- src/GitHub.Exports/Models/IBranch.cs | 2 + .../Services/TeamExplorerContextTests.cs | 162 ++++++++---------- 4 files changed, 89 insertions(+), 196 deletions(-) diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index 6c22e4c73f..a860f81450 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -2,12 +2,10 @@ using System.Linq; using System.ComponentModel; using System.ComponentModel.Composition; -using System.Collections.Generic; using GitHub.Models; using GitHub.Logging; using Serilog; using EnvDTE; -using LibGit2Sharp; namespace GitHub.Services { @@ -26,12 +24,9 @@ namespace GitHub.Services [PartCreationPolicy(CreationPolicy.Shared)] public class TeamExplorerContext : ITeamExplorerContext { - const string GitExtTypeName = "Microsoft.VisualStudio.TeamFoundation.Git.Extensibility.IGitExt, Microsoft.TeamFoundation.Git.Provider"; - readonly ILogger log; readonly DTE dte; - readonly IService service; - readonly GitExt gitExt; + readonly IVSGitExt gitExt; string solutionPath; string repositoryPath; @@ -41,35 +36,16 @@ public class TeamExplorerContext : ITeamExplorerContext ILocalRepositoryModel repositoryModel; - [ImportingConstructor] - public TeamExplorerContext(IGitHubServiceProvider serviceProvider) - : this(LogManager.ForContext(), new Service(), GitExtTypeName, serviceProvider) + public TeamExplorerContext(IGitHubServiceProvider serviceProvider, IVSGitExt gitExt) + : this(LogManager.ForContext(), gitExt, serviceProvider) { } - public TeamExplorerContext(ILogger log, IService service, string gitExtTypeName, IGitHubServiceProvider serviceProvider) + public TeamExplorerContext(ILogger log, IVSGitExt gitExt, IGitHubServiceProvider serviceProvider) { this.log = log; - this.service = service; - - // Visual Studio 2015 and 2017 use different versions of the Microsoft.TeamFoundation.Git.Provider assembly. - // There are no binding redirections between them, but the package that includes them defines a ProvideBindingPath - // attrubute. This means the required IGitExt type can be found using an unqualified assembly name (GitExtTypeName). - var gitExtType = Type.GetType(gitExtTypeName, false); - if (gitExtType == null) - { - log.Error("Couldn't find type {GitExtTypeName}", gitExtTypeName); - } - - var gitExt = serviceProvider.GetService(gitExtType); - if (gitExt == null) - { - log.Error("Couldn't find service for type {GitExtType}", gitExtType); - return; - } - - this.gitExt = new GitExt(gitExt); + this.gitExt = gitExt; dte = serviceProvider.TryGetService(); if (dte == null) @@ -77,45 +53,17 @@ public TeamExplorerContext(ILogger log, IService service, string gitExtTypeName, log.Error("Couldn't find service for type {DteType}", typeof(DTE)); } - Refresh(); - - var notifyPropertyChanged = gitExt as INotifyPropertyChanged; - if (notifyPropertyChanged == null) - { - log.Error("The service {ServiceObject} doesn't implement {Interface}", gitExt, typeof(INotifyPropertyChanged)); - return; - } - - notifyPropertyChanged.PropertyChanged += (s, e) => Refresh(); - } - - /// - /// Used for unit testing. - /// - public interface IService - { - string FindTrackedSha(string repositoryPath); - ILocalRepositoryModel CreateRepository(string path); - } - - class Service : IService - { - public string FindTrackedSha(string repositoryPath) - { - using (var repo = new Repository(repositoryPath)) - { - return repo.Head.TrackedBranch?.Tip.Sha; - } - } + gitExt.Refresh(serviceProvider); - public ILocalRepositoryModel CreateRepository(string path) => new LocalRepositoryModel(path); + Refresh(); + gitExt.ActiveRepositoriesChanged += Refresh; } void Refresh() { try { - var repo = gitExt.ActiveRepository; + var repo = gitExt.ActiveRepositories?.FirstOrDefault(); var newSolutionPath = dte?.Solution?.FullName; if (repo == null && newSolutionPath == solutionPath) @@ -126,15 +74,15 @@ void Refresh() } else { - var newRepositoryPath = repo?.RepositoryPath; - var newBranchName = repo?.BranchName; - var newHeadSha = repo?.HeadSha; - var newTrackedSha = newRepositoryPath != null ? service.FindTrackedSha(newRepositoryPath) : null; + var newRepositoryPath = repo?.LocalPath; + var newBranchName = repo?.CurrentBranch?.Name; + var newHeadSha = repo?.CurrentBranch?.Sha; + var newTrackedSha = repo?.CurrentBranch?.TrackedSha; if (newRepositoryPath != repositoryPath) { log.Information("Fire PropertyChanged event for ActiveRepository"); - ActiveRepository = repo != null ? service.CreateRepository(repo.RepositoryPath) : null; + ActiveRepository = repo; } else if (newBranchName != branchName) { @@ -165,42 +113,6 @@ void Refresh() } } - class GitExt - { - readonly object gitExt; - - internal GitExt(object gitExt) - { - this.gitExt = gitExt; - } - - internal RepositoryInfo ActiveRepository - { - get - { - var activeRepositories = (IEnumerable)gitExt.GetType().GetProperty("ActiveRepositories")?.GetValue(gitExt); - var repo = activeRepositories?.FirstOrDefault(); - return repo != null ? new RepositoryInfo(repo) : null; - } - } - } - - class RepositoryInfo - { - internal RepositoryInfo(object repo) - { - // Only called a handful times per session so not caching reflection calls. - var currentBranch = repo.GetType().GetProperty("CurrentBranch")?.GetValue(repo); - RepositoryPath = (string)repo.GetType().GetProperty("RepositoryPath")?.GetValue(repo); - BranchName = (string)currentBranch?.GetType().GetProperty("Name")?.GetValue(currentBranch); - HeadSha = (string)currentBranch?.GetType().GetProperty("HeadSha")?.GetValue(currentBranch); - } - - public string RepositoryPath { get; } - public string BranchName { get; } - public string HeadSha { get; } - } - /// /// The active repository or null if not in a repository. /// diff --git a/src/GitHub.Exports/Models/BranchModel.cs b/src/GitHub.Exports/Models/BranchModel.cs index c352cd1abb..9e7c3d37c9 100644 --- a/src/GitHub.Exports/Models/BranchModel.cs +++ b/src/GitHub.Exports/Models/BranchModel.cs @@ -34,6 +34,8 @@ public BranchModel(LibGit2Sharp.Branch branch, IRepositoryModel repo) Repository = branch.IsRemote ? new LocalRepositoryModel(branch.Remote.Url) : repo; #pragma warning restore 0618 IsTracking = branch.IsTracking; + Sha = branch.Tip?.Sha; + TrackedSha = branch.TrackedBranch?.Tip?.Sha; Id = String.Format(CultureInfo.InvariantCulture, "{0}/{1}", Repository.Owner, Name); } @@ -42,7 +44,8 @@ public BranchModel(LibGit2Sharp.Branch branch, IRepositoryModel repo) public IRepositoryModel Repository { get; private set; } public bool IsTracking { get; private set; } public string DisplayName { get; set; } - + public string Sha { get; private set; } + public string TrackedSha { get; private set; } #region Equality things public void CopyFrom(IBranch other) diff --git a/src/GitHub.Exports/Models/IBranch.cs b/src/GitHub.Exports/Models/IBranch.cs index 52d48bfc77..5e7059391e 100644 --- a/src/GitHub.Exports/Models/IBranch.cs +++ b/src/GitHub.Exports/Models/IBranch.cs @@ -11,5 +11,7 @@ public interface IBranch : ICopyable, IRepositoryModel Repository { get; } bool IsTracking { get; } string DisplayName { get; set; } + string Sha { get; } + string TrackedSha { get; } } } diff --git a/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs b/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs index d699e6f38e..e0f696416b 100644 --- a/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs +++ b/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs @@ -1,7 +1,5 @@ using System; using System.IO; -using System.ComponentModel; -using System.Collections.Generic; using GitHub.Services; using NUnit.Framework; using NSubstitute; @@ -24,7 +22,7 @@ public void SetUp() [Test] public void NoActiveRepository() { - var gitExt = new FakeGitExt(); + var gitExt = CreateGitExt(); var target = CreateTeamExplorerContext(gitExt); var repo = target.ActiveRepository; @@ -33,36 +31,33 @@ public void NoActiveRepository() } [Test] - public void SetActiveRepository() + public void SetActiveRepository_CheckWasSet() { - var gitExt = new FakeGitExt(); + var gitExt = CreateGitExt(); var repositoryPath = Directory.GetCurrentDirectory(); - var repoInfo = new GitRepositoryInfo(repositoryPath, null); - gitExt.SetActiveRepository(repoInfo); - var service = Substitute.For(); - var expectRepo = Substitute.For(); - service.CreateRepository(repositoryPath).Returns(expectRepo); - var target = CreateTeamExplorerContext(gitExt, service: service); + var repoInfo = CreateRepositoryModel(repositoryPath); + SetActiveRepository(gitExt, repoInfo); + var target = CreateTeamExplorerContext(gitExt); var repo = target.ActiveRepository; - Assert.That(repo, Is.EqualTo(expectRepo)); + Assert.That(repo, Is.EqualTo(repoInfo)); } } public class ThePropertyChangedEvent { [Test] - public void SetActiveRepository() + public void SetActiveRepository_CheckEventWasRaised() { - var gitExt = new FakeGitExt(); + var gitExt = CreateGitExt(); var repositoryPath = Directory.GetCurrentDirectory(); - var repoInfo = new GitRepositoryInfo(repositoryPath, null); + var repoInfo = CreateRepositoryModel(repositoryPath); var target = CreateTeamExplorerContext(gitExt); var eventWasRaised = false; target.PropertyChanged += (s, e) => eventWasRaised = e.PropertyName == nameof(target.ActiveRepository); - gitExt.SetActiveRepository(repoInfo); + SetActiveRepository(gitExt, repoInfo); Assert.That(eventWasRaised, Is.True); } @@ -70,15 +65,15 @@ public void SetActiveRepository() [Test] public void SetTwicePropertyChangedFiresOnce() { - var gitExt = new FakeGitExt(); + var gitExt = CreateGitExt(); var repositoryPath = Directory.GetCurrentDirectory(); - var repoInfo = new GitRepositoryInfo(repositoryPath, null); + var repoInfo = CreateRepositoryModel(repositoryPath); var target = CreateTeamExplorerContext(gitExt); var eventWasRaisedCount = 0; target.PropertyChanged += (s, e) => eventWasRaisedCount++; - gitExt.SetActiveRepository(repoInfo); - gitExt.SetActiveRepository(repoInfo); + SetActiveRepository(gitExt, repoInfo); + SetActiveRepository(gitExt, repoInfo); Assert.That(1, Is.EqualTo(1)); } @@ -86,17 +81,17 @@ public void SetTwicePropertyChangedFiresOnce() [Test] public void ChangeActiveRepository_NoSolutionChange() { - var gitExt = new FakeGitExt(); + var gitExt = CreateGitExt(); var repositoryPath = Directory.GetCurrentDirectory(); - var repoInfo = new GitRepositoryInfo(repositoryPath, null); + var repoInfo = CreateRepositoryModel(repositoryPath); var repositoryPath2 = Path.GetTempPath(); - var repoInfo2 = new GitRepositoryInfo(repositoryPath2, null); + var repoInfo2 = CreateRepositoryModel(repositoryPath2); var target = CreateTeamExplorerContext(gitExt); - gitExt.SetActiveRepository(repoInfo); + SetActiveRepository(gitExt, repoInfo); var eventWasRaised = false; target.PropertyChanged += (s, e) => eventWasRaised = e.PropertyName == nameof(target.ActiveRepository); - gitExt.SetActiveRepository(repoInfo2); + SetActiveRepository(gitExt, repoInfo2); Assert.That(eventWasRaised, Is.True); } @@ -104,38 +99,35 @@ public void ChangeActiveRepository_NoSolutionChange() [Test] public void ClearActiveRepository_NoEventWhenNoSolutionChange() { - var gitExt = new FakeGitExt(); + var gitExt = CreateGitExt(); var repositoryPath = Directory.GetCurrentDirectory(); - var repoInfo = new GitRepositoryInfo(repositoryPath, null); - var service = Substitute.For(); - var expectRepo = Substitute.For(); - service.CreateRepository(repositoryPath).Returns(expectRepo); - var target = CreateTeamExplorerContext(gitExt, service: service); - gitExt.SetActiveRepository(repoInfo); + var repoInfo = CreateRepositoryModel(repositoryPath); + var target = CreateTeamExplorerContext(gitExt); + SetActiveRepository(gitExt, repoInfo); var eventWasRaised = false; target.PropertyChanged += (s, e) => eventWasRaised = e.PropertyName == nameof(target.ActiveRepository); - gitExt.SetActiveRepository(null); + SetActiveRepository(gitExt, null); Assert.That(eventWasRaised, Is.False); - Assert.That(target.ActiveRepository, Is.EqualTo(expectRepo)); + Assert.That(target.ActiveRepository, Is.EqualTo(repoInfo)); } [Test] public void ClearActiveRepository_FireWhenSolutionChanged() { - var gitExt = new FakeGitExt(); + var gitExt = CreateGitExt(); var repositoryPath = Directory.GetCurrentDirectory(); - var repoInfo = new GitRepositoryInfo(repositoryPath, null); + var repoInfo = CreateRepositoryModel(repositoryPath); var dte = Substitute.For(); var target = CreateTeamExplorerContext(gitExt, dte); dte.Solution.FullName.Returns("Solution1"); - gitExt.SetActiveRepository(repoInfo); + SetActiveRepository(gitExt, repoInfo); var eventWasRaised = false; target.PropertyChanged += (s, e) => eventWasRaised = e.PropertyName == nameof(target.ActiveRepository); dte.Solution.FullName.Returns("Solution2"); - gitExt.SetActiveRepository(null); + SetActiveRepository(gitExt, null); Assert.That(eventWasRaised, Is.True); Assert.That(target.ActiveRepository, Is.Null); @@ -144,18 +136,18 @@ public void ClearActiveRepository_FireWhenSolutionChanged() [Test] public void NoActiveRepositoryChange_SolutionChanges() { - var gitExt = new FakeGitExt(); + var gitExt = CreateGitExt(); var repositoryPath = Directory.GetCurrentDirectory(); - var repoInfo = new GitRepositoryInfo(repositoryPath, null); + var repoInfo = CreateRepositoryModel(repositoryPath); var dte = Substitute.For(); var target = CreateTeamExplorerContext(gitExt, dte); dte.Solution.FullName.Returns(""); - gitExt.SetActiveRepository(repoInfo); + SetActiveRepository(gitExt, repoInfo); var eventWasRaised = false; target.PropertyChanged += (s, e) => eventWasRaised = e.PropertyName == nameof(target.ActiveRepository); dte.Solution.FullName.Returns("Solution"); - gitExt.SetActiveRepository(repoInfo); + SetActiveRepository(gitExt, repoInfo); Assert.That(eventWasRaised, Is.False); } @@ -171,18 +163,19 @@ public class TheStatusChangedEvent [TestCase(true, "name1", "sha1", "name2", "sha2", false)] public void SameActiveRepository_ExpectWasRaised(bool changePath, string name1, string sha1, string name2, string sha2, bool expectWasRaised) { - var gitExt = new FakeGitExt(); + var gitExt = CreateGitExt(); var repositoryPaths = new[] { Directory.GetCurrentDirectory(), Path.GetTempPath() }; var path1 = Directory.GetCurrentDirectory(); var path2 = changePath ? Path.GetTempPath() : path1; - var repoInfo1 = new GitRepositoryInfo(path1, new GitBranchInfo(name1, sha1)); - var repoInfo2 = new GitRepositoryInfo(path2, new GitBranchInfo(name2, sha2)); + var repoInfo1 = CreateRepositoryModel(path1, name1, sha1); + var repoInfo2 = CreateRepositoryModel(path2, name2, sha2); + var target = CreateTeamExplorerContext(gitExt); var eventWasRaised = false; target.StatusChanged += (s, e) => eventWasRaised = true; - gitExt.SetActiveRepository(repoInfo1); - gitExt.SetActiveRepository(repoInfo2); + SetActiveRepository(gitExt, repoInfo1); + SetActiveRepository(gitExt, repoInfo2); Assert.That(eventWasRaised, Is.EqualTo(expectWasRaised)); } @@ -191,18 +184,17 @@ public void SameActiveRepository_ExpectWasRaised(bool changePath, string name1, [TestCase("trackedSha1", "trackedSha2", true)] public void TrackedShaChanges_CheckWasRaised(string trackedSha1, string trackedSha2, bool expectWasRaised) { - var gitExt = new FakeGitExt(); + var gitExt = CreateGitExt(); var repositoryPaths = new[] { Directory.GetCurrentDirectory(), Path.GetTempPath() }; var repoPath = Directory.GetCurrentDirectory(); - var repoInfo = new GitRepositoryInfo(repoPath, new GitBranchInfo("name", "sha")); - var service = Substitute.For(); - service.FindTrackedSha(Arg.Any()).Returns(trackedSha1, trackedSha2); - var target = CreateTeamExplorerContext(gitExt, service: service); - gitExt.SetActiveRepository(repoInfo); + var repoInfo1 = CreateRepositoryModel(repoPath, "name", "sha", trackedSha1); + var repoInfo2 = CreateRepositoryModel(repoPath, "name", "sha", trackedSha2); + var target = CreateTeamExplorerContext(gitExt); + SetActiveRepository(gitExt, repoInfo1); var eventWasRaised = false; target.StatusChanged += (s, e) => eventWasRaised = true; - gitExt.SetActiveRepository(repoInfo); + SetActiveRepository(gitExt, repoInfo2); Assert.That(eventWasRaised, Is.EqualTo(expectWasRaised)); } @@ -210,69 +202,53 @@ public void TrackedShaChanges_CheckWasRaised(string trackedSha1, string trackedS [Test] public void SolutionUnloadedAndReloaded_DontFireStatusChanged() { - var gitExt = new FakeGitExt(); + var gitExt = CreateGitExt(); var path = Directory.GetCurrentDirectory(); - var repoInfo1 = new GitRepositoryInfo(path, new GitBranchInfo("name", "sha")); - var repoInfo2 = new GitRepositoryInfo(null, new GitBranchInfo(null, null)); + var repoInfo1 = CreateRepositoryModel(path, "name", "sha"); + var repoInfo2 = CreateRepositoryModel(null); var target = CreateTeamExplorerContext(gitExt); - gitExt.SetActiveRepository(repoInfo1); - gitExt.SetActiveRepository(repoInfo2); + SetActiveRepository(gitExt, repoInfo1); + SetActiveRepository(gitExt, repoInfo2); var eventWasRaised = false; target.StatusChanged += (s, e) => eventWasRaised = true; - gitExt.SetActiveRepository(repoInfo1); + SetActiveRepository(gitExt, repoInfo1); Assert.That(eventWasRaised, Is.False); } } - static TeamExplorerContext CreateTeamExplorerContext(FakeGitExt gitExt, DTE dte = null, TeamExplorerContext.IService service = null) + static TeamExplorerContext CreateTeamExplorerContext(IVSGitExt gitExt, DTE dte = null) { - var gitExtType = typeof(FakeGitExt); dte = dte ?? Substitute.For(); - service = service ?? Substitute.For(); var sp = Substitute.For(); - sp.GetService(gitExtType).Returns(gitExt); sp.TryGetService().Returns(dte); var log = Substitute.For(); - return new TeamExplorerContext(log, service, gitExtType.AssemblyQualifiedName, sp); + return new TeamExplorerContext(log, gitExt, sp); } - class FakeGitExt : INotifyPropertyChanged + static ILocalRepositoryModel CreateRepositoryModel(string path, string branchName = null, string headSha = null, string trackedSha = null) { - public void SetActiveRepository(GitRepositoryInfo repo) - { - ActiveRepositories = repo != null ? new[] { repo } : new GitRepositoryInfo[0]; - PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(ActiveRepositories))); - } - - public IReadOnlyList ActiveRepositories { get; private set; } - - public event PropertyChangedEventHandler PropertyChanged; + var repo = Substitute.For(); + repo.LocalPath.Returns(path); + var currentBranch = Substitute.For(); + currentBranch.Name.Returns(branchName); + currentBranch.Sha.Returns(headSha); + currentBranch.TrackedSha.Returns(trackedSha); + repo.CurrentBranch.Returns(currentBranch); + return repo; } - class GitRepositoryInfo + static IVSGitExt CreateGitExt() { - public GitRepositoryInfo(string repositoryPath, GitBranchInfo currentBranch) - { - RepositoryPath = repositoryPath; - CurrentBranch = currentBranch; - } - - public string RepositoryPath { get; } - public GitBranchInfo CurrentBranch { get; } + return Substitute.For(); } - class GitBranchInfo + static void SetActiveRepository(IVSGitExt gitExt, ILocalRepositoryModel repo) { - public GitBranchInfo(string name, string headSha) - { - Name = name; - HeadSha = headSha; - } - - public string Name { get; } - public string HeadSha { get; } + var repos = repo != null ? new[] { repo } : new ILocalRepositoryModel[0]; + gitExt.ActiveRepositories.Returns(repos); + gitExt.ActiveRepositoriesChanged += Raise.Event(); } } } From 1bb4be9a11c9d716d9b138ed180489570105ee27 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Thu, 25 Jan 2018 09:21:09 +0000 Subject: [PATCH 42/64] Don't manually disable logging when unit testing This should be done as part of the logging framework. --- src/GitHub.App/Services/TeamExplorerContext.cs | 8 ++++---- .../GitHub.App/Services/TeamExplorerContextTests.cs | 10 +--------- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index a860f81450..608d4c85ec 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -24,7 +24,8 @@ namespace GitHub.Services [PartCreationPolicy(CreationPolicy.Shared)] public class TeamExplorerContext : ITeamExplorerContext { - readonly ILogger log; + static ILogger log = LogManager.ForContext(); + readonly DTE dte; readonly IVSGitExt gitExt; @@ -38,13 +39,12 @@ public class TeamExplorerContext : ITeamExplorerContext [ImportingConstructor] public TeamExplorerContext(IGitHubServiceProvider serviceProvider, IVSGitExt gitExt) - : this(LogManager.ForContext(), gitExt, serviceProvider) + : this(gitExt, serviceProvider) { } - public TeamExplorerContext(ILogger log, IVSGitExt gitExt, IGitHubServiceProvider serviceProvider) + public TeamExplorerContext(IVSGitExt gitExt, IGitHubServiceProvider serviceProvider) { - this.log = log; this.gitExt = gitExt; dte = serviceProvider.TryGetService(); diff --git a/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs b/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs index e0f696416b..c5f269b6e1 100644 --- a/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs +++ b/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs @@ -4,7 +4,6 @@ using NUnit.Framework; using NSubstitute; using EnvDTE; -using Serilog; using GitHub.Models; namespace GitHub.App.UnitTests.Services @@ -13,12 +12,6 @@ public class TeamExplorerContextTests { public class TheActiveRepositoryProperty { - [SetUp] - public void SetUp() - { - Splat.ModeDetector.Current.SetInUnitTestRunner(true); - } - [Test] public void NoActiveRepository() { @@ -223,8 +216,7 @@ static TeamExplorerContext CreateTeamExplorerContext(IVSGitExt gitExt, DTE dte = dte = dte ?? Substitute.For(); var sp = Substitute.For(); sp.TryGetService().Returns(dte); - var log = Substitute.For(); - return new TeamExplorerContext(log, gitExt, sp); + return new TeamExplorerContext(gitExt, sp); } static ILocalRepositoryModel CreateRepositoryModel(string path, string branchName = null, string headSha = null, string trackedSha = null) From 3f534697069f6968c575d0e30f0e8573986d02be Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Thu, 25 Jan 2018 09:35:13 +0000 Subject: [PATCH 43/64] Clean up and comment --- src/GitHub.App/Services/TeamExplorerContext.cs | 10 ++++------ .../GitHub.App/Services/TeamExplorerContextTests.cs | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index 608d4c85ec..9801408840 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -47,12 +47,10 @@ public TeamExplorerContext(IVSGitExt gitExt, IGitHubServiceProvider serviceProvi { this.gitExt = gitExt; - dte = serviceProvider.TryGetService(); - if (dte == null) - { - log.Error("Couldn't find service for type {DteType}", typeof(DTE)); - } + // This is a standard service which should always be available. + dte = serviceProvider.GetService(); + // HACK: In a future version of VSGitExt this hopefully won't be necessary. gitExt.Refresh(serviceProvider); Refresh(); @@ -64,7 +62,7 @@ void Refresh() try { var repo = gitExt.ActiveRepositories?.FirstOrDefault(); - var newSolutionPath = dte?.Solution?.FullName; + var newSolutionPath = dte.Solution?.FullName; if (repo == null && newSolutionPath == solutionPath) { diff --git a/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs b/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs index c5f269b6e1..95fee30b60 100644 --- a/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs +++ b/test/UnitTests/GitHub.App/Services/TeamExplorerContextTests.cs @@ -215,7 +215,7 @@ static TeamExplorerContext CreateTeamExplorerContext(IVSGitExt gitExt, DTE dte = { dte = dte ?? Substitute.For(); var sp = Substitute.For(); - sp.TryGetService().Returns(dte); + sp.GetService().Returns(dte); return new TeamExplorerContext(gitExt, sp); } From 375bdd65952be81050c3c1af630f2d905625b72c Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Thu, 25 Jan 2018 15:15:23 +0000 Subject: [PATCH 44/64] Change VSGitExt to lazy-initialize using UIContextChanged Removed `Refresh` hack from `TeamExplorerContext`. VSGitExt now self-initializes and doesn't depend on Refresh being called by TeamExplorerServiceHolder. --- .../Services/TeamExplorerContext.cs | 3 - src/GitHub.Exports/Services/IVSGitExt.cs | 1 - .../Base/TeamExplorerServiceHolder.cs | 5 +- .../Services/VSGitExt.cs | 60 +++++++++++++++---- 4 files changed, 51 insertions(+), 18 deletions(-) diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index 9801408840..0187c1e7e5 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -50,9 +50,6 @@ public TeamExplorerContext(IVSGitExt gitExt, IGitHubServiceProvider serviceProvi // This is a standard service which should always be available. dte = serviceProvider.GetService(); - // HACK: In a future version of VSGitExt this hopefully won't be necessary. - gitExt.Refresh(serviceProvider); - Refresh(); gitExt.ActiveRepositoriesChanged += Refresh; } diff --git a/src/GitHub.Exports/Services/IVSGitExt.cs b/src/GitHub.Exports/Services/IVSGitExt.cs index 0b1be63c95..86c7294173 100644 --- a/src/GitHub.Exports/Services/IVSGitExt.cs +++ b/src/GitHub.Exports/Services/IVSGitExt.cs @@ -8,7 +8,6 @@ namespace GitHub.Services { public interface IVSGitExt { - void Refresh(IServiceProvider serviceProvider); IEnumerable ActiveRepositories { get; } event Action ActiveRepositoriesChanged; } diff --git a/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs b/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs index eb0912372c..59470f4da4 100644 --- a/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs +++ b/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs @@ -181,7 +181,10 @@ async void UIContextChanged(bool active, bool refresh) if (repos == null) { log.Error("Error 2001: ActiveRepositories is null. GitService: '{GitService}'", GitService); - GitService.Refresh(ServiceProvider); + + // NOTE: VSGitExt is now self-refreshing + //GitService.Refresh(ServiceProvider); + repos = GitService.ActiveRepositories; if (repos == null) log.Error("Error 2002: ActiveRepositories is null. GitService: '{GitService}'", GitService); diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index a0498bdfd2..1bc308d392 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -1,10 +1,11 @@ using System; +using System.Linq; using System.Collections.Generic; using System.ComponentModel.Composition; -using System.Linq; -using GitHub.Extensions; using GitHub.Models; using GitHub.Services; +using GitHub.Logging; +using Serilog; using Microsoft.VisualStudio.TeamFoundation.Git.Extensibility; namespace GitHub.VisualStudio.Base @@ -13,24 +14,57 @@ namespace GitHub.VisualStudio.Base [PartCreationPolicy(CreationPolicy.Shared)] public class VSGitExt : IVSGitExt { + static readonly ILogger log = LogManager.ForContext(); + + IGitHubServiceProvider serviceProvider; + IVSUIContext context; IGitExt gitService; - public void Refresh(IServiceProvider serviceProvider) + [ImportingConstructor] + public VSGitExt(IGitHubServiceProvider serviceProvider) { - if (gitService != null) - gitService.PropertyChanged -= CheckAndUpdate; - gitService = serviceProvider.GetServiceSafe(); - if (gitService != null) - gitService.PropertyChanged += CheckAndUpdate; + this.serviceProvider = serviceProvider; + var factory = serviceProvider.GetService(); + + // The IGitExt service is only available when in the SccProvider context. + // This could be changed to VSConstants.UICONTEXT.SolutionExists_guid when testing. + context = factory.GetUIContext(new Guid(Guids.GitSccProviderId)); + + // If we're not in the GitSccProvider context or TryInitialize fails, have another go when the context changes. + if (!context.IsActive || !TryInitialize()) + { + context.UIContextChanged += ContextChanged; + } } + void ContextChanged(object sender, IVSUIContextChangedEventArgs e) + { + // If we're in the GitSccProvider context and TryInitialize succeeds, we can stop listening for events. + if (e.Activated && TryInitialize()) + { + context.UIContextChanged -= ContextChanged; + } + } - void CheckAndUpdate(object sender, System.ComponentModel.PropertyChangedEventArgs e) + bool TryInitialize() { - Guard.ArgumentNotNull(e, nameof(e)); - if (e.PropertyName != "ActiveRepositories" || gitService == null) - return; - ActiveRepositoriesChanged?.Invoke(); + gitService = serviceProvider.GetService(); + if (gitService != null) + { + // The IGitExt service is now available so let consumers know to read ActiveRepositories. + ActiveRepositoriesChanged?.Invoke(); + gitService.PropertyChanged += (s, e) => + { + if (e.PropertyName == nameof(gitService.ActiveRepositories)) + { + ActiveRepositoriesChanged?.Invoke(); + } + }; + + return true; + } + + return false; } public IEnumerable ActiveRepositories => gitService?.ActiveRepositories.Select(x => x.ToModel()); From 572760b3e9c691325fef6a2649d39f9eee59adf8 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Thu, 25 Jan 2018 20:33:31 +0000 Subject: [PATCH 45/64] Remove unused logger --- src/GitHub.TeamFoundation.14/Services/VSGitExt.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index 1bc308d392..b9b433dad0 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -14,8 +14,6 @@ namespace GitHub.VisualStudio.Base [PartCreationPolicy(CreationPolicy.Shared)] public class VSGitExt : IVSGitExt { - static readonly ILogger log = LogManager.ForContext(); - IGitHubServiceProvider serviceProvider; IVSUIContext context; IGitExt gitService; From 24d45b76471d8039df1ca27f6d6b356b64a02e37 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Thu, 25 Jan 2018 20:33:45 +0000 Subject: [PATCH 46/64] Add unit tests for VSGitExt --- .../GitHub.TeamFoundation/VSGitExtTests.cs | 151 ++++++++++++++++++ test/UnitTests/UnitTests.csproj | 2 + 2 files changed, 153 insertions(+) create mode 100644 test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs diff --git a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs new file mode 100644 index 0000000000..217392fa6e --- /dev/null +++ b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs @@ -0,0 +1,151 @@ +using System; +using System.Linq; +using System.ComponentModel; +using System.Collections.Generic; +using GitHub.Services; +using GitHub.VisualStudio; +using GitHub.VisualStudio.Base; +using NUnit.Framework; +using NSubstitute; +using Microsoft.VisualStudio.TeamFoundation.Git.Extensibility; + +public class VSGitExtTests +{ + public class TheConstructor + { + [TestCase(true, 1)] + [TestCase(false, 0)] + public void GetServiceIGitExt_WhenSccProviderContextIsActive(bool isActive, int expectCalls) + { + var context = CreateVSUIContext(isActive); + var sp = CreateGitHubServiceProvider(context); + + var target = new VSGitExt(sp); + + sp.Received(expectCalls).GetService(); + } + + [TestCase(true, 1)] + [TestCase(false, 0)] + public void GetServiceIGitExt_WhenUIContextChanged(bool activated, int expectCalls) + { + var context = CreateVSUIContext(false); + var sp = CreateGitHubServiceProvider(context); + + var target = new VSGitExt(sp); + var eventArgs = Substitute.For(); + eventArgs.Activated.Returns(activated); + context.UIContextChanged += Raise.Event>(context, eventArgs); + + sp.Received(expectCalls).GetService(); + } + } + + public class TheActiveRepositoriesChangedEvent + { + [Test] + public void GitExtPropertyChangedEvent_ActiveRepositoriesChangedIsFired() + { + var context = CreateVSUIContext(true); + var gitExt = CreateGitExt(); + var sp = CreateGitHubServiceProvider(context, gitExt); + var target = new VSGitExt(sp); + + bool wasFired = false; + target.ActiveRepositoriesChanged += () => wasFired = true; + var eventArgs = new PropertyChangedEventArgs(nameof(gitExt.ActiveRepositories)); + gitExt.PropertyChanged += Raise.Event(gitExt, eventArgs); + + Assert.That(wasFired, Is.True); + } + + [Test] + public void WhenUIContextChanged_ActiveRepositoriesChangedIsFired() + { + var context = CreateVSUIContext(false); + var gitExt = CreateGitExt(); + var sp = CreateGitHubServiceProvider(context, gitExt); + var target = new VSGitExt(sp); + + bool wasFired = false; + target.ActiveRepositoriesChanged += () => wasFired = true; + + var eventArgs = Substitute.For(); + eventArgs.Activated.Returns(true); + context.UIContextChanged += Raise.Event>(context, eventArgs); + + Assert.That(wasFired, Is.True); + } + } + + public class TheActiveRepositoriesProperty + { + [Test] + public void SccProviderContextNotActive_IsNull() + { + var context = CreateVSUIContext(false); + var sp = CreateGitHubServiceProvider(context); + + var target = new VSGitExt(sp); + + Assert.That(target.ActiveRepositories, Is.Null); + } + + [Test] + public void SccProviderContextIsActive_DefaultEmptyListFromGitExt() + { + var context = CreateVSUIContext(true); + var gitExt = CreateGitExt(new string[0]); + var sp = CreateGitHubServiceProvider(context, gitExt); + var target = new VSGitExt(sp); + + var activeRepositories = target.ActiveRepositories; + + Assert.That(activeRepositories.Count(), Is.EqualTo(0)); + } + + // TODO: We can't currently test returning a non-empty list because it constructs a live LocalRepositoryModel object. + // Move could move the responsibility for constructing a LocalRepositoryModel object outside of VSGitExt. + } + + static IVSUIContext CreateVSUIContext(bool isActive) + { + var context = Substitute.For(); + context.IsActive.Returns(isActive); + return context; + } + + static IGitExt CreateGitExt(IList repositoryPaths = null) + { + repositoryPaths = repositoryPaths ?? new string[0]; + var gitExt = Substitute.For(); + var repoList = CreateActiveRepositories(new string[0]); + gitExt.ActiveRepositories.Returns(repoList); + return gitExt; + } + + static IReadOnlyList CreateActiveRepositories(IList repositoryPaths) + { + var repositories = new List(); + foreach (var repositoryPath in repositoryPaths) + { + var repoInfo = Substitute.For(); + repoInfo.RepositoryPath.Returns(repositoryPath); + repositories.Add(repoInfo); + } + + return repositories.AsReadOnly(); + } + + static IGitHubServiceProvider CreateGitHubServiceProvider(IVSUIContext context, IGitExt gitExt = null) + { + var contextFactory = Substitute.For(); + gitExt = gitExt ?? Substitute.For(); + var contextGuid = new Guid(Guids.GitSccProviderId); + contextFactory.GetUIContext(contextGuid).Returns(context); + var sp = Substitute.For(); + sp.GetService().Returns(contextFactory); + sp.GetService().Returns(gitExt); + return sp; + } +} diff --git a/test/UnitTests/UnitTests.csproj b/test/UnitTests/UnitTests.csproj index 98fd53727b..3ad668a626 100644 --- a/test/UnitTests/UnitTests.csproj +++ b/test/UnitTests/UnitTests.csproj @@ -260,6 +260,7 @@ + @@ -371,6 +372,7 @@ Resources.Designer.cs + From 34109f2a1a7d4741132e5eab78c45c1746e23c9c Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 26 Jan 2018 11:05:51 +0000 Subject: [PATCH 47/64] Remove UIContext watching responsibilities from TeamExplorerServiceHolder This is now being handled by VSGitExt. --- .../Services/ITeamExplorerServiceHolder.cs | 5 - .../Base/TeamExplorerServiceHolder.cs | 179 +----------------- .../Services/VSGitExt.cs | 68 +++++++ 3 files changed, 74 insertions(+), 178 deletions(-) diff --git a/src/GitHub.Exports/Services/ITeamExplorerServiceHolder.cs b/src/GitHub.Exports/Services/ITeamExplorerServiceHolder.cs index ae60d6c8de..dd6ca75bd1 100644 --- a/src/GitHub.Exports/Services/ITeamExplorerServiceHolder.cs +++ b/src/GitHub.Exports/Services/ITeamExplorerServiceHolder.cs @@ -42,11 +42,6 @@ public interface ITeamExplorerServiceHolder void Unsubscribe(object who); IGitAwareItem HomeSection { get; } - - /// - /// Refresh the information on the active repo (in case of remote url changes or other such things) - /// - void Refresh(); } public interface IGitAwareItem diff --git a/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs b/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs index 59470f4da4..a26050e5fe 100644 --- a/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs +++ b/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs @@ -1,8 +1,6 @@ using System; using System.Collections.Generic; using System.ComponentModel.Composition; -using System.Diagnostics; -using System.Globalization; using System.Linq; using System.Threading; using GitHub.Extensions; @@ -10,8 +8,6 @@ using GitHub.Models; using GitHub.Services; using Microsoft.TeamFoundation.Controls; -using Microsoft.VisualStudio.Shell; -using Microsoft.VisualStudio.TeamFoundation.Git.Extensibility; using Serilog; namespace GitHub.VisualStudio.Base @@ -26,9 +22,7 @@ public class TeamExplorerServiceHolder : ITeamExplorerServiceHolder bool activeRepoNotified = false; IServiceProvider serviceProvider; - IVSGitExt gitService; - IVSUIContext gitUIContext; - IVSUIContextFactory uiContextFactory; + readonly IVSGitExt gitService; // ActiveRepositories PropertyChanged event comes in on a non-main thread readonly SynchronizationContext syncContext; @@ -43,10 +37,14 @@ public class TeamExplorerServiceHolder : ITeamExplorerServiceHolder [ImportingConstructor] public TeamExplorerServiceHolder(IVSGitExt gitService) { - this.GitService = gitService; + this.gitService = gitService; syncContext = SynchronizationContext.Current; + + UpdateActiveRepo(); + gitService.ActiveRepositoriesChanged += UpdateActiveRepo; } + // set by the sections when they get initialized public IServiceProvider ServiceProvider { @@ -59,8 +57,6 @@ public IServiceProvider ServiceProvider serviceProvider = value; if (serviceProvider == null) return; - GitUIContext = GitUIContext ?? UIContextFactory.GetUIContext(new Guid(Guids.GitSccProviderId)); - UIContextChanged(GitUIContext?.IsActive ?? false, false); } } @@ -133,12 +129,6 @@ public void ClearServiceProvider(IServiceProvider provider) ServiceProvider = null; } - public void Refresh() - { - GitUIContext = GitUIContext ?? UIContextFactory.GetUIContext(new Guid(Guids.GitSccProviderId)); - UIContextChanged(GitUIContext?.IsActive ?? false, true); - } - void NotifyActiveRepo() { lock (activeRepoHandlers) @@ -149,54 +139,6 @@ void NotifyActiveRepo() } } - void UIContextChanged(object sender, IVSUIContextChangedEventArgs e) - { - Guard.ArgumentNotNull(e, nameof(e)); - - ActiveRepo = null; - UIContextChanged(e.Activated, false); - } - - /// - /// This is called on a background thread. Do not do synchronous GetService calls here. - /// - /// - /// - async void UIContextChanged(bool active, bool refresh) - { - Debug.Assert(ServiceProvider != null, "UIContextChanged called before service provider is set"); - if (ServiceProvider == null) - return; - - if (active) - { - if (ActiveRepo == null || refresh) - { - ActiveRepo = await System.Threading.Tasks.Task.Run(() => - { - var repos = GitService.ActiveRepositories; - // Looks like this might return null after a while, for some unknown reason - // if it does, let's refresh the GitService instance in case something got wonky - // and try again. See issue #23 - if (repos == null) - { - log.Error("Error 2001: ActiveRepositories is null. GitService: '{GitService}'", GitService); - - // NOTE: VSGitExt is now self-refreshing - //GitService.Refresh(ServiceProvider); - - repos = GitService.ActiveRepositories; - if (repos == null) - log.Error("Error 2002: ActiveRepositories is null. GitService: '{GitService}'", GitService); - } - return repos?.FirstOrDefault(); - }); - } - } - else - ActiveRepo = null; - } - void UpdateActiveRepo() { var repo = gitService.ActiveRepositories.FirstOrDefault(); @@ -230,114 +172,5 @@ ITeamExplorerPage PageService { get { return ServiceProvider.GetServiceSafe(); } } - - IVSUIContext GitUIContext - { - get { return gitUIContext; } - set - { - if (gitUIContext == value) - return; - if (gitUIContext != null) - gitUIContext.UIContextChanged -= UIContextChanged; - gitUIContext = value; - if (gitUIContext != null) - gitUIContext.UIContextChanged += UIContextChanged; - } - } - - IVSGitExt GitService - { - get { return gitService; } - set - { - if (gitService == value) - return; - if (gitService != null) - gitService.ActiveRepositoriesChanged -= UpdateActiveRepo; - gitService = value; - if (gitService != null) - gitService.ActiveRepositoriesChanged += UpdateActiveRepo; - } - } - - IVSUIContextFactory UIContextFactory - { - get - { - if (uiContextFactory == null) - { - uiContextFactory = ServiceProvider.GetServiceSafe(); - } - return uiContextFactory; - } - } - } - - [Export(typeof(IVSUIContextFactory))] - [PartCreationPolicy(CreationPolicy.Shared)] - class VSUIContextFactory : IVSUIContextFactory - { - public IVSUIContext GetUIContext(Guid contextGuid) - { - return new VSUIContext(UIContext.FromUIContextGuid(contextGuid)); - } - } - - class VSUIContextChangedEventArgs : IVSUIContextChangedEventArgs - { - public bool Activated { get; } - - public VSUIContextChangedEventArgs(bool activated) - { - Activated = activated; - } - } - - class VSUIContext : IVSUIContext - { - readonly UIContext context; - readonly Dictionary, EventHandler> handlers = - new Dictionary, EventHandler>(); - public VSUIContext(UIContext context) - { - this.context = context; - } - - public bool IsActive { get { return context.IsActive; } } - - public event EventHandler UIContextChanged - { - add - { - EventHandler handler = null; - if (!handlers.TryGetValue(value, out handler)) - { - handler = (s, e) => value.Invoke(s, new VSUIContextChangedEventArgs(e.Activated)); - handlers.Add(value, handler); - } - context.UIContextChanged += handler; - } - remove - { - EventHandler handler = null; - if (handlers.TryGetValue(value, out handler)) - { - handlers.Remove(value); - context.UIContextChanged -= handler; - } - } - } - } - - static class IGitRepositoryInfoExtensions - { - /// - /// Create a LocalRepositoryModel from a VS git repo object - /// - public static ILocalRepositoryModel ToModel(this IGitRepositoryInfo repo) - { - return repo == null ? null : new LocalRepositoryModel(repo.RepositoryPath); - } } } diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index b9b433dad0..1c91e97e72 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -7,6 +7,7 @@ using GitHub.Logging; using Serilog; using Microsoft.VisualStudio.TeamFoundation.Git.Extensibility; +using Microsoft.VisualStudio.Shell; namespace GitHub.VisualStudio.Base { @@ -68,4 +69,71 @@ bool TryInitialize() public IEnumerable ActiveRepositories => gitService?.ActiveRepositories.Select(x => x.ToModel()); public event Action ActiveRepositoriesChanged; } + + [Export(typeof(IVSUIContextFactory))] + [PartCreationPolicy(CreationPolicy.Shared)] + class VSUIContextFactory : IVSUIContextFactory + { + public IVSUIContext GetUIContext(Guid contextGuid) + { + return new VSUIContext(UIContext.FromUIContextGuid(contextGuid)); + } + } + + class VSUIContextChangedEventArgs : IVSUIContextChangedEventArgs + { + public bool Activated { get; } + + public VSUIContextChangedEventArgs(bool activated) + { + Activated = activated; + } + } + + class VSUIContext : IVSUIContext + { + readonly UIContext context; + readonly Dictionary, EventHandler> handlers = + new Dictionary, EventHandler>(); + public VSUIContext(UIContext context) + { + this.context = context; + } + + public bool IsActive { get { return context.IsActive; } } + + public event EventHandler UIContextChanged + { + add + { + EventHandler handler = null; + if (!handlers.TryGetValue(value, out handler)) + { + handler = (s, e) => value.Invoke(s, new VSUIContextChangedEventArgs(e.Activated)); + handlers.Add(value, handler); + } + context.UIContextChanged += handler; + } + remove + { + EventHandler handler = null; + if (handlers.TryGetValue(value, out handler)) + { + handlers.Remove(value); + context.UIContextChanged -= handler; + } + } + } + } + + static class IGitRepositoryInfoExtensions + { + /// + /// Create a LocalRepositoryModel from a VS git repo object + /// + public static ILocalRepositoryModel ToModel(this IGitRepositoryInfo repo) + { + return repo == null ? null : new LocalRepositoryModel(repo.RepositoryPath); + } + } } \ No newline at end of file From d69d35887269bafb6aef600ae93184ede20fb858 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 26 Jan 2018 12:02:05 +0000 Subject: [PATCH 48/64] Export single IVSUIContextFactory implementation in GH.TF.14 This allows us to consume IVSUIContextFactory as a MEF service and simplify unit tests. --- .../GitHub.TeamFoundation.14.csproj | 1 + .../Services/VSGitExt.cs | 60 +---------------- .../Services/VSUIContextFactory.cs | 64 +++++++++++++++++++ .../GitHub.TeamFoundation/VSGitExtTests.cs | 36 +++++------ 4 files changed, 82 insertions(+), 79 deletions(-) create mode 100644 src/GitHub.TeamFoundation.14/Services/VSUIContextFactory.cs diff --git a/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj b/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj index 90f158efde..ca8accb5a8 100644 --- a/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj +++ b/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj @@ -134,6 +134,7 @@ + diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index 1c91e97e72..7e0c5ae395 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -7,7 +7,6 @@ using GitHub.Logging; using Serilog; using Microsoft.VisualStudio.TeamFoundation.Git.Extensibility; -using Microsoft.VisualStudio.Shell; namespace GitHub.VisualStudio.Base { @@ -20,10 +19,9 @@ public class VSGitExt : IVSGitExt IGitExt gitService; [ImportingConstructor] - public VSGitExt(IGitHubServiceProvider serviceProvider) + public VSGitExt(IGitHubServiceProvider serviceProvider, IVSUIContextFactory factory) { this.serviceProvider = serviceProvider; - var factory = serviceProvider.GetService(); // The IGitExt service is only available when in the SccProvider context. // This could be changed to VSConstants.UICONTEXT.SolutionExists_guid when testing. @@ -70,62 +68,6 @@ bool TryInitialize() public event Action ActiveRepositoriesChanged; } - [Export(typeof(IVSUIContextFactory))] - [PartCreationPolicy(CreationPolicy.Shared)] - class VSUIContextFactory : IVSUIContextFactory - { - public IVSUIContext GetUIContext(Guid contextGuid) - { - return new VSUIContext(UIContext.FromUIContextGuid(contextGuid)); - } - } - - class VSUIContextChangedEventArgs : IVSUIContextChangedEventArgs - { - public bool Activated { get; } - - public VSUIContextChangedEventArgs(bool activated) - { - Activated = activated; - } - } - - class VSUIContext : IVSUIContext - { - readonly UIContext context; - readonly Dictionary, EventHandler> handlers = - new Dictionary, EventHandler>(); - public VSUIContext(UIContext context) - { - this.context = context; - } - - public bool IsActive { get { return context.IsActive; } } - - public event EventHandler UIContextChanged - { - add - { - EventHandler handler = null; - if (!handlers.TryGetValue(value, out handler)) - { - handler = (s, e) => value.Invoke(s, new VSUIContextChangedEventArgs(e.Activated)); - handlers.Add(value, handler); - } - context.UIContextChanged += handler; - } - remove - { - EventHandler handler = null; - if (handlers.TryGetValue(value, out handler)) - { - handlers.Remove(value); - context.UIContextChanged -= handler; - } - } - } - } - static class IGitRepositoryInfoExtensions { /// diff --git a/src/GitHub.TeamFoundation.14/Services/VSUIContextFactory.cs b/src/GitHub.TeamFoundation.14/Services/VSUIContextFactory.cs new file mode 100644 index 0000000000..9a7211f2a0 --- /dev/null +++ b/src/GitHub.TeamFoundation.14/Services/VSUIContextFactory.cs @@ -0,0 +1,64 @@ +using System; +using System.Collections.Generic; +using System.ComponentModel.Composition; +using Microsoft.VisualStudio.Shell; +using GitHub.Services; + +namespace GitHub.TeamFoundation.Services +{ + [Export(typeof(IVSUIContextFactory))] + [PartCreationPolicy(CreationPolicy.Shared)] + class VSUIContextFactory : IVSUIContextFactory + { + public IVSUIContext GetUIContext(Guid contextGuid) + { + return new VSUIContext(UIContext.FromUIContextGuid(contextGuid)); + } + } + + class VSUIContextChangedEventArgs : IVSUIContextChangedEventArgs + { + public bool Activated { get; } + + public VSUIContextChangedEventArgs(bool activated) + { + Activated = activated; + } + } + + class VSUIContext : IVSUIContext + { + readonly UIContext context; + readonly Dictionary, EventHandler> handlers = + new Dictionary, EventHandler>(); + public VSUIContext(UIContext context) + { + this.context = context; + } + + public bool IsActive { get { return context.IsActive; } } + + public event EventHandler UIContextChanged + { + add + { + EventHandler handler = null; + if (!handlers.TryGetValue(value, out handler)) + { + handler = (s, e) => value.Invoke(s, new VSUIContextChangedEventArgs(e.Activated)); + handlers.Add(value, handler); + } + context.UIContextChanged += handler; + } + remove + { + EventHandler handler = null; + if (handlers.TryGetValue(value, out handler)) + { + handlers.Remove(value); + context.UIContextChanged -= handler; + } + } + } + } +} diff --git a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs index 217392fa6e..a34dee7e83 100644 --- a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs +++ b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs @@ -18,9 +18,9 @@ public class TheConstructor public void GetServiceIGitExt_WhenSccProviderContextIsActive(bool isActive, int expectCalls) { var context = CreateVSUIContext(isActive); - var sp = CreateGitHubServiceProvider(context); + var sp = Substitute.For(); - var target = new VSGitExt(sp); + var target = CreateVSGitExt(context, sp: sp); sp.Received(expectCalls).GetService(); } @@ -30,9 +30,9 @@ public void GetServiceIGitExt_WhenSccProviderContextIsActive(bool isActive, int public void GetServiceIGitExt_WhenUIContextChanged(bool activated, int expectCalls) { var context = CreateVSUIContext(false); - var sp = CreateGitHubServiceProvider(context); + var sp = Substitute.For(); + var target = CreateVSGitExt(context, sp: sp); - var target = new VSGitExt(sp); var eventArgs = Substitute.For(); eventArgs.Activated.Returns(activated); context.UIContextChanged += Raise.Event>(context, eventArgs); @@ -48,8 +48,8 @@ public void GitExtPropertyChangedEvent_ActiveRepositoriesChangedIsFired() { var context = CreateVSUIContext(true); var gitExt = CreateGitExt(); - var sp = CreateGitHubServiceProvider(context, gitExt); - var target = new VSGitExt(sp); + + var target = CreateVSGitExt(context, gitExt); bool wasFired = false; target.ActiveRepositoriesChanged += () => wasFired = true; @@ -64,8 +64,7 @@ public void WhenUIContextChanged_ActiveRepositoriesChangedIsFired() { var context = CreateVSUIContext(false); var gitExt = CreateGitExt(); - var sp = CreateGitHubServiceProvider(context, gitExt); - var target = new VSGitExt(sp); + var target = CreateVSGitExt(context, gitExt); bool wasFired = false; target.ActiveRepositoriesChanged += () => wasFired = true; @@ -84,9 +83,7 @@ public class TheActiveRepositoriesProperty public void SccProviderContextNotActive_IsNull() { var context = CreateVSUIContext(false); - var sp = CreateGitHubServiceProvider(context); - - var target = new VSGitExt(sp); + var target = CreateVSGitExt(context); Assert.That(target.ActiveRepositories, Is.Null); } @@ -96,8 +93,7 @@ public void SccProviderContextIsActive_DefaultEmptyListFromGitExt() { var context = CreateVSUIContext(true); var gitExt = CreateGitExt(new string[0]); - var sp = CreateGitHubServiceProvider(context, gitExt); - var target = new VSGitExt(sp); + var target = CreateVSGitExt(context, gitExt); var activeRepositories = target.ActiveRepositories; @@ -105,7 +101,7 @@ public void SccProviderContextIsActive_DefaultEmptyListFromGitExt() } // TODO: We can't currently test returning a non-empty list because it constructs a live LocalRepositoryModel object. - // Move could move the responsibility for constructing a LocalRepositoryModel object outside of VSGitExt. + // We could move the responsibility for constructing a LocalRepositoryModel object outside of VSGitExt. } static IVSUIContext CreateVSUIContext(bool isActive) @@ -137,15 +133,15 @@ static IReadOnlyList CreateActiveRepositories(IList return repositories.AsReadOnly(); } - static IGitHubServiceProvider CreateGitHubServiceProvider(IVSUIContext context, IGitExt gitExt = null) + static IVSGitExt CreateVSGitExt(IVSUIContext context, IGitExt gitExt = null, IGitHubServiceProvider sp = null) { - var contextFactory = Substitute.For(); + sp = sp ?? Substitute.For(); + var factory = Substitute.For(); gitExt = gitExt ?? Substitute.For(); var contextGuid = new Guid(Guids.GitSccProviderId); - contextFactory.GetUIContext(contextGuid).Returns(context); - var sp = Substitute.For(); - sp.GetService().Returns(contextFactory); + factory.GetUIContext(contextGuid).Returns(context); + sp.GetService().Returns(factory); sp.GetService().Returns(gitExt); - return sp; + return new VSGitExt(sp, factory); } } From 470991d8d3b4033f0dededcc1c18a335d2597b32 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 26 Jan 2018 12:07:10 +0000 Subject: [PATCH 49/64] Keep CA happy --- src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs b/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs index a26050e5fe..f13aa32c4a 100644 --- a/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs +++ b/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs @@ -4,11 +4,9 @@ using System.Linq; using System.Threading; using GitHub.Extensions; -using GitHub.Logging; using GitHub.Models; using GitHub.Services; using Microsoft.TeamFoundation.Controls; -using Serilog; namespace GitHub.VisualStudio.Base { @@ -16,7 +14,6 @@ namespace GitHub.VisualStudio.Base [PartCreationPolicy(CreationPolicy.Shared)] public class TeamExplorerServiceHolder : ITeamExplorerServiceHolder { - static readonly ILogger log = LogManager.ForContext(); readonly Dictionary> activeRepoHandlers = new Dictionary>(); ILocalRepositoryModel activeRepo; bool activeRepoNotified = false; From a801953da94b633d3fabc32bff0afee38e55055d Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 26 Jan 2018 12:17:52 +0000 Subject: [PATCH 50/64] Remove IVSUIContextChangedEventArgs interface The VSUIContextChangedEventArgs class can be used directly and doesn't need an interface. --- src/GitHub.Exports/Services/IVSUIContext.cs | 14 +++++++++----- .../Services/VSGitExt.cs | 2 +- .../Services/VSUIContextFactory.cs | 16 +++------------- .../GitHub.TeamFoundation/VSGitExtTests.cs | 10 ++++------ 4 files changed, 17 insertions(+), 25 deletions(-) diff --git a/src/GitHub.Exports/Services/IVSUIContext.cs b/src/GitHub.Exports/Services/IVSUIContext.cs index 25acd9b632..e3aca6cbb7 100644 --- a/src/GitHub.Exports/Services/IVSUIContext.cs +++ b/src/GitHub.Exports/Services/IVSUIContext.cs @@ -1,5 +1,4 @@ -using Microsoft.VisualStudio.Shell; -using System; +using System; namespace GitHub.Services { @@ -8,14 +7,19 @@ public interface IVSUIContextFactory IVSUIContext GetUIContext(Guid contextGuid); } - public interface IVSUIContextChangedEventArgs + public sealed class VSUIContextChangedEventArgs { - bool Activated { get; } + public VSUIContextChangedEventArgs(bool activated) + { + Activated = activated; + } + + public bool Activated { get; } } public interface IVSUIContext { bool IsActive { get; } - event EventHandler UIContextChanged; + event EventHandler UIContextChanged; } } \ No newline at end of file diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index 7e0c5ae395..21afa0385e 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -34,7 +34,7 @@ public VSGitExt(IGitHubServiceProvider serviceProvider, IVSUIContextFactory fact } } - void ContextChanged(object sender, IVSUIContextChangedEventArgs e) + void ContextChanged(object sender, VSUIContextChangedEventArgs e) { // If we're in the GitSccProvider context and TryInitialize succeeds, we can stop listening for events. if (e.Activated && TryInitialize()) diff --git a/src/GitHub.TeamFoundation.14/Services/VSUIContextFactory.cs b/src/GitHub.TeamFoundation.14/Services/VSUIContextFactory.cs index 9a7211f2a0..12d9c3ff40 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSUIContextFactory.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSUIContextFactory.cs @@ -16,21 +16,11 @@ public IVSUIContext GetUIContext(Guid contextGuid) } } - class VSUIContextChangedEventArgs : IVSUIContextChangedEventArgs - { - public bool Activated { get; } - - public VSUIContextChangedEventArgs(bool activated) - { - Activated = activated; - } - } - class VSUIContext : IVSUIContext { readonly UIContext context; - readonly Dictionary, EventHandler> handlers = - new Dictionary, EventHandler>(); + readonly Dictionary, EventHandler> handlers = + new Dictionary, EventHandler>(); public VSUIContext(UIContext context) { this.context = context; @@ -38,7 +28,7 @@ public VSUIContext(UIContext context) public bool IsActive { get { return context.IsActive; } } - public event EventHandler UIContextChanged + public event EventHandler UIContextChanged { add { diff --git a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs index a34dee7e83..f6f322ee3f 100644 --- a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs +++ b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs @@ -33,9 +33,8 @@ public void GetServiceIGitExt_WhenUIContextChanged(bool activated, int expectCal var sp = Substitute.For(); var target = CreateVSGitExt(context, sp: sp); - var eventArgs = Substitute.For(); - eventArgs.Activated.Returns(activated); - context.UIContextChanged += Raise.Event>(context, eventArgs); + var eventArgs = new VSUIContextChangedEventArgs(activated); + context.UIContextChanged += Raise.Event>(context, eventArgs); sp.Received(expectCalls).GetService(); } @@ -69,9 +68,8 @@ public void WhenUIContextChanged_ActiveRepositoriesChangedIsFired() bool wasFired = false; target.ActiveRepositoriesChanged += () => wasFired = true; - var eventArgs = Substitute.For(); - eventArgs.Activated.Returns(true); - context.UIContextChanged += Raise.Event>(context, eventArgs); + var eventArgs = new VSUIContextChangedEventArgs(true); + context.UIContextChanged += Raise.Event>(context, eventArgs); Assert.That(wasFired, Is.True); } From 79ea85011f4ca77b5a791a3ef77e3ba7cec81007 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 26 Jan 2018 12:39:02 +0000 Subject: [PATCH 51/64] Make IVSUIContextFactory filename consistent with service We're exporting the IVSUIContextFactory service not IVSUIContext. --- src/GitHub.Exports/GitHub.Exports.csproj | 2 +- .../Services/{IVSUIContext.cs => IVSUIContextFactory.cs} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename src/GitHub.Exports/Services/{IVSUIContext.cs => IVSUIContextFactory.cs} (100%) diff --git a/src/GitHub.Exports/GitHub.Exports.csproj b/src/GitHub.Exports/GitHub.Exports.csproj index 4bf3018444..52e4262891 100644 --- a/src/GitHub.Exports/GitHub.Exports.csproj +++ b/src/GitHub.Exports/GitHub.Exports.csproj @@ -256,7 +256,7 @@ - + diff --git a/src/GitHub.Exports/Services/IVSUIContext.cs b/src/GitHub.Exports/Services/IVSUIContextFactory.cs similarity index 100% rename from src/GitHub.Exports/Services/IVSUIContext.cs rename to src/GitHub.Exports/Services/IVSUIContextFactory.cs From 2ccabb1a4c78774e5ff7a72c66f813f42113bea9 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 26 Jan 2018 12:45:48 +0000 Subject: [PATCH 52/64] Add logging for VSGitExt --- src/GitHub.TeamFoundation.14/Services/VSGitExt.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index 21afa0385e..f45980e976 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -14,6 +14,8 @@ namespace GitHub.VisualStudio.Base [PartCreationPolicy(CreationPolicy.Shared)] public class VSGitExt : IVSGitExt { + static readonly ILogger log = LogManager.ForContext(); + IGitHubServiceProvider serviceProvider; IVSUIContext context; IGitExt gitService; @@ -31,6 +33,7 @@ public VSGitExt(IGitHubServiceProvider serviceProvider, IVSUIContextFactory fact if (!context.IsActive || !TryInitialize()) { context.UIContextChanged += ContextChanged; + log.Information("VSGitExt will be initialized later"); } } @@ -40,6 +43,7 @@ void ContextChanged(object sender, VSUIContextChangedEventArgs e) if (e.Activated && TryInitialize()) { context.UIContextChanged -= ContextChanged; + log.Information("Initialized VSGitExt on UIContextChanged"); } } @@ -58,9 +62,11 @@ bool TryInitialize() } }; + log.Information("Found IGitExt service and initialized VSGitExt"); return true; } + log.Information("Couldn't find IGitExt service"); return false; } From 7e9405ab39db34db9114800e1a1e76a1391cbe6c Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 26 Jan 2018 13:19:36 +0000 Subject: [PATCH 53/64] Update and add xmldoc comments TeamExplorerServiceHolder no longer watches for UIContextChanged events and refreshes VSGitExt. --- .../Base/TeamExplorerServiceHolder.cs | 7 ++----- src/GitHub.TeamFoundation.14/Services/VSGitExt.cs | 3 +++ 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs b/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs index f13aa32c4a..b0725a014d 100644 --- a/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs +++ b/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs @@ -25,12 +25,9 @@ public class TeamExplorerServiceHolder : ITeamExplorerServiceHolder readonly SynchronizationContext syncContext; /// - /// This class relies on IVSUIContextFactory to get the UIContext object that provides information - /// when VS switches repositories. Unfortunately, for some reason MEF fails to create the instance - /// when imported from the constructor, so it's imported manually when first accessed via the - /// ServiceProvider instance (when mocking, make sure that the ServiceProvider includes this factory) + /// This class relies on IVSGitExt that provides information when VS switches repositories. /// - /// + /// Used for monitoring the active repository. [ImportingConstructor] public TeamExplorerServiceHolder(IVSGitExt gitService) { diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index f45980e976..23447508ea 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -10,6 +10,9 @@ namespace GitHub.VisualStudio.Base { + /// + /// This service acts as an always available version of . + /// [Export(typeof(IVSGitExt))] [PartCreationPolicy(CreationPolicy.Shared)] public class VSGitExt : IVSGitExt From 77bd3b2bd1907bffca81c06c849b716b95d43e5e Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 26 Jan 2018 18:14:40 +0000 Subject: [PATCH 54/64] Ensure ActiveRepositories is read using ThreadPoolThread Make sure ActiveRepositories isn't read during the construction of our service. ActiveRepositories is first initialized on ThreadPoolThread. It is later updated when the property changed event is fired. --- .../Services/VSGitExt.cs | 34 ++++++++-- .../GitHub.TeamFoundation/VSGitExtTests.cs | 62 ++++++++++++------- 2 files changed, 70 insertions(+), 26 deletions(-) diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index 23447508ea..893781ae40 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -7,6 +7,7 @@ using GitHub.Logging; using Serilog; using Microsoft.VisualStudio.TeamFoundation.Git.Extensibility; +using Task = System.Threading.Tasks.Task; namespace GitHub.VisualStudio.Base { @@ -32,11 +33,20 @@ public VSGitExt(IGitHubServiceProvider serviceProvider, IVSUIContextFactory fact // This could be changed to VSConstants.UICONTEXT.SolutionExists_guid when testing. context = factory.GetUIContext(new Guid(Guids.GitSccProviderId)); + // Start with empty array until we have a change to initialize. + ActiveRepositories = new ILocalRepositoryModel[0]; + // If we're not in the GitSccProvider context or TryInitialize fails, have another go when the context changes. - if (!context.IsActive || !TryInitialize()) + if (context.IsActive && TryInitialize()) + { + //// RefreshActiveRepositories on background thread so we don't impact startup. + InitializeTask = Task.Run(() => RefreshActiveRepositories()); + } + else { context.UIContextChanged += ContextChanged; log.Information("VSGitExt will be initialized later"); + InitializeTask = Task.CompletedTask; } } @@ -45,6 +55,7 @@ void ContextChanged(object sender, VSUIContextChangedEventArgs e) // If we're in the GitSccProvider context and TryInitialize succeeds, we can stop listening for events. if (e.Activated && TryInitialize()) { + RefreshActiveRepositories(); context.UIContextChanged -= ContextChanged; log.Information("Initialized VSGitExt on UIContextChanged"); } @@ -55,13 +66,11 @@ bool TryInitialize() gitService = serviceProvider.GetService(); if (gitService != null) { - // The IGitExt service is now available so let consumers know to read ActiveRepositories. - ActiveRepositoriesChanged?.Invoke(); gitService.PropertyChanged += (s, e) => { if (e.PropertyName == nameof(gitService.ActiveRepositories)) { - ActiveRepositoriesChanged?.Invoke(); + RefreshActiveRepositories(); } }; @@ -73,8 +82,23 @@ bool TryInitialize() return false; } - public IEnumerable ActiveRepositories => gitService?.ActiveRepositories.Select(x => x.ToModel()); + void RefreshActiveRepositories() + { + try + { + ActiveRepositories = gitService?.ActiveRepositories.Select(x => x.ToModel()); + ActiveRepositoriesChanged?.Invoke(); + } + catch (Exception e) + { + log.Error(e, "Error refreshing repositories"); + } + } + + public IEnumerable ActiveRepositories { get; private set; } public event Action ActiveRepositoriesChanged; + + public Task InitializeTask { get; } } static class IGitRepositoryInfoExtensions diff --git a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs index f6f322ee3f..78cf48eb19 100644 --- a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs +++ b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs @@ -1,5 +1,6 @@ using System; using System.Linq; +using System.Threading; using System.ComponentModel; using System.Collections.Generic; using GitHub.Services; @@ -38,6 +39,22 @@ public void GetServiceIGitExt_WhenUIContextChanged(bool activated, int expectCal sp.Received(expectCalls).GetService(); } + + [Test] + public void ActiveRepositories_ReadUsingThreadPoolThread() + { + var gitExt = Substitute.For(); + bool threadPool = false; + gitExt.ActiveRepositories.Returns(x => + { + threadPool = Thread.CurrentThread.IsThreadPoolThread; + return new IGitRepositoryInfo[0]; + }); + + var target = CreateVSGitExt(gitExt: gitExt); + + Assert.That(threadPool, Is.True); + } } public class TheActiveRepositoriesChangedEvent @@ -78,12 +95,12 @@ public void WhenUIContextChanged_ActiveRepositoriesChangedIsFired() public class TheActiveRepositoriesProperty { [Test] - public void SccProviderContextNotActive_IsNull() + public void SccProviderContextNotActive_IsEmpty() { var context = CreateVSUIContext(false); var target = CreateVSGitExt(context); - Assert.That(target.ActiveRepositories, Is.Null); + Assert.That(target.ActiveRepositories, Is.Empty); } [Test] @@ -102,22 +119,6 @@ public void SccProviderContextIsActive_DefaultEmptyListFromGitExt() // We could move the responsibility for constructing a LocalRepositoryModel object outside of VSGitExt. } - static IVSUIContext CreateVSUIContext(bool isActive) - { - var context = Substitute.For(); - context.IsActive.Returns(isActive); - return context; - } - - static IGitExt CreateGitExt(IList repositoryPaths = null) - { - repositoryPaths = repositoryPaths ?? new string[0]; - var gitExt = Substitute.For(); - var repoList = CreateActiveRepositories(new string[0]); - gitExt.ActiveRepositories.Returns(repoList); - return gitExt; - } - static IReadOnlyList CreateActiveRepositories(IList repositoryPaths) { var repositories = new List(); @@ -131,15 +132,34 @@ static IReadOnlyList CreateActiveRepositories(IList return repositories.AsReadOnly(); } - static IVSGitExt CreateVSGitExt(IVSUIContext context, IGitExt gitExt = null, IGitHubServiceProvider sp = null) + static IVSGitExt CreateVSGitExt(IVSUIContext context = null, IGitExt gitExt = null, IGitHubServiceProvider sp = null) { + context = context ?? CreateVSUIContext(true); + gitExt = gitExt ?? CreateGitExt(); sp = sp ?? Substitute.For(); var factory = Substitute.For(); - gitExt = gitExt ?? Substitute.For(); var contextGuid = new Guid(Guids.GitSccProviderId); factory.GetUIContext(contextGuid).Returns(context); sp.GetService().Returns(factory); sp.GetService().Returns(gitExt); - return new VSGitExt(sp, factory); + var vsGitExt = new VSGitExt(sp, factory); + vsGitExt.InitializeTask.Wait(); + return vsGitExt; + } + + static IGitExt CreateGitExt(IList repositoryPaths = null) + { + repositoryPaths = repositoryPaths ?? new string[0]; + var gitExt = Substitute.For(); + var repoList = CreateActiveRepositories(new string[0]); + gitExt.ActiveRepositories.Returns(repoList); + return gitExt; + } + + static IVSUIContext CreateVSUIContext(bool isActive) + { + var context = Substitute.For(); + context.IsActive.Returns(isActive); + return context; } } From dd3920273a93051178b856cdcfdfb551ca23803b Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 26 Jan 2018 18:51:30 +0000 Subject: [PATCH 55/64] Update comments on how we're using UIContext --- src/GitHub.TeamFoundation.14/Services/VSGitExt.cs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index 893781ae40..ac8d34dd08 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -29,21 +29,21 @@ public VSGitExt(IGitHubServiceProvider serviceProvider, IVSUIContextFactory fact { this.serviceProvider = serviceProvider; - // The IGitExt service is only available when in the SccProvider context. - // This could be changed to VSConstants.UICONTEXT.SolutionExists_guid when testing. + // The IGitExt service isn't available when a TFS based solution is opened directly. + // It will become available when moving to a Git based solution (cause a UIContext event to fire). context = factory.GetUIContext(new Guid(Guids.GitSccProviderId)); // Start with empty array until we have a change to initialize. ActiveRepositories = new ILocalRepositoryModel[0]; - // If we're not in the GitSccProvider context or TryInitialize fails, have another go when the context changes. if (context.IsActive && TryInitialize()) { - //// RefreshActiveRepositories on background thread so we don't impact startup. + // Refresh ActiveRepositories on background thread so we don't delay startup. InitializeTask = Task.Run(() => RefreshActiveRepositories()); } else { + // If we're not in the UIContext or TryInitialize fails, have another go when the UIContext changes. context.UIContextChanged += ContextChanged; log.Information("VSGitExt will be initialized later"); InitializeTask = Task.CompletedTask; @@ -52,7 +52,8 @@ public VSGitExt(IGitHubServiceProvider serviceProvider, IVSUIContextFactory fact void ContextChanged(object sender, VSUIContextChangedEventArgs e) { - // If we're in the GitSccProvider context and TryInitialize succeeds, we can stop listening for events. + // If we're in the UIContext and TryInitialize succeeds, we can stop listening for events. + // NOTE: this event can fire with UIContext=true in a TFS solution (not just Git). if (e.Activated && TryInitialize()) { RefreshActiveRepositories(); From e2c51e82b9de798d938ab62bac87363e81af4871 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 26 Jan 2018 18:51:39 +0000 Subject: [PATCH 56/64] Cleanup usings --- src/GitHub.Exports/Services/IVSGitExt.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/GitHub.Exports/Services/IVSGitExt.cs b/src/GitHub.Exports/Services/IVSGitExt.cs index 86c7294173..f0944c5aca 100644 --- a/src/GitHub.Exports/Services/IVSGitExt.cs +++ b/src/GitHub.Exports/Services/IVSGitExt.cs @@ -1,8 +1,6 @@ -using GitHub.Models; -using System; +using System; using System.Collections.Generic; -using System.ComponentModel; -using System.ComponentModel.Composition; +using GitHub.Models; namespace GitHub.Services { From edf9ff83865d6a1310af3c8ce6fb556ef75b1ec1 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Mon, 29 Jan 2018 09:36:06 +0000 Subject: [PATCH 57/64] Don't read ActiveRepositories on UIContext thread Ensure that ActiveRepositories is only read on thread pool thread or its own property change event. --- .../Services/VSGitExt.cs | 5 +++-- .../GitHub.TeamFoundation/VSGitExtTests.cs | 20 ++++++++++++++++++- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index ac8d34dd08..eefbe024e2 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -56,7 +56,8 @@ void ContextChanged(object sender, VSUIContextChangedEventArgs e) // NOTE: this event can fire with UIContext=true in a TFS solution (not just Git). if (e.Activated && TryInitialize()) { - RefreshActiveRepositories(); + // Refresh ActiveRepositories on background thread so we don't delay UI context change. + InitializeTask = Task.Run(() => RefreshActiveRepositories()); context.UIContextChanged -= ContextChanged; log.Information("Initialized VSGitExt on UIContextChanged"); } @@ -99,7 +100,7 @@ void RefreshActiveRepositories() public IEnumerable ActiveRepositories { get; private set; } public event Action ActiveRepositoriesChanged; - public Task InitializeTask { get; } + public Task InitializeTask { get; private set; } } static class IGitRepositoryInfoExtensions diff --git a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs index 78cf48eb19..f53946d3cd 100644 --- a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs +++ b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs @@ -87,9 +87,27 @@ public void WhenUIContextChanged_ActiveRepositoriesChangedIsFired() var eventArgs = new VSUIContextChangedEventArgs(true); context.UIContextChanged += Raise.Event>(context, eventArgs); + target.InitializeTask.Wait(); Assert.That(wasFired, Is.True); } + + [Test] + public void WhenUIContextChanged_FiredUsingThreadPoolThread() + { + var context = CreateVSUIContext(false); + var gitExt = CreateGitExt(); + var target = CreateVSGitExt(context, gitExt); + + bool threadPool = false; + target.ActiveRepositoriesChanged += () => threadPool = Thread.CurrentThread.IsThreadPoolThread; + + var eventArgs = new VSUIContextChangedEventArgs(true); + context.UIContextChanged += Raise.Event>(context, eventArgs); + target.InitializeTask.Wait(); + + Assert.That(threadPool, Is.True); + } } public class TheActiveRepositoriesProperty @@ -132,7 +150,7 @@ static IReadOnlyList CreateActiveRepositories(IList return repositories.AsReadOnly(); } - static IVSGitExt CreateVSGitExt(IVSUIContext context = null, IGitExt gitExt = null, IGitHubServiceProvider sp = null) + static VSGitExt CreateVSGitExt(IVSUIContext context = null, IGitExt gitExt = null, IGitHubServiceProvider sp = null) { context = context ?? CreateVSUIContext(true); gitExt = gitExt ?? CreateGitExt(); From a65027ce0b0c03ee968449d3b9c9101841e8f19d Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Mon, 29 Jan 2018 10:49:17 +0000 Subject: [PATCH 58/64] Add repo model factory and tests for repository creation Added LocalRepositoryModelFactory service (and interface) so we can check the correct repositorys are being created. Create repositories on thread pool rather than calling thread. Return empty list if there's an exception creating repositories. --- src/GitHub.Exports/GitHub.Exports.csproj | 1 + .../Models/ILocalRepositoryModelFactory.cs | 15 +++++++ .../GitHub.TeamFoundation.14.csproj | 1 + .../Services/LocalRepositoryModelFactory.cs | 15 +++++++ .../Services/VSGitExt.cs | 23 ++++------- .../GitHub.TeamFoundation/VSGitExtTests.cs | 39 ++++++++++++++----- 6 files changed, 70 insertions(+), 24 deletions(-) create mode 100644 src/GitHub.Exports/Models/ILocalRepositoryModelFactory.cs create mode 100644 src/GitHub.TeamFoundation.14/Services/LocalRepositoryModelFactory.cs diff --git a/src/GitHub.Exports/GitHub.Exports.csproj b/src/GitHub.Exports/GitHub.Exports.csproj index 52e4262891..91a658933b 100644 --- a/src/GitHub.Exports/GitHub.Exports.csproj +++ b/src/GitHub.Exports/GitHub.Exports.csproj @@ -149,6 +149,7 @@ + diff --git a/src/GitHub.Exports/Models/ILocalRepositoryModelFactory.cs b/src/GitHub.Exports/Models/ILocalRepositoryModelFactory.cs new file mode 100644 index 0000000000..3c8dfed0ce --- /dev/null +++ b/src/GitHub.Exports/Models/ILocalRepositoryModelFactory.cs @@ -0,0 +1,15 @@ +namespace GitHub.Models +{ + /// + /// A factory for objects. + /// + public interface ILocalRepositoryModelFactory + { + /// + /// Construct a new . + /// + /// The local path for the repository. + /// A new repository model. + ILocalRepositoryModel Create(string localPath); + } +} diff --git a/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj b/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj index ca8accb5a8..352f4c7b3b 100644 --- a/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj +++ b/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj @@ -134,6 +134,7 @@ + diff --git a/src/GitHub.TeamFoundation.14/Services/LocalRepositoryModelFactory.cs b/src/GitHub.TeamFoundation.14/Services/LocalRepositoryModelFactory.cs new file mode 100644 index 0000000000..d4445b09ec --- /dev/null +++ b/src/GitHub.TeamFoundation.14/Services/LocalRepositoryModelFactory.cs @@ -0,0 +1,15 @@ +using System.ComponentModel.Composition; +using GitHub.Models; + +namespace GitHub.TeamFoundation.Services +{ + [Export(typeof(ILocalRepositoryModelFactory))] + [PartCreationPolicy(CreationPolicy.Shared)] + class LocalRepositoryModelFactory : ILocalRepositoryModelFactory + { + public ILocalRepositoryModel Create(string localPath) + { + return new LocalRepositoryModel(localPath); + } + } +} diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index eefbe024e2..a9fa2c66ea 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -20,14 +20,17 @@ public class VSGitExt : IVSGitExt { static readonly ILogger log = LogManager.ForContext(); - IGitHubServiceProvider serviceProvider; - IVSUIContext context; + readonly IGitHubServiceProvider serviceProvider; + readonly IVSUIContext context; + readonly ILocalRepositoryModelFactory repositoryFactory; + IGitExt gitService; [ImportingConstructor] - public VSGitExt(IGitHubServiceProvider serviceProvider, IVSUIContextFactory factory) + public VSGitExt(IGitHubServiceProvider serviceProvider, IVSUIContextFactory factory, ILocalRepositoryModelFactory repositoryFactory) { this.serviceProvider = serviceProvider; + this.repositoryFactory = repositoryFactory; // The IGitExt service isn't available when a TFS based solution is opened directly. // It will become available when moving to a Git based solution (cause a UIContext event to fire). @@ -88,12 +91,13 @@ void RefreshActiveRepositories() { try { - ActiveRepositories = gitService?.ActiveRepositories.Select(x => x.ToModel()); + ActiveRepositories = gitService?.ActiveRepositories.Select(x => repositoryFactory.Create(x.RepositoryPath)).ToList(); ActiveRepositoriesChanged?.Invoke(); } catch (Exception e) { log.Error(e, "Error refreshing repositories"); + ActiveRepositories = new ILocalRepositoryModel[0]; } } @@ -102,15 +106,4 @@ void RefreshActiveRepositories() public Task InitializeTask { get; private set; } } - - static class IGitRepositoryInfoExtensions - { - /// - /// Create a LocalRepositoryModel from a VS git repo object - /// - public static ILocalRepositoryModel ToModel(this IGitRepositoryInfo repo) - { - return repo == null ? null : new LocalRepositoryModel(repo.RepositoryPath); - } - } } \ No newline at end of file diff --git a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs index f53946d3cd..e8597797bd 100644 --- a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs +++ b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs @@ -3,6 +3,7 @@ using System.Threading; using System.ComponentModel; using System.Collections.Generic; +using GitHub.Models; using GitHub.Services; using GitHub.VisualStudio; using GitHub.VisualStudio.Base; @@ -122,19 +123,37 @@ public void SccProviderContextNotActive_IsEmpty() } [Test] - public void SccProviderContextIsActive_DefaultEmptyListFromGitExt() + public void SccProviderContextIsActive_InitializeWithActiveRepositories() { + var repoPath = "repoPath"; + var repoFactory = Substitute.For(); var context = CreateVSUIContext(true); - var gitExt = CreateGitExt(new string[0]); - var target = CreateVSGitExt(context, gitExt); + var gitExt = CreateGitExt(new[] { repoPath }); + var target = CreateVSGitExt(context, gitExt, repoFactory: repoFactory); + target.InitializeTask.Wait(); var activeRepositories = target.ActiveRepositories; - Assert.That(activeRepositories.Count(), Is.EqualTo(0)); + Assert.That(activeRepositories.Count(), Is.EqualTo(1)); + repoFactory.Received(1).Create(repoPath); } - // TODO: We can't currently test returning a non-empty list because it constructs a live LocalRepositoryModel object. - // We could move the responsibility for constructing a LocalRepositoryModel object outside of VSGitExt. + [Test] + public void ExceptionRefreshingRepositories_ReturnsEmptyList() + { + var repoPath = "repoPath"; + var repoFactory = Substitute.For(); + repoFactory.Create(repoPath).ReturnsForAnyArgs(x => { throw new Exception("Boom!"); }); + var context = CreateVSUIContext(true); + var gitExt = CreateGitExt(new[] { repoPath }); + var target = CreateVSGitExt(context, gitExt, repoFactory: repoFactory); + target.InitializeTask.Wait(); + + var activeRepositories = target.ActiveRepositories; + + repoFactory.Received(1).Create(repoPath); + Assert.That(activeRepositories.Count(), Is.EqualTo(0)); + } } static IReadOnlyList CreateActiveRepositories(IList repositoryPaths) @@ -150,17 +169,19 @@ static IReadOnlyList CreateActiveRepositories(IList return repositories.AsReadOnly(); } - static VSGitExt CreateVSGitExt(IVSUIContext context = null, IGitExt gitExt = null, IGitHubServiceProvider sp = null) + static VSGitExt CreateVSGitExt(IVSUIContext context = null, IGitExt gitExt = null, IGitHubServiceProvider sp = null, + ILocalRepositoryModelFactory repoFactory = null) { context = context ?? CreateVSUIContext(true); gitExt = gitExt ?? CreateGitExt(); sp = sp ?? Substitute.For(); + repoFactory = repoFactory ?? Substitute.For(); var factory = Substitute.For(); var contextGuid = new Guid(Guids.GitSccProviderId); factory.GetUIContext(contextGuid).Returns(context); sp.GetService().Returns(factory); sp.GetService().Returns(gitExt); - var vsGitExt = new VSGitExt(sp, factory); + var vsGitExt = new VSGitExt(sp, factory, repoFactory); vsGitExt.InitializeTask.Wait(); return vsGitExt; } @@ -169,7 +190,7 @@ static IGitExt CreateGitExt(IList repositoryPaths = null) { repositoryPaths = repositoryPaths ?? new string[0]; var gitExt = Substitute.For(); - var repoList = CreateActiveRepositories(new string[0]); + var repoList = CreateActiveRepositories(repositoryPaths); gitExt.ActiveRepositories.Returns(repoList); return gitExt; } From 7ea9d8151b6db1b54e6c0ddfe9ae17bc6bd33b09 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Mon, 29 Jan 2018 11:30:14 +0000 Subject: [PATCH 59/64] Ensure repo changed event fires even when exception Clear the list and fire repo changes event if there is an exception when reading or creating repositories (there shouldn't be). --- .../Services/VSGitExt.cs | 17 +++++++++++++++-- .../GitHub.TeamFoundation/VSGitExtTests.cs | 18 ++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index a9fa2c66ea..f2be1c85d2 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -25,6 +25,7 @@ public class VSGitExt : IVSGitExt readonly ILocalRepositoryModelFactory repositoryFactory; IGitExt gitService; + IEnumerable activeRepositories; [ImportingConstructor] public VSGitExt(IGitHubServiceProvider serviceProvider, IVSUIContextFactory factory, ILocalRepositoryModelFactory repositoryFactory) @@ -92,7 +93,6 @@ void RefreshActiveRepositories() try { ActiveRepositories = gitService?.ActiveRepositories.Select(x => repositoryFactory.Create(x.RepositoryPath)).ToList(); - ActiveRepositoriesChanged?.Invoke(); } catch (Exception e) { @@ -101,7 +101,20 @@ void RefreshActiveRepositories() } } - public IEnumerable ActiveRepositories { get; private set; } + public IEnumerable ActiveRepositories + { + get + { + return activeRepositories; + } + + private set + { + activeRepositories = value; + ActiveRepositoriesChanged?.Invoke(); + } + } + public event Action ActiveRepositoriesChanged; public Task InitializeTask { get; private set; } diff --git a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs index e8597797bd..45c7ed1059 100644 --- a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs +++ b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs @@ -76,6 +76,24 @@ public void GitExtPropertyChangedEvent_ActiveRepositoriesChangedIsFired() Assert.That(wasFired, Is.True); } + [Test] + public void ExceptionReadingActiveRepositories_ActiveRepositoriesChangedIsFired() + { + var context = CreateVSUIContext(true); + var gitExt = CreateGitExt(); + gitExt.ActiveRepositories.Returns(x => { throw new Exception("Boom!"); }); + + var target = CreateVSGitExt(context, gitExt); + + bool wasFired = false; + target.ActiveRepositoriesChanged += () => wasFired = true; + var eventArgs = new PropertyChangedEventArgs(nameof(gitExt.ActiveRepositories)); + gitExt.PropertyChanged += Raise.Event(gitExt, eventArgs); + + Assert.That(wasFired, Is.True); + Assert.That(target.ActiveRepositories, Is.Empty); + } + [Test] public void WhenUIContextChanged_ActiveRepositoriesChangedIsFired() { From a41dbc157f0c9cec8c4afc6de5bac095df213011 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Mon, 29 Jan 2018 11:35:36 +0000 Subject: [PATCH 60/64] Use IReadOnlyList and keep IVSGitExt consistent with IGitExt There's now no reason not to return a IReadOnlyList like IGitExt.ActiveRepositories. --- src/GitHub.Exports/Services/IVSGitExt.cs | 2 +- src/GitHub.TeamFoundation.14/Services/VSGitExt.cs | 4 ++-- test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/GitHub.Exports/Services/IVSGitExt.cs b/src/GitHub.Exports/Services/IVSGitExt.cs index f0944c5aca..997864add0 100644 --- a/src/GitHub.Exports/Services/IVSGitExt.cs +++ b/src/GitHub.Exports/Services/IVSGitExt.cs @@ -6,7 +6,7 @@ namespace GitHub.Services { public interface IVSGitExt { - IEnumerable ActiveRepositories { get; } + IReadOnlyList ActiveRepositories { get; } event Action ActiveRepositoriesChanged; } } \ No newline at end of file diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index f2be1c85d2..a882615fe2 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -25,7 +25,7 @@ public class VSGitExt : IVSGitExt readonly ILocalRepositoryModelFactory repositoryFactory; IGitExt gitService; - IEnumerable activeRepositories; + IReadOnlyList activeRepositories; [ImportingConstructor] public VSGitExt(IGitHubServiceProvider serviceProvider, IVSUIContextFactory factory, ILocalRepositoryModelFactory repositoryFactory) @@ -101,7 +101,7 @@ void RefreshActiveRepositories() } } - public IEnumerable ActiveRepositories + public IReadOnlyList ActiveRepositories { get { diff --git a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs index 45c7ed1059..58faecdd63 100644 --- a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs +++ b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs @@ -152,7 +152,7 @@ public void SccProviderContextIsActive_InitializeWithActiveRepositories() var activeRepositories = target.ActiveRepositories; - Assert.That(activeRepositories.Count(), Is.EqualTo(1)); + Assert.That(activeRepositories.Count, Is.EqualTo(1)); repoFactory.Received(1).Create(repoPath); } @@ -170,7 +170,7 @@ public void ExceptionRefreshingRepositories_ReturnsEmptyList() var activeRepositories = target.ActiveRepositories; repoFactory.Received(1).Create(repoPath); - Assert.That(activeRepositories.Count(), Is.EqualTo(0)); + Assert.That(activeRepositories.Count, Is.EqualTo(0)); } } From 6a494e30c7e05331a389f2342a47e1703b8b3ecc Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Mon, 29 Jan 2018 11:49:48 +0000 Subject: [PATCH 61/64] Fix merge error --- test/UnitTests/UnitTests.csproj | 1 + 1 file changed, 1 insertion(+) diff --git a/test/UnitTests/UnitTests.csproj b/test/UnitTests/UnitTests.csproj index 0e30f5a872..d2a8578c43 100644 --- a/test/UnitTests/UnitTests.csproj +++ b/test/UnitTests/UnitTests.csproj @@ -168,6 +168,7 @@ ..\..\packages\Serilog.2.5.0\lib\net46\Serilog.dll + ..\..\packages\Rothko.0.0.3-ghfvs\lib\net45\rothko.dll True From a4662955eb95127ad6f3a307f94d3696e061894c Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Mon, 29 Jan 2018 11:55:45 +0000 Subject: [PATCH 62/64] Only fire property change event if property has changed Keep the property change event pattern consistent. --- src/GitHub.TeamFoundation.14/Services/VSGitExt.cs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index a882615fe2..d57f61fbbd 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -38,7 +38,7 @@ public VSGitExt(IGitHubServiceProvider serviceProvider, IVSUIContextFactory fact context = factory.GetUIContext(new Guid(Guids.GitSccProviderId)); // Start with empty array until we have a change to initialize. - ActiveRepositories = new ILocalRepositoryModel[0]; + ActiveRepositories = Array.Empty(); if (context.IsActive && TryInitialize()) { @@ -97,7 +97,7 @@ void RefreshActiveRepositories() catch (Exception e) { log.Error(e, "Error refreshing repositories"); - ActiveRepositories = new ILocalRepositoryModel[0]; + ActiveRepositories = Array.Empty(); } } @@ -110,8 +110,11 @@ public IReadOnlyList ActiveRepositories private set { - activeRepositories = value; - ActiveRepositoriesChanged?.Invoke(); + if (value != activeRepositories) + { + activeRepositories = value; + ActiveRepositoriesChanged?.Invoke(); + } } } From 01c81b2867c12c8c94b4a348bc008359a8d06e81 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Mon, 29 Jan 2018 12:00:57 +0000 Subject: [PATCH 63/64] Clean up redundant usings --- src/GitHub.App/Services/PullRequestService.cs | 1 - src/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModel.cs | 1 - .../ViewModels/GitHubPane/PullRequestDetailViewModel.cs | 2 -- src/GitHub.Exports/Models/IBranch.cs | 4 ++-- .../Services/PullRequestSessionManager.cs | 1 - .../Services/PullRequestSessionManagerTests.cs | 2 +- test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs | 1 - 7 files changed, 3 insertions(+), 9 deletions(-) diff --git a/src/GitHub.App/Services/PullRequestService.cs b/src/GitHub.App/Services/PullRequestService.cs index bb611fa11e..d9dda83dfa 100644 --- a/src/GitHub.App/Services/PullRequestService.cs +++ b/src/GitHub.App/Services/PullRequestService.cs @@ -14,7 +14,6 @@ using System.Reactive; using System.Collections.Generic; using LibGit2Sharp; -using System.Diagnostics; using GitHub.Logging; namespace GitHub.Services diff --git a/src/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModel.cs b/src/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModel.cs index 24c9e6ff45..1125b6c211 100644 --- a/src/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModel.cs +++ b/src/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModel.cs @@ -15,7 +15,6 @@ using GitHub.Primitives; using GitHub.Services; using GitHub.VisualStudio; -using GitHub.Helpers; using ReactiveUI; using OleMenuCommand = Microsoft.VisualStudio.Shell.OleMenuCommand; diff --git a/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs b/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs index c521c2227f..67d41729b1 100644 --- a/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs +++ b/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs @@ -4,8 +4,6 @@ using System.IO; using System.Linq; using System.Reactive; -using System.ComponentModel; -using System.Reactive.Disposables; using System.Reactive.Linq; using System.Reactive.Threading.Tasks; using System.Threading.Tasks; diff --git a/src/GitHub.Exports/Models/IBranch.cs b/src/GitHub.Exports/Models/IBranch.cs index 5e7059391e..223172e8fe 100644 --- a/src/GitHub.Exports/Models/IBranch.cs +++ b/src/GitHub.Exports/Models/IBranch.cs @@ -1,5 +1,5 @@ -using GitHub.Collections; -using System; +using System; +using GitHub.Collections; namespace GitHub.Models { diff --git a/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs b/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs index 7ce6eb71b9..432f84c308 100644 --- a/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs +++ b/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs @@ -10,7 +10,6 @@ using System.Threading.Tasks; using GitHub.Extensions; using GitHub.Factories; -using GitHub.Helpers; using GitHub.InlineReviews.Models; using GitHub.Logging; using GitHub.Models; diff --git a/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionManagerTests.cs b/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionManagerTests.cs index a9cd86cb07..0f337d04c6 100644 --- a/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionManagerTests.cs +++ b/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionManagerTests.cs @@ -3,6 +3,7 @@ using System.Linq; using System.Reactive.Linq; using System.Reactive.Subjects; +using System.Reactive.Disposables; using System.Text; using System.Threading.Tasks; using GitHub.Factories; @@ -17,7 +18,6 @@ using Microsoft.VisualStudio.Utilities; using NSubstitute; using NUnit.Framework; -using System.Reactive.Disposables; namespace GitHub.InlineReviews.UnitTests.Services { diff --git a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs index 58faecdd63..85e43efdab 100644 --- a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs +++ b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs @@ -1,5 +1,4 @@ using System; -using System.Linq; using System.Threading; using System.ComponentModel; using System.Collections.Generic; From 7f8011e6988e34989f40a7777745c59abd825c57 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Mon, 29 Jan 2018 13:10:30 +0000 Subject: [PATCH 64/64] Update test to reflect no duplicate change events --- test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs index 85e43efdab..616b17e08b 100644 --- a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs +++ b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs @@ -76,10 +76,10 @@ public void GitExtPropertyChangedEvent_ActiveRepositoriesChangedIsFired() } [Test] - public void ExceptionReadingActiveRepositories_ActiveRepositoriesChangedIsFired() + public void ExceptionReadingActiveRepositories_StillEmptySoNoEvent() { var context = CreateVSUIContext(true); - var gitExt = CreateGitExt(); + var gitExt = CreateGitExt(new[] { "repoPath" }); gitExt.ActiveRepositories.Returns(x => { throw new Exception("Boom!"); }); var target = CreateVSGitExt(context, gitExt); @@ -89,8 +89,8 @@ public void ExceptionReadingActiveRepositories_ActiveRepositoriesChangedIsFired( var eventArgs = new PropertyChangedEventArgs(nameof(gitExt.ActiveRepositories)); gitExt.PropertyChanged += Raise.Event(gitExt, eventArgs); - Assert.That(wasFired, Is.True); Assert.That(target.ActiveRepositories, Is.Empty); + Assert.That(wasFired, Is.False); } [Test] @@ -205,7 +205,7 @@ static VSGitExt CreateVSGitExt(IVSUIContext context = null, IGitExt gitExt = nul static IGitExt CreateGitExt(IList repositoryPaths = null) { - repositoryPaths = repositoryPaths ?? new string[0]; + repositoryPaths = repositoryPaths ?? Array.Empty(); var gitExt = Substitute.For(); var repoList = CreateActiveRepositories(repositoryPaths); gitExt.ActiveRepositories.Returns(repoList);