Need Hooks API

[johnbfair]

I'd like to contribute a base web hook API call based on the definition here: http://developer.github.com/v3/repos/hooks/

The payloads for all of the hooks are here: https://api.github.com/hooks

I'm only targeting the Web Hook because it's the main hook that most developers looking to interact w/ GitHub programmatically would most likely leverage. Since there are so many hooks that I was planning on naming things explicitly like:

• /Apis/WebHookApi.cs
• /Entities/WebHook.cs

Additionally: Are there any contribution guidelines? Do you follow GitFlow, or is it preferable to use rebasing over merging? I also noticed that you're not using enums, is that purposeful? I was thinking of making the Supported Events an enum:

• "commit_comment",
• "create",
• "delete",
• "download",
• ...etc

[johnbfair]

Update: It looks like "public" is one of the supported events, so enum wouldn't work (might explain the overall avoidance of enums). So I'll have to rethink the enum idea.

[timerickson]

@johnbfair, go for it. I prefere PR's - will have to consider GitFlow.

I definitely prefer using enums, so if there is something that is not an enum, please submit an Issue and/or PR and let's fix it.

I've added a Contrib.md for your reference that I think pretty much covers everything else.

I'm excited to see your PR!

[timerickson]

I think it will be fine since it will become Public in the C# and redefined as public via the EnumMember(Value="public")

[johnbfair]

Good call, hadn't gotten as far as EnumMember(Value="public") yet! :+1:

I'm trying to follow the coding standards, but if there's something wrong with my PR(s) just let me know and I'll fix it. :smile:

[johnbfair]

@timerickson I believe I'm through the object generation phase: https://github.com/johnbfair/IronGitHub/tree/events The only thing I'm not 100% on is in Event.cs where I'm setting the hook name for each event individually like so:

        public string HookName
        {
            get
            {
                return "team_add";
            }
        }

It's entirely possible that we won't need the hook name itself, or that there's a better way to do it. Let me know your thoughts.

[timerickson]

So I think something more like

public abstract class EventBase { protected EventBase(SupportedEvents supportedEvent){ _supportedEvent = supportedEvent; } private readonly SupportedEvents _supportedEvent; public SupportedEvents Hook { get { return _supportedEvent; } } } public class TeamAddEvent : EventBase { public TeamAddEvent() : base(SupportedEvents.TeamAdd) { } }

[johnbfair]

Good idea. I wrote that before I went back and converted SupportedEvents to an enum. (which PS I found the rest of the enums :smile: ).

FYI if you use triple backtick + c# you can get C# syntax highlighting

// ``` c#
public string MyMethod ()
{
    return "Hello world";
}
// ```

[timerickson]

What's the status of this? Close to a PR? Wondering because there's a new PR today that would make a difficult merge for you...

On Fri, Aug 16, 2013 at 6:01 AM, John B Fair notifications@github.comwrote:

Good idea. I wrote that before I went back and converted SupportedEvents to an enum. (which PS I found the rest of the enums [image: :smile:] ).

FYI if you use triple backtick + c# you can get C# syntax highlighting

// c#public string MyMethod (){ return "Hello world";}//

— Reply to this email directly or view it on GitHubhttps://github.com/in2bits/IronGitHub/issues/6#issuecomment-22764459 .

[johnbfair]

Hey Tim, I had 2 weeks off at work but was planning to get back on this tomorrow.

Which PR were you more worried about? My work is "done", but pending Unit Tests. It sounds like PR #7 is a full overhaul of the testing system which might actually benefit me for what I'm working on. However PR #8 I've already built this out (in a slightly more robust way): https://github.com/johnbfair/IronGitHub/blob/events/IronGitHub/Apis/IssuesApi.cs

I'd like to get PR #7 integrated into what I'm doing at the moment since I think having all the tests in NUnit is a good idea. Thoughts?

[timerickson]

If you're good with it I'll just go ahead and merge 7 so you can pull that into your work. I like NUnit ;-) Though Fluent extensions are new to me (but not the concept). Works for me, especially since it rationalizes the Tests structure significantly.

On Mon, Sep 2, 2013 at 3:29 PM, John B Fair notifications@github.comwrote:

