From 33f75ed1494fe7a92b9630a2a065505d70a9d3fd Mon Sep 17 00:00:00 2001 From: Henrik Andersson Date: Sun, 24 Feb 2019 21:48:55 +1000 Subject: [PATCH] Add pagination support to Migrations client (#1949) * Add pagination to *MigrationsClient * Add unit tests for *MigrationsClient * Add integration tests for *MigrationsClient * Fix the broken tests --- .../Clients/IObservableMigrationsClient.cs | 15 ++++- .../Clients/ObservableMigrationsClient.cs | 14 ++++- .../Clients/MigrationsClientTests.cs | 56 ++++++++++++++++++- .../Clients/MigrationsClientTests.cs | 27 ++++++++- .../ObservableMigrationsClientTests.cs | 20 ++++++- Octokit/Clients/IMigrationsClient.cs | 16 +++++- Octokit/Clients/MigrationsClient.cs | 20 ++++++- 7 files changed, 157 insertions(+), 11 deletions(-) diff --git a/Octokit.Reactive/Clients/IObservableMigrationsClient.cs b/Octokit.Reactive/Clients/IObservableMigrationsClient.cs index 6174cdfe..f95c7861 100644 --- a/Octokit.Reactive/Clients/IObservableMigrationsClient.cs +++ b/Octokit.Reactive/Clients/IObservableMigrationsClient.cs @@ -36,9 +36,22 @@ namespace Octokit.Reactive /// /// The organization of which to list migrations. /// List of most recent s. - IObservable> GetAll( + IObservable GetAll( string org); + /// + /// Gets the list of the most recent migrations of the the organization. + /// + /// + /// https://developer.github.com/v3/migration/migrations/#get-a-list-of-migrations + /// + /// The organization of which to list migrations. + /// Options for changing the API response + /// List of most recent s. + IObservable GetAll( + string org, + ApiOptions options); + /// /// Get the status of a migration. /// diff --git a/Octokit.Reactive/Clients/ObservableMigrationsClient.cs b/Octokit.Reactive/Clients/ObservableMigrationsClient.cs index 93f471af..39ce9aec 100644 --- a/Octokit.Reactive/Clients/ObservableMigrationsClient.cs +++ b/Octokit.Reactive/Clients/ObservableMigrationsClient.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Reactive; using System.Reactive.Threading.Tasks; +using Octokit.Reactive.Internal; namespace Octokit.Reactive { @@ -15,6 +16,7 @@ namespace Octokit.Reactive public class ObservableMigrationsClient : IObservableMigrationsClient { private readonly IMigrationsClient _client; + private readonly IConnection _connection; /// /// Instantiates a GitHub Migrations API client. @@ -25,6 +27,7 @@ namespace Octokit.Reactive Ensure.ArgumentNotNull(client, nameof(client)); _client = client.Migration.Migrations; + _connection = client.Connection; } /// @@ -50,9 +53,16 @@ namespace Octokit.Reactive /// /// The organization of which to list migrations. /// List of most recent s. - public IObservable> GetAll(string org) + public IObservable GetAll(string org) { - return _client.GetAll(org).ToObservable(); + return GetAll(org, ApiOptions.None); + } + + public IObservable GetAll(string org, ApiOptions options) + { + Ensure.ArgumentNotNull(options, nameof(options)); + + return _connection.GetAndFlattenAllPages(ApiUrls.EnterpriseMigrations(org), null, AcceptHeaders.MigrationsApiPreview, options); } /// diff --git a/Octokit.Tests.Integration/Clients/MigrationsClientTests.cs b/Octokit.Tests.Integration/Clients/MigrationsClientTests.cs index 3ed7d8bd..d4556fad 100644 --- a/Octokit.Tests.Integration/Clients/MigrationsClientTests.cs +++ b/Octokit.Tests.Integration/Clients/MigrationsClientTests.cs @@ -1,7 +1,6 @@ using System; using System.Collections.Generic; using System.Linq; -using System.Security.Cryptography; using System.Threading; using System.Threading.Tasks; using Octokit; @@ -68,6 +67,61 @@ public class MigrationsClientTests : IDisposable Assert.NotEqual(0, migrations.Count); } + [IntegrationTest] + public async Task ReturnsCorrectCountOfMigrationsWithoutStart() + { + var options = new ApiOptions + { + PageCount = 1, + PageSize = 1 + }; + + var migrations = await _gitHub.Migration.Migrations.GetAll(_orgName, options); + + Assert.Equal(1, migrations.Count); + } + + [IntegrationTest] + public async Task ReturnsCorrectCountOfMigrationsWithStart() + { + var options = new ApiOptions + { + PageCount = 1, + PageSize = 1, + StartPage = 2 + }; + + var migrations = await _gitHub.Migration.Migrations.GetAll(_orgName, options); + + Assert.Equal(1, migrations.Count); + } + + [IntegrationTest] + public async Task ReturnsDistinctMigrationsBasedOnStartPage() + { + var startOptions = new ApiOptions + { + PageCount = 1, + PageSize = 1, + StartPage = 1 + }; + + var firstPage = await _gitHub.Migration.Migrations.GetAll(_orgName, startOptions); + + var skipStartOptions = new ApiOptions + { + PageCount = 1, + PageSize = 1, + StartPage = 2 + }; + var secondPage = await _gitHub.Migration.Migrations.GetAll(_orgName, skipStartOptions); + + Assert.Equal(1, firstPage.Count); + Assert.Equal(1, secondPage.Count); + Assert.NotEqual(firstPage[0].Id, secondPage[0].Id); + Assert.NotEqual(firstPage[0].Repositories, secondPage[0].Repositories); + } + [IntegrationTest] public async Task CanGetMigration() { diff --git a/Octokit.Tests/Clients/MigrationsClientTests.cs b/Octokit.Tests/Clients/MigrationsClientTests.cs index a1d610a2..5dac3601 100644 --- a/Octokit.Tests/Clients/MigrationsClientTests.cs +++ b/Octokit.Tests/Clients/MigrationsClientTests.cs @@ -55,10 +55,32 @@ namespace Octokit.Tests.Clients client.GetAll("fake"); - connection.Received().Get>( + connection.Received().GetAll( Arg.Is(u => u.ToString() == "orgs/fake/migrations"), null, - AcceptHeaders.MigrationsApiPreview); + AcceptHeaders.MigrationsApiPreview, + Args.ApiOptions); + } + + [Fact] + public void RequestsCorrectUrlApiOptions() + { + var connection = Substitute.For(); + var client = new MigrationsClient(connection); + + var options = new ApiOptions + { + PageCount = 1, + PageSize = 1 + }; + + client.GetAll("fake", options); + + connection.Received().GetAll( + Arg.Is(u => u.ToString() == "orgs/fake/migrations"), + null, + AcceptHeaders.MigrationsApiPreview, + options); } [Fact] @@ -68,6 +90,7 @@ namespace Octokit.Tests.Clients var client = new MigrationsClient(connection); await Assert.ThrowsAsync(() => client.GetAll(null)); + await Assert.ThrowsAsync(() => client.GetAll("fake", null)); await Assert.ThrowsAsync(() => client.GetAll("")); } } diff --git a/Octokit.Tests/Reactive/ObservableMigrationsClientTests.cs b/Octokit.Tests/Reactive/ObservableMigrationsClientTests.cs index 90be692f..d06bb6c8 100644 --- a/Octokit.Tests/Reactive/ObservableMigrationsClientTests.cs +++ b/Octokit.Tests/Reactive/ObservableMigrationsClientTests.cs @@ -1,7 +1,9 @@ using System; using System.Collections.Generic; +using System.Linq; using NSubstitute; using Octokit.Reactive; +using Octokit.Reactive.Internal; using Xunit; namespace Octokit.Tests.Reactive @@ -46,7 +48,23 @@ namespace Octokit.Tests.Reactive var client = new ObservableMigrationsClient(github); client.GetAll("fake"); - github.Migration.Migrations.Received(1).GetAll("fake"); + github.Received().Migration.Migrations.GetAll("fake", Args.ApiOptions); + } + + [Fact] + public void CallsIntoClientApiOptions() + { + var github = Substitute.For(); + var client = new ObservableMigrationsClient(github); + var options = new ApiOptions + { + PageCount = 1, + PageSize = 1 + }; + + client.GetAll("fake", options); + + github.Received().Migration.Migrations.GetAll("fake", options); } } diff --git a/Octokit/Clients/IMigrationsClient.cs b/Octokit/Clients/IMigrationsClient.cs index 6980483b..7e9cca5b 100644 --- a/Octokit/Clients/IMigrationsClient.cs +++ b/Octokit/Clients/IMigrationsClient.cs @@ -35,10 +35,22 @@ namespace Octokit /// /// The organization of which to list migrations. /// List of most recent s. - [ExcludeFromPaginationApiOptionsConventionTest("TODO: Implement pagination for this method")] - Task> GetAll( + Task> GetAll( string org); + /// + /// Gets the list of the most recent migrations of the the organization. + /// + /// + /// https://developer.github.com/v3/migration/migrations/#get-a-list-of-migrations + /// + /// The organization of which to list migrations. + /// Options for changing the API response + /// List of most recent s. + Task> GetAll( + string org, + ApiOptions options); + /// /// Get the status of a migration /// diff --git a/Octokit/Clients/MigrationsClient.cs b/Octokit/Clients/MigrationsClient.cs index d4475207..659eef83 100644 --- a/Octokit/Clients/MigrationsClient.cs +++ b/Octokit/Clients/MigrationsClient.cs @@ -1,4 +1,5 @@ using System.Collections.Generic; +using System.Linq; using System.Threading.Tasks; namespace Octokit @@ -47,13 +48,28 @@ namespace Octokit /// /// The organization of which to list migrations. /// List of most recent s. - public async Task> GetAll(string org) + public async Task> GetAll(string org) + { + return await GetAll(org, ApiOptions.None); + } + + /// + /// Gets the list of the most recent migrations of the the organization. + /// + /// + /// https://developer.github.com/v3/migration/migrations/#get-a-list-of-migrations + /// + /// The organization of which to list migrations. + /// Options for changing the API response + /// List of most recent s. + public async Task> GetAll(string org, ApiOptions options) { Ensure.ArgumentNotNullOrEmptyString(org, nameof(org)); + Ensure.ArgumentNotNull(options, nameof(options)); var endpoint = ApiUrls.EnterpriseMigrations(org); - return await ApiConnection.Get>(endpoint, null, AcceptHeaders.MigrationsApiPreview).ConfigureAwait(false); + return await ApiConnection.GetAll(endpoint, null, AcceptHeaders.MigrationsApiPreview, options).ConfigureAwait(false); } ///