New method on RepositoryCommits client - Get the sha1 of a commit reference #1195

rogertinsley · 2016-03-16T23:12:33Z

Title
New method on RepositoryCommits client - Get the sha1 of a commit reference. Fixes #1189

Description

Add a sha1 method to IRepositoryCommitsClient
Add unit and integration tests for this new method
Add a sha1 method to IObservableRepositoryCommitsClients
Update AcceptHeaders to include the preview http accept header

This is my first foray into contributing to OSS. Any feedback is greatly appreciated.

Updated RepositoryCommitsClients and unit/integration tests

Added implementation for Reactive framework

shiftkey · 2016-03-16T23:22:11Z

Octokit/Clients/IRepositoryCommitsClient.cs

+        /// <param name="name">The name of the repository</param>
+        /// <param name="reference">The repository reference</param>
+        /// <returns></returns>
+        Task<string> Sha1(string owner, string name, string reference);


Could we add a verb to this method? Say something like GetSha1?

shiftkey · 2016-03-16T23:25:24Z

@rogertinsley welcome!

To be perfectly honest, the only bits of feedback I have are minor - this is really nice work!

One suggestion - if you wanted to add another integration test where you:

create a repository
make a commit through the API, get the new SHA
verify the SHA from this new endpoint matches what you got earlier

I think that'll give us enough coverage for this endpoint to close this out.

rogertinsley · 2016-03-16T23:29:26Z

Thanks @shiftkey, I'll make the changes that you've suggested 👍

ryangribble · 2016-03-17T00:21:07Z

Octokit.Tests/Clients/RepositoriesClientTests.cs

+        public class TheSha1Method
+        {
+            [Fact]
+            public async Task EnsureNonNullArguments()


Minor note that the wording should be "Ensures" rather than "Ensure". Since the test shoudl read TheGetSha1Method.EnsuresNonNullArguments() 😀

Also this test is checking Null and Empty arguments. In other places ive seen us implement this as 2 test methods eg EnsuresNonNullArguments() and EnsuresNonEmptyArguments()

ryangribble · 2016-03-17T00:21:55Z

Great first contribution 👏

I agree with @shiftkey regarding adding verb for GetSha1() and the extra integration test (look around for other examples that use the repository context helper to create a repo for the test then automatically clean it up afterwards)...

I've also raised one minor nitpick on consistency/wording in the tests

- Create repository - Call GetSha1 - Call it a second time - Verify the two responses are equal

- One unit test for NonNullArguments - Another unit test for NonEMptyArguments

M-Zuber · 2016-03-17T07:54:22Z

👏 Nice stuff!
Welcome to OSS 😄

rogertinsley · 2016-03-17T07:57:21Z

I've made the changes you both have suggested.

Are you happy with the integration test approach? I've used a similar approach to other tests in the same class, that is to use CreateTheWorld to create repo & commit. Get the Sha1 and compare that it hasn't changed.

shiftkey · 2016-03-17T08:01:45Z

Octokit.Tests/Clients/RepositoriesClientTests.cs

+        public class TheSha1Method
+        {
+            [Fact]
+            public async void EnsuresNonNullArguments()


Make this async Task as this is more test-runner friendly...

And the CI test won't fail like it currently is...

- CreateTheWorld now returns a Task<Reference> - This reference is compared with client.GetSha1

rogertinsley · 2016-03-17T08:40:18Z

I've updated CreateTheWorld to return a Task<Reference> and compare it against the GetSha1 and the class TheSha1Method methods should be async

ryangribble · 2016-03-17T08:50:40Z

Octokit.Tests/Clients/RepositoriesClientTests.cs

@@ -790,5 +790,40 @@ public void GetsCorrectUrl()
                await Assert.ThrowsAsync<ArgumentException>(() => client.EditBranch("owner", "repo", "", update));
            }
        }
+
+        public class TheSha1Method


Should be TheGetSha1Method

Thanks - updated it, I'm getting there 😄

