This repository was archived by the owner on Jun 21, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
SimpleApiClient Unit Tests #38
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
ad65938
Add unit tests for SimpleApiClient
haacked 6d87b3b
Remove redundant boolean comparison
haacked 56ef142
:art: Replace conditional with return statement
haacked fc55a67
Add unit tests for SimpleApiClientFactory
haacked c5ac231
Avoid potential race condition
haacked c571592
Move everything to the GitHub.Api folder
haacked 8bba2b6
Switch to ConcurrentDictionary for the api client cache
shana b1e012a
Merge master into haacked/add-unit-tests
shana File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| using System; | ||
| using GitHub.Api; | ||
| using GitHub.Primitives; | ||
| using GitHub.Services; | ||
| using GitHub.VisualStudio; | ||
| using NSubstitute; | ||
| using Xunit; | ||
|
|
||
| public class SimpleApiClientFactoryTests | ||
| { | ||
| public class TheCreateMethod | ||
| { | ||
| [Fact] | ||
| public void CreatesNewInstanceOfSimpleApiClient() | ||
| { | ||
| var program = new Program(); | ||
| var enterpriseProbe = Substitute.For<IEnterpriseProbeTask>(); | ||
| var wikiProbe = Substitute.For<IWikiProbe>(); | ||
| var factory = new SimpleApiClientFactory( | ||
| program, | ||
| new Lazy<IEnterpriseProbeTask>(() => enterpriseProbe), | ||
| new Lazy<IWikiProbe>(() => wikiProbe)); | ||
|
|
||
| var client = factory.Create("https://github.com/github/visualstudio"); | ||
|
|
||
| Assert.Equal("https://github.com/github/visualstudio", client.OriginalUrl); | ||
| Assert.Equal(HostAddress.GitHubDotComHostAddress, client.HostAddress); | ||
| Assert.Same(client, factory.Create("https://github.com/github/visualstudio")); // Tests caching. | ||
| } | ||
| } | ||
|
|
||
| public class TheClearFromCacheMethod | ||
| { | ||
| [Fact] | ||
| public void RemovesClientFromCache() | ||
| { | ||
| var program = new Program(); | ||
| var enterpriseProbe = Substitute.For<IEnterpriseProbeTask>(); | ||
| var wikiProbe = Substitute.For<IWikiProbe>(); | ||
| var factory = new SimpleApiClientFactory( | ||
| program, | ||
| new Lazy<IEnterpriseProbeTask>(() => enterpriseProbe), | ||
| new Lazy<IWikiProbe>(() => wikiProbe)); | ||
|
|
||
| var client = factory.Create("https://github.com/github/visualstudio"); | ||
| factory.ClearFromCache(client); | ||
|
|
||
| Assert.NotSame(client, factory.Create("https://github.com/github/visualstudio")); | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,165 @@ | ||
| using System; | ||
| using System.Threading.Tasks; | ||
| using GitHub.Api; | ||
| using GitHub.Primitives; | ||
| using GitHub.Services; | ||
| using NSubstitute; | ||
| using Octokit; | ||
| using Xunit; | ||
|
|
||
| public class SimpleApiClientTests | ||
| { | ||
| public class TheGetRepositoryMethod | ||
| { | ||
| [Fact] | ||
| public async Task RetrievesRepositoryFromWeb() | ||
| { | ||
| var gitHubHost = HostAddress.GitHubDotComHostAddress; | ||
| var gitHubClient = Substitute.For<IGitHubClient>(); | ||
| var repository = new Repository(42); | ||
| gitHubClient.Repository.Get("github", "visualstudio").Returns(Task.FromResult(repository)); | ||
| var enterpriseProbe = Substitute.For<IEnterpriseProbeTask>(); | ||
| var wikiProbe = Substitute.For<IWikiProbe>(); | ||
| var client = new SimpleApiClient( | ||
| gitHubHost, | ||
| "https://github.com/github/visualstudio", | ||
| gitHubClient, | ||
| new Lazy<IEnterpriseProbeTask>(() => enterpriseProbe), | ||
| new Lazy<IWikiProbe>(() => wikiProbe)); | ||
|
|
||
| var result = await client.GetRepository(); | ||
|
|
||
| Assert.Equal(42, result.Id); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task RetrievesCachedRepositoryForSubsequentCalls() | ||
| { | ||
| var gitHubHost = HostAddress.GitHubDotComHostAddress; | ||
| var gitHubClient = Substitute.For<IGitHubClient>(); | ||
| var repository = new Repository(42); | ||
| gitHubClient.Repository.Get("github", "visualstudio") | ||
| .Returns(_ => Task.FromResult(repository), _ => { throw new Exception("Should only be called once."); }); | ||
| var enterpriseProbe = Substitute.For<IEnterpriseProbeTask>(); | ||
| var wikiProbe = Substitute.For<IWikiProbe>(); | ||
| var client = new SimpleApiClient( | ||
| gitHubHost, | ||
| "https://github.com/github/visualstudio", | ||
| gitHubClient, | ||
| new Lazy<IEnterpriseProbeTask>(() => enterpriseProbe), | ||
| new Lazy<IWikiProbe>(() => wikiProbe)); | ||
| await client.GetRepository(); | ||
|
|
||
| var result = await client.GetRepository(); | ||
|
|
||
| Assert.Equal(42, result.Id); | ||
| } | ||
| } | ||
|
|
||
| public class TheHasWikiMethod | ||
| { | ||
| [Theory] | ||
| [InlineData(WikiProbeResult.Ok, true)] | ||
| [InlineData(WikiProbeResult.Failed, false)] | ||
| [InlineData(WikiProbeResult.NotFound, false)] | ||
| public async Task ReturnsTrueWhenWikiProbeReturnsOk(WikiProbeResult probeResult, bool expected) | ||
| { | ||
| var gitHubHost = HostAddress.GitHubDotComHostAddress; | ||
| var gitHubClient = Substitute.For<IGitHubClient>(); | ||
| var repository = CreateRepository(42, true); | ||
| gitHubClient.Repository.Get("github", "visualstudio").Returns(Task.FromResult(repository)); | ||
| var enterpriseProbe = Substitute.For<IEnterpriseProbeTask>(); | ||
| var wikiProbe = Substitute.For<IWikiProbe>(); | ||
| wikiProbe.ProbeAsync(repository) | ||
| .Returns(_ => Task.FromResult(probeResult), _ => { throw new Exception("Only call it once"); }); | ||
| var client = new SimpleApiClient( | ||
| gitHubHost, | ||
| "https://github.com/github/visualstudio", | ||
| gitHubClient, | ||
| new Lazy<IEnterpriseProbeTask>(() => enterpriseProbe), | ||
| new Lazy<IWikiProbe>(() => wikiProbe)); | ||
| await client.GetRepository(); | ||
|
|
||
| var result = client.HasWiki(); | ||
|
|
||
| Assert.Equal(expected, result); | ||
| Assert.Equal(expected, client.HasWiki()); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void ReturnsFalseWhenWeHaveNotRequestedRepository() | ||
| { | ||
| var gitHubHost = HostAddress.GitHubDotComHostAddress; | ||
| var gitHubClient = Substitute.For<IGitHubClient>(); | ||
| var enterpriseProbe = Substitute.For<IEnterpriseProbeTask>(); | ||
| var wikiProbe = Substitute.For<IWikiProbe>(); | ||
| var client = new SimpleApiClient( | ||
| gitHubHost, | ||
| "https://github.com/github/visualstudio", | ||
| gitHubClient, | ||
| new Lazy<IEnterpriseProbeTask>(() => enterpriseProbe), | ||
| new Lazy<IWikiProbe>(() => wikiProbe)); | ||
|
|
||
| var result = client.HasWiki(); | ||
|
|
||
| Assert.False(result); | ||
| } | ||
| } | ||
|
|
||
| public class TheIsEnterpriseMethod | ||
| { | ||
| [Theory] | ||
| [InlineData(EnterpriseProbeResult.Ok, true)] | ||
| [InlineData(EnterpriseProbeResult.Failed, false)] | ||
| [InlineData(EnterpriseProbeResult.NotFound, false)] | ||
| public async Task ReturnsTrueWhenEnterpriseProbeReturnsOk(EnterpriseProbeResult probeResult, bool expected) | ||
| { | ||
| var gitHubHost = HostAddress.GitHubDotComHostAddress; | ||
| var gitHubClient = Substitute.For<IGitHubClient>(); | ||
| var repository = CreateRepository(42, true); | ||
| gitHubClient.Repository.Get("github", "visualstudio").Returns(Task.FromResult(repository)); | ||
| var enterpriseProbe = Substitute.For<IEnterpriseProbeTask>(); | ||
| enterpriseProbe.ProbeAsync(Args.Uri) | ||
| .Returns(_ => Task.FromResult(probeResult), _ => { throw new Exception("Only call it once"); }); | ||
| var wikiProbe = Substitute.For<IWikiProbe>(); | ||
| var client = new SimpleApiClient( | ||
| gitHubHost, | ||
| "https://github.com/github/visualstudio", | ||
| gitHubClient, | ||
| new Lazy<IEnterpriseProbeTask>(() => enterpriseProbe), | ||
| new Lazy<IWikiProbe>(() => wikiProbe)); | ||
| await client.GetRepository(); | ||
|
|
||
| var result = client.IsEnterprise(); | ||
|
|
||
| Assert.Equal(expected, result); | ||
| Assert.Equal(expected, client.IsEnterprise()); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void ReturnsFalseWhenWeHaveNotRequestedRepository() | ||
| { | ||
| var gitHubHost = HostAddress.GitHubDotComHostAddress; | ||
| var gitHubClient = Substitute.For<IGitHubClient>(); | ||
| var enterpriseProbe = Substitute.For<IEnterpriseProbeTask>(); | ||
| var wikiProbe = Substitute.For<IWikiProbe>(); | ||
| var client = new SimpleApiClient( | ||
| gitHubHost, | ||
| "https://github.com/github/visualstudio", | ||
| gitHubClient, | ||
| new Lazy<IEnterpriseProbeTask>(() => enterpriseProbe), | ||
| new Lazy<IWikiProbe>(() => wikiProbe)); | ||
|
|
||
| var result = client.IsEnterprise(); | ||
|
|
||
| Assert.False(result); | ||
| } | ||
| } | ||
|
|
||
| private static Repository CreateRepository(int id, bool hasWiki) | ||
| { | ||
| return new Repository("", "", "", "", "", "", "", id, new User(), "", "", "", "", "", false, false, 0, 0, 0, "", | ||
| 0, null, DateTimeOffset.Now, DateTimeOffset.Now, new RepositoryPermissions(), new User(), null, null, false, | ||
| hasWiki, false); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the above block because we were accessing
cacheoutside of the lock here. In theory, ifClearFormCacheis called after we checkcontains(line 32) but before we execute thereturn(line 33), we could get an exception here because the item could be removed from thecache.If we're concerned about performance here, we could consider switching to the
ConcurrentDictionary, but I'm not sure this is a hot spot. Thoughts @shana?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm almost certain I did this because Create will never get called at the same time as ClearFromCache, and the lock in Create is not to protect Create from ClearFromCache, but to have only the first instance that comes into Create trigger the creation, and keep everyone else from never even reaching the lock.
When the team explorer home page loads, there's a series of requests that get triggered to go through Create, all at the same time - one for each section and navigation item. If there's already an api client, great, return that, but if there isn't, whoever gets to the lock first gets to create it. Since the section gets created first and there's a slight delay between it and the nav items being created, that means that potentially the nav items never even hit the lock and just return immediately.
I need to confirm this, and maybe change the names on these locks to be more clear about what their role is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like
ConcurrentDictionarywould be useful for this then.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it might be. I'm looking at it now, and
ConcurrentDictionaryhas this interesting remark on its MSDN page (https://msdn.microsoft.com/en-us/library/ee378677(v=vs.110).aspx):If you call GetOrAdd simultaneously on different threads, addValueFactory may be called multiple times, but its key/value pair might not be added to the dictionary for every call.That means that it can potentially create multiple instances of
SimpleApiClientand then discard them, returning only the first one created (or the first one inserted, whichever comes, erm, first). Given that the SimpleApiClient ctor doesn't do anything, that should be fine.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm fine with that behavior because creating them is cheap. Especially if it's discarded. Locks are less cheap.