Hey Tim, I had 2 weeks off at work but was planning to get back on this tomorrow.

Which PR were you more worried about? My work is "done", but pending Unit Tests. It sounds like PR #7https://github.com/in2bits/IronGitHub/issues/7is a full overhaul of the testing system which might actually benefit me for what I'm working on. However PR #8https://github.com/in2bits/IronGitHub/issues/8I've already built this out (in a slightly more robust way): https://github.com/johnbfair/IronGitHub/blob/events/IronGitHub/Apis/IssuesApi.cs

I'd like to get PR #7 https://github.com/in2bits/IronGitHub/issues/7integrated into what I'm doing at the moment since I think having all the tests in NUnit is a good idea. Thoughts?

— Reply to this email directly or view it on GitHubhttps://github.com/in2bits/IronGitHub/issues/6#issuecomment-23680668 .

[johnbfair]

Definitely interested in the changes to the structure of the tests too! :smile: I'm also a big fan of NUnit, I'll just have to get used to the Fluent piece.

I'll get working on getting my tests to pass tomorrow. I assume there'll be some changes as I get my tests written, but the bulk of the work is done.

[timerickson]

Sounds good.

On Mon, Sep 2, 2013 at 3:37 PM, John B Fair notifications@github.comwrote:

Definitely interested in the changes to the structure of the tests too! [image: :smile:] I'm also a big fan of NUnit, I'll just have to get used to the Fluent piece.

I'll get working on getting my tests to pass tomorrow. I assume there'll be some changes as I get my tests written, but the bulk of the work is done.

— Reply to this email directly or view it on GitHubhttps://github.com/in2bits/IronGitHub/issues/6#issuecomment-23681468 .

[timerickson]

Honestly I don't see the PR #8 covered in the current IssuesApi. I think I'm going to go ahead and bring that in also.

[johnbfair]

Gotcha, looks like PR #8 retrieves the list of issues for the repo which I haven't touched.

[johnbfair]

I've merged all of the previous PRs into my events branch. Starting work on the tests again. My apologies for taking so long to get back to this! :bow:

[johnbfair]

Currently fighting the Authorization Context...I'm trying to use the new TestBase (WithGitHubApi) Authorize task and it's authorizing the authorization request properly, but when I run any other requests they aren't getting the authorization as I'd expect...

If anyone has any suggestions that'd be awesome!

[johnbfair]

Alright, after an hour of fighting it I've gotten rid of the pseudo-memoization that's happening in WithGitHubApi and it works:

        protected async Task Authorize(IEnumerable<Scopes> scopes = null)
        {
                var authorization = await Api.Authorize(
                    new NetworkCredential(IntegrationTestParameters.GitHubUsername, IntegrationTestParameters.GitHubPassword),
                    scopes,
                    "IronGithub Integration Test");
                Api.Context.Authorize(authorization);
        }

I get why we were trying to memoize here, but we're not doing it properly and it's causing my tests to crash (not passing authorization headers in my requests).

[timerickson]

I'm curious re: just how it was not passing auth headers in your requests - makes me suspicious something else is going on. Looking forward to the PR so we can push another version on NuGet!

[johnbfair]

I still think it's a good idea to memoize the auth credentials, at least for the tests. The problem is that GitHubApiBase isn't leveraging a context that can pull from the memoized list. So the call to

await Authorize(new[] { Scopes.Repo });

actually auths properly, however the subsequent call to:

var hooks = await Api.Hooks.Get(_testUsername, _testRepo);

doesn't have the proper credentials in Context.Authorization (I'm looking at GitHubApiBase Line 59-63).

[johnduhart]

The GitHubApi object is being recreated for each test, so you need to call authorize each time.

[johnbfair]

