Add Page property to control pagination. Add asynchronous support for GetCommits, GetCommit, and GetCommitDiff #754
Conversation
…rt for GetCommits, GetCommit, and GetCommitDiff
| using var context = await RepositoryClientTestsContext.CreateAsync(commitCount: 5); | ||
|
|
||
| var defaultBranch = context.Project.DefaultBranch; | ||
| var since = DateTime.UtcNow; |
There was a problem hiding this comment.
Unless I'm missing something, the test here will collect no commit whatsoever. To truly test this, you should create a few (e.g. 5) commits over a small period of time, say one every 200 ms, and then list commits from the creation time of one of the middle commits.
There was a problem hiding this comment.
This function is similar to GetCommitsSince. The common code has been refactored and is now shared between both functions. Should I also update GetCommitsSince to align with these changes? Also, I agree I need to at least add a new test to ensure it is thoroughly tested.
| // Assert | ||
| var lastRequestQueryString = context.Context.LastRequest.RequestUri.Query; | ||
|
|
||
| Assert.That(lastRequestQueryString, Does.Contain($"since={expectedSinceValue}")); |
There was a problem hiding this comment.
This assertion has little value. What's more relevant is that we do get only the commits from since...
| // Assert | ||
| var lastRequestQueryString = context.Context.LastRequest.RequestUri.Query; | ||
|
|
||
| Assert.That(lastRequestQueryString, Does.Not.Contain("since=")); |
There was a problem hiding this comment.
Again, I'm not sure this assertion is relevant... I would probably remove that test altogether.
| // Arrange | ||
| using var context = await RepositoryClientTestsContext.CreateAsync(commitCount: 5); | ||
|
|
||
| var defaultBranch = context.Project.DefaultBranch; |
There was a problem hiding this comment.
Again, I would stagger the creation commits over a short period of time and validate that specifying until works as expected.
| @@ -170,6 +293,178 @@ public async Task GetCommitsDoesntIncludeUntilWhenNotSpecified() | |||
| Assert.That(lastRequestQueryString, Does.Not.Contain("until=")); | |||
| } | |||
|
|
|||
| [Test] | |||
| [NGitLabRetry] | |||
| public async Task GetCommitsDoesntIncludeUntilWhenNotSpecifiedAsync() | |||
| var lastRequestQueryString = context.Context.LastRequest.RequestUri.Query; | ||
|
|
||
| NameValueCollection queryParameters = HttpUtility.ParseQueryString(lastRequestQueryString); | ||
| Assert.That(queryParameters, Does.Not.Contain("page")); |
There was a problem hiding this comment.
I would prefer that the behavior, from a user's perspective, be tested. With a 5-commit setup and per_page=2 you could test what happens when page
- is not set
- is < 3
- is >= 3
|
|
||
| [Test] | ||
| [NGitLabRetry] | ||
| public async Task GetCommitsDoesntIncludePageWhenNotSpecifiedAsync() |
Added a
Pageproperty to control pagination, helping to avoid unnecessary data fetches.Introduced asynchronous support for the following methods: GetCommits, GetCommit, and GetCommitDiff.