ryangribble · 2016-03-21T12:51:10Z

This looks good!

But I noticed there are no Unit or Integration tests for the ObservableRepositoryCommitsClient.

I realise this was like that before this PR... Some parts of the code base (particularly observable implementations) don't have tests around them, but as we touch these areas ideally we should start to correct that problem. Without tests there really could be something lurking in there making the observable implementation not work and we dont want to find out from end users if possible!

So at a minimum, I'd like to see tests added for the Observable GetSha1() method that has been added... And ideally - tests for ALL the methods in this observable client could be added! 😀

If you have a look at some existing examples of Observable client unit and integration tests you will find that the unit tests of observable would have a CallsIntoClient type test (that checks the underlying regular client method is called with appropriate args), as well as EnsuresNonNullArguments and EnsuresNonEmptyArguments tests as appropriate (depending on what parameters the call takes and whether they have checks for nulls/empty string). Also a test for the Ctor guarding against a null client argument.

Im not sure where @shiftkey stands on integration tests for observable clients - given that alot are missing, and it's pretty much only ever calling the regular client anyway... perhaps all we need are unit tests for observable clients...

But in my contributions I do typically add integration tests, I pretty much copy/paste the regular client test, modify it to use the observable client and also like to write it as per below, so it's obvious the item is an observable (eventhough you could simply await it inline). This is just personal preference.

var observable = _fixture.GetSha1(x,x,x);
var sha1 = await observable;

Added tests for GetSha1

Added integration test for GetSha1 for observableclient

Check the ctor for a null IGitHubClient

rogertinsley · 2016-03-21T19:28:38Z

I've added unit and integrations tests for the ObservableRepositoryCommitsClient, specifically the GetSha1 method.

shiftkey · 2016-03-21T21:56:48Z

Im not sure where @shiftkey stands on integration tests for observable clients - given that alot are missing, and it's pretty much only ever calling the regular client anyway... perhaps all we need are unit tests for observable clients...

This is essentially my views - if the implementation is just calling the Task-based API, then additional integration tests for the Observables are just doubling up. But yes, 👍 to additional unit tests.

ryangribble · 2016-03-22T03:09:30Z

Oops! Looks like just a minor issue there with the "NotNull" test checking empty strings, and the "NonEmpty" test checking nulls! If you can just fix that up we are good to go

Also given @shiftkey's comment let's ditch the integration test for the observable implementation, since it's just calling the real client anyway. SORRY!

NoNull needs to test null strings and NonEmpty tests empty strings

rogertinsley · 2016-03-22T09:06:42Z

No problem - I've removed the integration test and updated the unit test.

I've learnt a lot about the codebase and how to contribute with this PR and you guys haven't scared me off either 😁 so I'm going to pick up some more up-for-grabs 😄

shiftkey · 2016-03-22T09:36:21Z

@rogertinsley this all looks good! I can't wait for the next one.

I'll let @ryangribble have the final word.

ryangribble · 2016-03-22T12:12:15Z

I've learnt a lot about the codebase and how to contribute with this PR and you guys haven't scared me off either

Great to hear! Bring on the next one, this one LGTM

New method on RepositoryCommits client - Get the sha1 of a commit reference

rogertinsley added 3 commits March 16, 2016 21:59

Get the SHA-1 of a commit reference

5dfe196

Updated RepositoryCommitsClients and unit/integration tests

Get the SHA-1 of a commit reference

5270b37

Added implementation for Reactive framework

added sha preview header

faa19af

rogertinsley changed the title ~~New method on RepositoryCommits client - Get the sha1 of a commit reference #1189~~ New method on RepositoryCommits client - Get the sha1 of a commit reference Mar 16, 2016

shiftkey reviewed Mar 16, 2016
View reviewed changes

Added verb to Sha1 method

d9fd0b7

ryangribble reviewed Mar 17, 2016
View reviewed changes

rogertinsley added 3 commits March 17, 2016 07:43

Added integration test to verify a new repository

64cea23

