Add reactions to issue, commit comment and review payload #1405

martinscholz83 · 2016-06-23T07:29:10Z

Add reactions into payloads for issue, commit comment and review payloads.

ryangribble · 2016-06-23T11:33:38Z

Octokit.Tests/Reactive/ObservableIssuesClientTests.cs

@@ -117,9 +117,9 @@ public class TheGetAllForRepositoryMethod
                    && d["sort"] == "created"
                    && d["filter"] == "assigned"), Arg.Any<string>())
                .Returns(Task.Factory.StartNew<IApiResponse<List<Issue>>>(() => firstPageResponse));
-            gitHubClient.Connection.Get<List<Issue>>(secondPageUrl, Arg.Any<Dictionary<string,string>>(), null)
+            gitHubClient.Connection.Get<List<Issue>>(secondPageUrl, Arg.Any<Dictionary<string,string>>(), Arg.Is<string>(s => s == "application/vnd.github.squirrel-girl-preview"))


You can simplify Arg.Is<string>(s => s == "application/vnd.github.squirrel-girl-preview") to just "application/vnd.github.squirrel-girl-preview"

ryangribble · 2016-06-23T11:43:28Z

Octokit.Tests.Integration/Clients/RepositoryCommentsClientTests.cs

+
+            Assert.True(retrieved.Id > 0);
+            Assert.Equal(1, retrieved.Reactions.TotalCount);
+            Assert.Equal(0, retrieved.Reactions.Plus1);


Given we had issues with the Plus1 and Minus1 on the other reactions stuff, It'd be good to ensure our integration test covers deserializing all reaction summary fields.

How had would it be to have a unique value for each of the reactions, eg 5x Plus1's, 4x Minus1's, 3x Hooray, 2x Heart, 1x Laugh, 0x Confused ?

That i have proposed in an old comment in Stand-Alone-Reaction-Client but it was rejected because of not necessary 😐

Something like this ?

Confused = 0, Laugh = 1, Heart = 2, Hooray = 3, [Parameter(Value = "+1")] Plus1 = 4, [Parameter(Value = "-1")] Minus1 = 5,

I mean have that number of each reaction on an issue then the integration test can retrieve that issue and check each one is desetialzed properly by asserting the unique number

Can you give me an example. I think I have a Blackout 😵

i meant that we would setup an issue comment somewhere on github that has 5 users react with ":confused:" and 4 users react with ":heart:" and 3 users react with ":+1:" etc, then the integration test would check each of these.

var retrieved = await _github.Repository.Comment.Get("octokit", "octokit.net", commentId); Assert.Equals(5, received.Reactions.Confused); Assert.Equals(4, received.Reactions.Heart); Assert.Equals(3, received.Reactions.Plus1); etc

But it was just a thought and it's not a particularly good one because it cant be setup in code easily (as you would need up to 5 individual test users to be able to create the reactions) and even setting one up manually involves co-ordination of multiple people and is then also able to start failing if any other user ever added an unwanted reaction to it!

So let's just settle for the integration test

creating a repository/issue/comment (you are already doing this)

adding 1 reaction of each type to it (currently you only add 1 reactoin type, so we dont know if we are failing to deserialize any of the other fields in the reaction summary part of the payload)

assert each reaction in the response payload had a count of 1 and total had a count of 6

at least that tests all fields deserialized ok

Ok. Then i would implement loops like in #1402 for reaction types.

ryangribble · 2016-07-04T11:14:18Z

Looking good! For consistency, it would be great if you could add an integration test that tests all reaction types, to the original IssueComment reaction payload work that was done on previous PR? 😀

…eactions-payload

martinscholz83 · 2016-07-04T12:07:18Z

Do you mean here
https://github.com/octokit/octokit.net/blob/master/Octokit.Tests.Integration/Clients/IssueCommentsClientTests.cs#L21

ryangribble · 2016-07-04T12:13:50Z

Yeah I reckon replacing that test with your one that creates a repo/issue/comment, then adds each reaction type today it, then retrieves them and checks the fields

…eactions-payload # Conflicts: # Octokit.Tests.Integration/Clients/PullRequestReviewCommentsClientTests.cs # Octokit.Tests/Clients/PullRequestReviewCommentsClientTests.cs

martinscholz83 · 2016-07-06T14:29:32Z

unit tests are failing after merge. should i rollback?

martinscholz83 · 2016-07-06T14:31:17Z

Is there something more to do with this request?

shiftkey · 2016-07-07T01:02:37Z

unit tests are failing after merge. should i rollback?

It looks like some upstream changes needed to have the preview API applied - this is the commit which fixes it for me, feel free to cherry-pick it in:

shiftkey@7e94dbc

Looking at the integration tests now to confirm this is all that's needed...

shiftkey · 2016-07-07T01:16:28Z

Octokit.Tests.Integration/Clients/IssuesClientTests.cs

@@ -230,6 +254,14 @@ public IssuesClientTests()
        Assert.True(retrieved.Any(i => i.Number == issue2.Number));
        Assert.True(retrieved.Any(i => i.Number == issue3.Number));
        Assert.True(retrieved.Any(i => i.Number == issue4.Number));
+        var issue = retrieved[0];
+        Assert.Equal(4, issue.Reactions.TotalCount);


The reactions created above are one-per-issue. So we only expect 1 here, not 4. Or perhaps we can simplify the test?

shiftkey · 2016-07-07T03:11:45Z

I'm seeing a few integration tests fail with this exception:

Octokit.ForbiddenException : Insufficient scopes for reacting to this Issue.

Examples:

IssuesClientTests.CanDeserializeIssue
PullRequestReviewCommentReactionsClientTests.CanCreateReaction

Not sure whether this is related to my setup or something I need to investigate further.

shiftkey · 2016-07-07T03:16:05Z

Octokit.Tests.Integration/Clients/IssueCommentsClientTests.cs

-    [IntegrationTest]
-    public async Task CanDeserializeIssueCommentWhenGettingAllForRepository()
-    {
-        var comments = await _issueCommentsClient.GetAllForRepository("alfhenrik-test", "repo-with-issue-comment-reactions");


Somewhat related - we have other tests here which use this repository, but this repository doesn't seem to exist any more cc @alfhenrik

…eactions-payload # Conflicts: # Octokit.Tests.Integration/Clients/IssueCommentsClientTests.cs

martinscholz83 · 2016-07-07T07:14:37Z

@shiftkey i have cherry-pick your commit. test are now passing 👍

martinscholz83 · 2016-07-07T07:49:54Z

These failing integration test was my mistake 😯

Also applied preview header to a couple of repositoryId based calls that were added by another PR

ryangribble · 2016-07-07T12:02:20Z

There are still some failing unit tests with missing preview header. Also I noticed a couple of new repositoryId based calls from other merged PRs now also needed the preview header applied.

If you cherry pick this commit TattsGroup@1185da2 it should hopefully get everything green 👍

Also applied preview header to a couple of repositoryId based calls that were added by another PR

ryangribble · 2016-07-07T13:08:16Z

~~And here is a commit showing what I meant with tidying up the reaction payloads tests, handling Get and GetAll methods and covering all reaction types:~~
TattsGroup@e319f0e

~~Something like this could be done for the other 3 clients (Issues, PullRequestReviewComments, CommitComments)~~

Actually ive almost got all of them done, Ill send a PR to your branch 😀

…ForIssue and GetAllForRepository calls

This reverts commit a6179b0.

…it.net into TattsGroup-reactions-tests

Tatts group reactions tests

martinscholz83 · 2016-07-07T15:30:32Z

I wasn't able to merge your request. So i have to create a branch of yours, resolve conflicts, and then create a pull request internally. Very confusing 😕. But now all should be fine

ryangribble · 2016-07-07T15:44:54Z

Not sure what you mean as the PR merged ok?

Anyway hopefully it's all good now. Can you run the new integration tests to confirm they work for you?

martinscholz83 · 2016-07-07T17:02:40Z

I think the conflict came from your pull request. Because in the meantime i have made a change in my branch:see_no_evil: Tests run perfectly!!

ryangribble · 2016-07-08T10:29:03Z

woohoo!

martinscholz83 · 2016-07-08T10:33:39Z

🎉 🎉

martinscholz83 added 7 commits June 21, 2016 10:02

add reactions to issue response payload

b06097b

add reactions to commit comment payload

71e5eab

add reactions to review comment payload

1f9aa88

create tests for issue client

6a2ad51

tests for commitcomment client

0f8f345

tests for pull request review comment

3cd92ef

change observable tests

93b4d98

ryangribble reviewed Jun 23, 2016
View reviewed changes

simplify strings

30cbd8e

ryangribble reviewed Jun 23, 2016
View reviewed changes

martinscholz83 added 2 commits June 23, 2016 13:56

remove unnecessary clients

3c88311

change integration tests to retrieve all reaction types

05b451b

Merge remote-tracking branch 'refs/remotes/octokit/master' into add-r…

3ad2800

…eactions-payload

martinscholz83 added 3 commits July 4, 2016 14:28

create integration test for issue comment client

6552b2e

Merge remote-tracking branch 'refs/remotes/octokit/master' into add-r…

3c636cf

…eactions-payload # Conflicts: # Octokit.Tests.Integration/Clients/PullRequestReviewCommentsClientTests.cs # Octokit.Tests/Clients/PullRequestReviewCommentsClientTests.cs

fix merge conflicts

e1c97fe

shiftkey reviewed Jul 7, 2016
View reviewed changes

martinscholz83 and others added 3 commits July 7, 2016 08:29

Merge remote-tracking branch 'refs/remotes/octokit/master' into add-r…

75ae58d

…eactions-payload # Conflicts: # Octokit.Tests.Integration/Clients/IssueCommentsClientTests.cs

fix merge conflicts

730dcd9

gets tests passing again

e588c29

fix some reaction integration tests

43411b8

Fixup unit tests wth preview accepts header

1185da2

Also applied preview header to a couple of repositoryId based calls that were added by another PR

Fixup unit tests wth preview accepts header

0a7bfc6

Also applied preview header to a couple of repositoryId based calls that were added by another PR

ryangribble and others added 10 commits July 7, 2016 15:17

Rework reaction payload tests for IssueComments to handle Get, GetAll…

6535255

…ForIssue and GetAllForRepository calls

[WIP] reaction payload tests

a6179b0

Rework reaction payload tests for IssueComments to handle Get, GetAll…

bd10c35

…ForIssue and GetAllForRepository calls

Rework reaction payload tests for Issues client

50cd04c

Rework reaction payload tests for PullRequestReviews client

9528faa

Rework reaction payload tests for CommitComments client

6770990

revert maddin's changes

8850126

Revert "[WIP] reaction payload tests"

86bc30e

This reverts commit a6179b0.

Merge branch 'reactions-tests' of https://github.com/TattsGroup/octok…

1118db3

…it.net into TattsGroup-reactions-tests

Merge pull request #6 from maddin2016/TattsGroup-reactions-tests

d4dc131

Tatts group reactions tests

ryangribble merged commit 72e30a7 into octokit:master Jul 8, 2016

ryangribble added this to the Reactions Support milestone Jul 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add reactions to issue, commit comment and review payload #1405

Add reactions to issue, commit comment and review payload #1405

martinscholz83 commented Jun 23, 2016

ryangribble Jun 23, 2016

ryangribble Jun 23, 2016

martinscholz83 Jun 23, 2016

martinscholz83 Jun 23, 2016 •

edited

Loading

ryangribble Jun 23, 2016

martinscholz83 Jun 23, 2016

ryangribble Jun 23, 2016 •

edited

Loading

martinscholz83 Jun 24, 2016

ryangribble commented Jul 4, 2016

martinscholz83 commented Jul 4, 2016

ryangribble commented Jul 4, 2016

martinscholz83 commented Jul 6, 2016

martinscholz83 commented Jul 6, 2016

shiftkey commented Jul 7, 2016

shiftkey Jul 7, 2016

shiftkey commented Jul 7, 2016

shiftkey Jul 7, 2016

martinscholz83 commented Jul 7, 2016

martinscholz83 commented Jul 7, 2016

ryangribble commented Jul 7, 2016

ryangribble commented Jul 7, 2016 •

edited

Loading

martinscholz83 commented Jul 7, 2016

ryangribble commented Jul 7, 2016

martinscholz83 commented Jul 7, 2016

ryangribble commented Jul 8, 2016

martinscholz83 commented Jul 8, 2016

Add reactions to issue, commit comment and review payload #1405

Add reactions to issue, commit comment and review payload #1405

Conversation

martinscholz83 commented Jun 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinscholz83 Jun 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryangribble Jun 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryangribble commented Jul 4, 2016

martinscholz83 commented Jul 4, 2016

ryangribble commented Jul 4, 2016

martinscholz83 commented Jul 6, 2016

martinscholz83 commented Jul 6, 2016

shiftkey commented Jul 7, 2016

Choose a reason for hiding this comment

shiftkey commented Jul 7, 2016

Choose a reason for hiding this comment

martinscholz83 commented Jul 7, 2016

martinscholz83 commented Jul 7, 2016

ryangribble commented Jul 7, 2016

ryangribble commented Jul 7, 2016 • edited Loading

martinscholz83 commented Jul 7, 2016

ryangribble commented Jul 7, 2016

martinscholz83 commented Jul 7, 2016

ryangribble commented Jul 8, 2016

martinscholz83 commented Jul 8, 2016

martinscholz83 Jun 23, 2016 •

edited

Loading

ryangribble Jun 23, 2016 •

edited

Loading

ryangribble commented Jul 7, 2016 •

edited

Loading