Why not do this:

    public class WithGitHubApi
    {
        protected GitHubApi Api;

        [TestFixtureSetUp]
        public void CreateGitHubApi()
        {
            Api = new GitHubApi();
        }

        /// <summary>
        /// Logs into the GitHub API using the test account for integration testing. If there is a matching authorization for the given scopes from a previous test, it is reused
        /// </summary>
        /// <param name="scopes">The scopes to request access to</param>
        protected async Task Authorize(IEnumerable<Scopes> scopes = null)
        {
            if (Api.Context.Authorization == Authorization.Anonymous || 
                (scopes != null && 
                Api.Context.Authorization.Scopes != null &&
                !Api.Context.Authorization.Scopes.Intersect(scopes).Any()))
            {
                var authorization = await Api.Authorize(
                                        new NetworkCredential(IntegrationTestParameters.GitHubUsername, IntegrationTestParameters.GitHubPassword),
                                        scopes,
                                        "IronGithub Integration Test");
                Api.Context.Authorize(authorization);
            }
        }
    }

Then I can reuse the GitHubApi and Authorization instance w/o having to reauth on each call. For Hooks I'm doing 2-3 API calls per method, so adding auth to that I'm at 3-4 per test. Just trying to keep that manageable.

[johnduhart]

The reason I didn't want to create the api at the test fixture level is because some tests fixtures have tests which vary the view the data is seen from (some tests are anonymous while others are logged in). Additionally, the API should ideally be in a clean state for each test.

[johnbfair]