- Create repository - Call GetSha1 - Call it a second time - Verify the two responses are equal

Fixed wording & split out unit tests

705b939

- One unit test for NonNullArguments - Another unit test for NonEMptyArguments

Changed integration test name

0fa9966

shiftkey reviewed Mar 17, 2016
View reviewed changes

shiftkey added the feature label Mar 17, 2016

Updated test to compare sha1 from reference with the GetSha1 method

6d1634c

- CreateTheWorld now returns a Task<Reference> - This reference is compared with client.GetSha1

Remove async from void

ebf2792

ryangribble reviewed Mar 17, 2016
View reviewed changes

Modified the test class name to reflect the method under test

5be778b

rogertinsley added 3 commits March 21, 2016 19:13

Created ObservableRepositoryCommitsClientTests

04ec997

Added tests for GetSha1

Created ObservableRepositoryCommitsClientTests

c34d904

Added integration test for GetSha1 for observableclient

Added constructor test

e57dfa3

Check the ctor for a null IGitHubClient

rogertinsley added 2 commits March 22, 2016 09:00

Removing integration test

dc8ef18

Correcting method names

dd63d1b

NoNull needs to test null strings and NonEmpty tests empty strings

shiftkey assigned ryangribble Mar 22, 2016

ryangribble added a commit that referenced this pull request Mar 22, 2016

Merge pull request #1195 from rogertinsley/issue-1189

e147837

New method on RepositoryCommits client - Get the sha1 of a commit reference

ryangribble merged commit e147837 into octokit:master Mar 22, 2016

shiftkey mentioned this pull request Mar 24, 2016

verify correct branch in integration test #1215

Merged

nickfloyd added Type: Feature New feature or request and removed category: feature labels Oct 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New method on RepositoryCommits client - Get the sha1 of a commit reference #1195

New method on RepositoryCommits client - Get the sha1 of a commit reference #1195

rogertinsley commented Mar 16, 2016

shiftkey Mar 16, 2016

shiftkey commented Mar 16, 2016

rogertinsley commented Mar 16, 2016

ryangribble Mar 17, 2016

ryangribble commented Mar 17, 2016

M-Zuber commented Mar 17, 2016

rogertinsley commented Mar 17, 2016

shiftkey Mar 17, 2016

shiftkey Mar 17, 2016

rogertinsley commented Mar 17, 2016

ryangribble Mar 17, 2016

rogertinsley Mar 17, 2016

ryangribble commented Mar 21, 2016

rogertinsley commented Mar 21, 2016

shiftkey commented Mar 21, 2016

ryangribble commented Mar 22, 2016

rogertinsley commented Mar 22, 2016

shiftkey commented Mar 22, 2016

ryangribble commented Mar 22, 2016

New method on RepositoryCommits client - Get the sha1 of a commit reference #1195

New method on RepositoryCommits client - Get the sha1 of a commit reference #1195

Conversation

rogertinsley commented Mar 16, 2016

shiftkey Mar 16, 2016

Choose a reason for hiding this comment

shiftkey commented Mar 16, 2016

rogertinsley commented Mar 16, 2016

ryangribble Mar 17, 2016

Choose a reason for hiding this comment

ryangribble commented Mar 17, 2016

M-Zuber commented Mar 17, 2016

rogertinsley commented Mar 17, 2016

shiftkey Mar 17, 2016

Choose a reason for hiding this comment

shiftkey Mar 17, 2016

Choose a reason for hiding this comment

rogertinsley commented Mar 17, 2016

ryangribble Mar 17, 2016

Choose a reason for hiding this comment

rogertinsley Mar 17, 2016

Choose a reason for hiding this comment

ryangribble commented Mar 21, 2016

rogertinsley commented Mar 21, 2016

shiftkey commented Mar 21, 2016

ryangribble commented Mar 22, 2016

rogertinsley commented Mar 22, 2016

shiftkey commented Mar 22, 2016

ryangribble commented Mar 22, 2016