Pull-Request-Squash-Commit #1245

Sarmad93 · 2016-04-05T09:15:54Z

Implementation of #1236 opened by @ryangribble for Preview Support for Pull Request Merge Api recently introduced by github.

shiftkey · 2016-04-05T11:44:33Z

Octokit.Tests/Reactive/ObservablePullRequestsClientTests.cs

@@ -225,7 +225,7 @@ public class TheMergeMethod
            [Fact]
            public void MergesPullRequest()
            {
-                var mergePullRequest = new MergePullRequest { CommitMessage = "fake commit message" };
+                var mergePullRequest = new MergePullRequest { CommitMessage = "fake commit message", CommitTitle = "fake title" };


Is this a necessary change?

No this is not necessary. but i thought i should also make changes in ObservablePullRequestsClientTests.cs .I wasn't sure that i should initialize property here or leave it as it was.. what do you say should i remove this?

It looks like we're not asserting anything about this object, so I think we should revert this change. 🆒?

yes you are right..i should revert it now. 👍

Sarmad93 · 2016-04-05T18:05:44Z

oops look like i did some thing wrong and haven't pushed required changes

shiftkey · 2016-04-05T18:57:56Z

Octokit.Tests.Integration/Clients/PullRequestsClientTests.cs

+        var merge = new MergePullRequest { CommitMessage = "fake message", CommitTitle = "fake title", Squash = true };
+        var result = await _fixture.Merge(Helper.UserName, _context.RepositoryName, pullRequest.Number, merge);
+
+        Assert.True(result.Merged);


Could we assert something here about the commit title or message versus what we actually committed, to indicate this was squashed?

sure i will add assert statement here. 😃

One suggestion - look at the commit:

var commit = await _github.Repository.Commit.Get(_context.RepositoryOwner, _context.RepositoryName, result.Sha); // TODO: something interesting goes here ...

shiftkey · 2016-04-05T18:58:49Z

@Sarmad93 thanks for starting this off. I added a couple of comments to help us complete this feature and verify we're doing everything right...

Sarmad93 · 2016-04-05T19:02:56Z

@shiftkey thanks for guiding me. Sure we wil complete this one 👍

Sarmad93 · 2016-04-06T10:15:12Z

hi @shiftkey i am doing changes in integration test. when i ran build i saw that integration test were actually not running(skipping) because of some error

OCTOKIT_GITHUBUSERNAME and OCTOKIT_GITHUBPASSWORD environment variables are not set.

how can i make sure my test are passing? do i need to make a separate testing account for this?
thanks.

ryangribble · 2016-04-06T13:06:41Z

how can i make sure my test are passing? do i need to make a separate testing account for this?
thanks.

@Sarmad93 yeah you should create a test account because the integration tests do things like create repos and other things. Generally they try to clean up after themselves where possible, but definitely you want a separate test account to do this under.

Refer to the integration test sectino in Contributors Guide for information about how to run the tests (essentially there's a powershell script to help you define those environment variables)

Sarmad93 · 2016-04-06T13:17:33Z

@ryangribble i will test these against testing account but i am pushing some changes could you please review these especially header functionality? i am not sure how this header parameter functionality works. i am stuck at this one need little help.

Sarmad93 · 2016-04-06T13:50:37Z

sorry for incomplete commits this smartgit github client really has some weird behavior 😠 doesn't always add all files to staging area.

ryangribble · 2016-04-06T14:51:27Z

Im not in a position to pull your code down and run it myself just now, but eyeballing your changes, yes that looks correct in specifying the accept header on the ApiConnection.Put<T> call. The real test is whether your integration tests succeed, and the merged PR was squashed rather than merged!

However on the unit test mock verification you shouldnt use AcceptHeaders.xxxxx there (instead hardcode the expected header string) - we want the unit test to independently verify the implementation code, so we dont want to "share" that AcceptHeaders helper between implementation and tests (otherwise a bug/typo in the helper class will still pass the unit test!). Same way we manually check the Url that is called rather than use the ApiUrl helper method 😀

Regarding the integration test assertion, is there also a way to verify the commitTitle specified was correctly applied?

Sarmad93 · 2016-04-06T15:13:19Z

@ryangribble thanks for the code review 😆 yes i need to test these on testing account now.
Regarding AcceptHeaders.xxxxx in unit test, you explained very well i need to pass header as string in parameter 👍

Sarmad93 · 2016-04-08T13:01:35Z

i have configured my test account for integration test by running script in scripts\ configure-integration-tests.ps1 now i am getting some other errors from Octokit\GitHubClient.cs . i thinks my username and password is not updating from script. how can i resolve this?

Sarmad93 · 2016-04-08T13:08:10Z

i am stuck in this testing phase 😞

shiftkey · 2016-04-08T13:19:50Z

@Sarmad93 looking at those errors, I think something wasn't set correctly. You can run the same script again to view and update the values that the tests need.

shiftkey · 2016-04-14T02:20:00Z

Just putting this back to WIP until we can confirm the integration test setup script is correct.

ryangribble · 2016-04-19T09:58:11Z

Hey @Sarmad93, I'd definitely like to ensure these new GitHub features make it into the next octokit release so if time is an issue for you to get back onto this (totally understandable!) just let me know if you want someone to pick it up from here 😀

Sarmad93 · 2016-04-19T13:02:10Z

@ryangribble yes i have been busy for last days and last time i remember i was stuck on integration test setup and i wasn't sure where to add parameter(header) in post request so i stopped working on this 😫 got tired. you can assign this to someone else. Really sorry i could not made it.

ryangribble · 2016-04-19T13:20:25Z

If you want to keep working on it you're more than welcome to!

It looks like you were at the point where we needed you to confirm your integration test settings... Easiest way is just to close visual studio, rerun the powershell script and ensure they are all set correctly, then have another go at running integration tests

Sarmad93 · 2016-04-19T13:34:32Z

@ryangribble i made a test account and set the script against testing account but after setting up the script i was getting these errors. why these test are failing? most of the test in PullRequestsClientTests.csare failing..they made me annoyed 😃 an exception is thrown from connection.cs file. something is not setting up or there is some thing wrong in response 😕

Sarmad93 · 2016-04-19T13:45:21Z

@ryangribble also when i build project in visual studio 2015 i get this error also

shiftkey · 2016-04-19T22:11:04Z

@Sarmad93 some of the integration tests depend on the account being in a certain state - I've tried to keep it as runnable as possible for everyone, but differences do creep in which means they can fail.

If you want to focus on just your test, I'd suggest running it from in Visual Studio.

devkhan · 2016-04-21T05:13:33Z

SolutionInfo.cs

-    internal static class AssemblyVersionInformation
-    {
+namespace System {
+    internal static class AssemblyVersionInformation {


This is unintended, right?

I think this is a side-effect of .\build FormatCode - I'm tracking this in shiftkey/Octokit.CodeFormatter#2

Sarmad93 · 2016-05-04T15:55:02Z

unit tests + convention tests build is passing locally but not here? 😕

devkhan · 2016-05-04T16:25:40Z

@Sarmad93 Your has conflicts with master, and that's why the CI builds are not running. Try to rebase your branch.

Sarmad93 · 2016-05-04T16:32:27Z

@devkhan still not building?

devkhan · 2016-05-04T16:36:39Z

What did you do exactly?

Sarmad93 · 2016-05-04T16:40:02Z

@devkhan i did only this git rebase master and all that happened as you can see above :D

devkhan · 2016-05-04T16:44:40Z

Did you updated you master from upstream first? You have to do that as that will fetch all the latest commits made in the main repo's master. Then you have to do the following to resolve conflicts:

git checkout <your_branch>
git rebase master
# Resolve any conflicts that may arise manually
# Then marked them resolved by:
git add .
# Then continue rebasing:
git rebase --continue
# Finally do a force push:
git push --force origin <your_branch>

Let me what happens after doing the above steps. 👍

Sarmad93 · 2016-05-04T17:08:22Z

@devkhan on step 4(git rebase --continue) i am getting this. 😕

devkhan · 2016-05-04T18:24:21Z

And if there are any conflicts while cherry-picking, resolve them and commit too.

Sarmad93 · 2016-05-04T19:59:45Z

@devkhan sir first i would to thank you so much for your time and efforts 👍 i think this pr has gone to some fatal condition.. appveyor build is generating some strange errors 😆

Sarmad93 · 2016-05-04T20:06:25Z

@devkhan i have to redo this in a single commit. maybe it will solve the problem.

devkhan · 2016-05-05T05:21:22Z

The reasons of those errors are the diffs which are inserted into the files on conflicts. I also think that a new PR may be better or if the conversation here is relevant, you can go actually delete your branch locally and then recreate it from the latest upstream master, and then re-do your work on top of it. And finally, do a force push.

Sarmad93 · 2016-05-17T11:11:45Z

i want to get back on this. i have free time for a while 😄 . I need a little help.
regarding passing the Header in put request i think i need to pass it in PullRequestClient.Merge . This merge method returns PullRequestMerge model object. In return statement the put method takes 4 input arguments. The one which confuses me the 3rd parameter string twoFactorAuthenticationCode Should i pass null here? or dummy text or i should update the position of parameters in overloaded put in IConnection class so that make Accept Header third input parameter and leave twoFactorAuthenticationCode without initializing it.

shiftkey · 2016-05-17T13:48:20Z

@Sarmad93

The one which confuses me the 3rd parameter string twoFactorAuthenticationCode Should i pass null here?

You can specify null fine here.

or i should update the position of parameters in overloaded put in IConnection class so that make Accept Header third input parameter and leave twoFactorAuthenticationCode without initializing it.

Don't reorder the parameters as part of this PR. I'm not really sure why twoFactorAuthenticationCode is so pervasive in the IConnection signatures but let's avoid that for now...

Sarmad93 · 2016-05-19T09:44:48Z

@shiftkey could you please review changes i have made especially for Accept header?
Integration tests are not working for me. so i have tested it on creating new project and using access token. it works fine for me and squashes more than one commit into a single commit. kindly if you pull this code and run the integration test by yourself so that it could be verified that it is working as it intended to be 😄

shiftkey · 2016-05-19T13:58:17Z

Octokit.Tests/OctoKit.Tests-NetCore45.csproj

@@ -184,6 +184,10 @@
      <Project>{c8bc13b6-3fa3-4716-827d-e7706f976fe1}</Project>
      <Name>Octokit-NetCore45</Name>
    </ProjectReference>
+    <ProjectReference Include="..\Octokit\Octokit.csproj">


Not sure how this crept in, but this project reference shouldn't be needed 🔥 🔥 🔥

Sarmad93 · 2016-05-19T14:19:01Z

@shiftkey Really sorry for that... i accidentally add all the files to staging area without observing closely...i will fix this and push new changes..sorry again if this made you angry..

shiftkey · 2016-05-19T14:22:15Z

@Sarmad93 I'm not angry, I just like to delete code whenever possible 🔥

shiftkey · 2016-05-19T14:28:29Z

@Sarmad93 I'm happy with that integration test - let's wrap this up and get it merged!

Sarmad93 · 2016-05-19T17:45:13Z

@shiftkey done.

shiftkey · 2016-05-20T08:34:29Z

@Sarmad93 excellent, thanks!

shiftkey · 2016-05-20T08:35:40Z

Sarmad93 · 2016-05-20T16:39:25Z

@shiftkey @ryangribble as always thanks for your code review and guidance 👍 you guyz helped me to finish it to end.
@devkhan sir you taught me how to resolve merge/rebase conflict rightly,appreciate your efforts.

devkhan · 2016-05-20T16:57:04Z

@Sarmad93 Glad I could help. You don't need to call me "Sir", we're all here to help each other. And I'm happy this PR is finally merged. Keep up the good work. 👍

shiftkey reviewed Apr 5, 2016
View reviewed changes

shiftkey changed the title ~~Pull-Request-Squash-Commit~~ [WIP] Pull-Request-Squash-Commit Apr 14, 2016

devkhan reviewed Apr 21, 2016
View reviewed changes

Sarmad93 added 7 commits May 5, 2016 00:40

Pull-Request-Squash-Commit

ac010b2

minor_formatting

eba30b4

revert incorrect change

12d7f38

reverting incorret changes in previous commit

aafec54

some fixes in test

45735fd

some fixes in tests

71def4d

Some Changes

0d70159

Sarmad93 force-pushed the Squash_Commit branch from 7b9fcb1 to 0d70159 Compare May 4, 2016 19:47

fixing issues after merge/rebase and made some changes for accept header

35d450d

shiftkey reviewed May 19, 2016
View reviewed changes

fixed some mistake

dce0be2

shiftkey changed the title ~~[WIP] Pull-Request-Squash-Commit~~ Pull-Request-Squash-Commit May 20, 2016

shiftkey merged commit 3f39302 into octokit:master May 20, 2016

shiftkey mentioned this pull request May 20, 2016

Implement Pull Request "Squash Commit" support (API preview) #1236

Closed

M-Zuber mentioned this pull request May 30, 2016

stand alone reaction client #1335

Merged

Pull-Request-Squash-Commit #1245

Pull-Request-Squash-Commit #1245

Conversation

Sarmad93 commented Apr 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sarmad93 commented Apr 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiftkey commented Apr 5, 2016

Sarmad93 commented Apr 5, 2016

Sarmad93 commented Apr 6, 2016

ryangribble commented Apr 6, 2016

Sarmad93 commented Apr 6, 2016

Sarmad93 commented Apr 6, 2016

ryangribble commented Apr 6, 2016

Sarmad93 commented Apr 6, 2016

Sarmad93 commented Apr 8, 2016

Sarmad93 commented Apr 8, 2016

shiftkey commented Apr 8, 2016

shiftkey commented Apr 14, 2016

ryangribble commented Apr 19, 2016

Sarmad93 commented Apr 19, 2016

ryangribble commented Apr 19, 2016

Sarmad93 commented Apr 19, 2016 • edited Loading

Sarmad93 commented Apr 19, 2016

shiftkey commented Apr 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sarmad93 commented May 4, 2016

devkhan commented May 4, 2016

Sarmad93 commented May 4, 2016

devkhan commented May 4, 2016

Sarmad93 commented May 4, 2016

devkhan commented May 4, 2016

Sarmad93 commented May 4, 2016

devkhan commented May 4, 2016

Sarmad93 commented May 4, 2016

Sarmad93 commented May 4, 2016

devkhan commented May 5, 2016

Sarmad93 commented May 17, 2016 • edited Loading

shiftkey commented May 17, 2016

Sarmad93 commented May 19, 2016

Choose a reason for hiding this comment

Sarmad93 commented May 19, 2016 • edited Loading

shiftkey commented May 19, 2016

shiftkey commented May 19, 2016

Sarmad93 commented May 19, 2016

shiftkey commented May 20, 2016

shiftkey commented May 20, 2016

Sarmad93 commented May 20, 2016

devkhan commented May 20, 2016

Sarmad93 commented Apr 19, 2016 •

edited

Loading

Sarmad93 commented May 17, 2016 •

edited

Loading

Sarmad93 commented May 19, 2016 •

edited

Loading