It's a bit more complicated though. I don't think any of the other endpoint tests are doing any modifications (mostly Gets if I'm not mistaken). For the Hooks I'm testing that I can CRUD hooks and somehow I need to ensure that all calls are Authorized and that we're cleaning out any existing hooks before we run and after we run (in case our tests failed on a previous run).

I'm open to suggestions!

[johnduhart]

Okay, if you make that change please be sure that none of the other tests break.

[johnbfair]

Heck, I can't get my tests to run yet so no worries there! :wink:

I keep running into problems where my async Task call of Get against Hooks just hangs. Just sits there and stares at me. Mind you I've done it 3-4 other times in the same session successfully. It's almost as if making too many calls too quickly causes me to get paused by GitHub, but nothing I can detect because

    response = (HttpWebResponse) await request.GetResponseAsync();

just sits there...judging me...

[johnbfair]

@timerickson @johnduhart Have you guys noticed a problem w/ running NUnit tests and they hang on X test out of Y?

I ask because I've been trying EVERYTHING today to get these HooksTests running and at the end of the day each of the work individually, but not when run as a group. The crazy thing is: when I open Fiddler and run the tests they all pass just fine! I've got no idea what I'm doing wrong at this point, but any input is welcome.

Also, we're relying on System.Net.HttpWebRequest for sending all of our communication to GitHub. Any interest in using something more robust (and less error prone) like RestSharp? The more I read around the internets about this issue (async works w/ Fiddler running but not otherwise) I'm seeing people complain more and more about HttpWebRequest.

Thoughts? Help!

[johnbfair]

All of my code is up on my fork (under the branch events). Please pull it down and try to execute all of the HookTests.cs tests.

PS I'm at a standstill as of right now. :frowning: My tests all pass individually, but not collectively. I've also had to go through a lot of hoops to get things to work, so if you see repeated code in each of them (for instance not using the [SetUp] method properly in the tests that's why.

[timerickson]

In general, I'm sure I've seen this type of thing before, but I don't recall it specifically right now with this code or the GitHub api specifically.

RestSharp uses HttpWebRequest underneath anyway, so that should not make the difference. If you want to try and see if it magically makes a difference, go ahead, but I would probably only want to then narrow down what it is doing differently and integrate that here.

I would love to look into this myself, but can't say reliably when that might happen as my available time has dwindled drastically with new fatherly responsibilities as my 3 month old becomes a 3.5 month old, etc.

[johnbfair]

Bummer (about RestSharp). Given the amount of change I'd have to enact to get RESTSharp up I'd say it's not worth the time at the moment.

So how would you recommend I proceed? I spent 3+hrs today battling the tests (and no actual code change to get the tests to pass). I'm confident in the changes I've made, but now there's a dependency on Fiddler running to execute the tests properly. :/

Let me know what you'd like me to try next.

[timerickson]

Lemme see how it behaves for me. I should be able to try it tonight. I certainly don't want your changes to get stale, but neither do I think we should introduce a possible dependency on Fiddler ;-)

[johnbfair]

Haha no certainly not :wink: Just let me know. I'll do more googling on my side tomorrow as well.

[johnbfair]

OK, I've heard of a few people who have had success w/ removing the call to GetResponseAsync and wrapping GetResponse in a task so it can be awaited. I'll be trying that today. :thumbsup:

[johnduhart]

Just as an FYI the build server has had no problem running these tests on a daily basis without issue :/

[johnbfair]

@johnduhart Maybe because the other tests are quite flat (one call per test)? I'm doing 3-4 async calls per test, which seems to be complicating things.

I've tried calling WebRequest.GetResponse() (w/o async) and that didn't solve it. Would it be possible to pull my events branch down into your CI server and see if it can run the tests? It could be NUnit, the Resharper/VS/NUnit/TestDriven.net test runners (I've tried all 4).

One of my coworkers is pulling the repo now and is going to try to run the tests on their box and see how it goes.

[johnduhart]

Sure, can you submit a pull request with your branch? The build server will pull the branch and run it.

[johnbfair]

@johnduhart cool. Let me just get the last changes I've made checked in and i'll submit a PR.

[johnbfair]

@johnduhart do I have to actually approve the PR first? I was hoping not to commit this code till we knew it was bulletproof...

[johnduhart]

Nope, it just takes a few minutes to get recognized.

[johnbfair]

Dunno why those 11 are failing (all of the non-Hook tests pass just fine over here), but I have a feeling we might be onto a sol'n on our side.

[johnduhart]

Right now a bunch of tests are failing because testaccount.json's location was moved, I'll make a quick change on the build server to fix that but I think it should go back were it was.

[johnduhart]

New build is reporting NotFoundExceptions http://ci.johnduhart.me/viewLog.html?buildId=46&buildTypeId=IronGitHub_IronGithubCi&tab=buildResultsDiv

[johnbfair]

@johnduhart The testaccount.json was moved, that's true. I'll move it back.

The not found errors are probably because I hard coded the "in2bitstest" account. Does your test account have a fork of this repo? It must have it in order for the tests to work. I've fixed the hard coding of the account name (using IntegrationTestParameters.GitHubUsername) and will push that in one second.

[johnbfair]

@johnduhart I think I've hosed you up. I fixed the testaccount.json location (which you just changed on your CI server), which I think is what's caused our tests to fail this time around. Sorry about that (but you were right about the location needing to go back to what it once was)!

[johnduhart]

http://ci.johnduhart.me/viewLog.html?buildId=47&tab=buildResultsDiv&buildTypeId=IronGitHub_IronGithubCi

This latest build is failing due to a NotFoundException in the FixtureSetup Method.

[johnduhart]

The biggest problem I see with your unit tests right now is they are not independent and rely on a shared object.

[johnbfair]

The latest build could be failing for a few reasons. You haven't told me if your testing account has a fork of IronGitHub. Are you using "in2bitstest" as the account?

As for the "biggest problem", I've tried this in literally 200 permutations. I've had each test do its own create/delete of hooks, I've had the TestFixtures do it. I've had the SetUps do it. I've had a single method for create and a single method for delete that each tests called as they ran. I've used Task.Wait/Result instead of await. I've tried writing my own async impl of HttpRequest.

Also, if you have orphaned Hooks that exist on the IronGitHub repo for your test account you won't be able to run the tests successfully. You have to start from a clean state. I'm going to try to fix that this morning so it erases any existing hooks before executing the tests.

[johnduhart]

The apitestaccount does not have a fork of the IronGitHub, I'll make a fork of it.

[johnbfair]

I have all of the tests passing locally now (when run all at once which was previously failing).

I've commented out the Delete test and everything works. We perform deletes in SetUp and TearDown so I'm not tremendously worried about it.

[johnbfair]

Huh...search tests failed but all the Hooks passed.

http://ci.johnduhart.me/viewLog.html?buildId=50&tab=buildResultsDiv&buildTypeId=IronGitHub_IronGithubCi

[johnduhart]

You're looking at the wrong branch.

http://ci.johnduhart.me/viewLog.html?buildId=49&tab=buildResultsDiv&buildTypeId=IronGitHub_IronGithubCi

Some hook tests are failing due to a hardcoded URL. As for the search tests, I'm investigating that now as its now failing on master as well.