Repo.hooks()

[yegor256]

Let's create method Repo.hooks() and an interface Hooks, see http://developer.github.com/v3/repos/hooks/

[dmarkov]

The budget here is 30 mins, which is exactly how much time will be paid for, when the task is completed

[dmarkov]

@longtimeago please pick this up, and keep in mind these instructions. Any technical questions - ask right here

[longtimeago]

@yegor256 Please, clarify the scope of this task:

1. Should I define all operations in Hooks (iterate, get, create, edit, delete)?
2. Should I define all attributes in Hook (name, url, createdAt, deletedAt, config, events ...)?
3. Should I define stub test classes? Should I add todo to these classes for implementation tests and business methods or tests only? Maybe it would be better to add one todo per each business method, e.g. "implement iterate() method, unit and integration tests for it"? As for me it's better approach, as you have business and test in one branch in one puzzle and you can be sure that code works and tested appropriately.
4. Should I define stub implementation classes? Mocks?

Thank you!

[yegor256]

The rule I tried to follow in this library is that its interfaces represent all REST methods of Github API, but no more than that. When Github API returns JSON our interfaces don't parse them. We just return JsonObject, like it's done in Issue.json(), Comment.json(), etc. When parsing is required, we have *.Smart classes. They implement that supplementary parse operations.

So, in this case we should implement iterate, get, create, edit and delete. Since all of them are REST methods.

So, no, we don't need to define methods in Hook for attributes retrieval. We just provide json(), and a Smart decorator.

I would recommend to start with:

1. implement RtHooksTest the way you think it should work
2. create interfaces
3. create stub classes that throw UnsupportedOperationException
4. skip tests created in step 1 and mark them with @todo puzzles
5. add RtHooksITCase, skeleton only, with one puzzle asking to implement it

The most important task here for you is in step 1.

[longtimeago]

Thank you for explanation! Concept of Smarts is clear for me. But many questions arose from words ambiguity

to 1 (implement RtHooksTest the way you think it should work):

• does it mean to provide working implementation of all tests for all RtHooks methods (iterate, get, create, edit and delete)?
• it can't be done before step 2 and 3

to 3 (create stub classes that throw UnsupportedOperationException)

• does it mean to create Smart stub for Hooks?
• should I create puzzle for Smart implementation?

to 4 (skip tests created in step 1 and mark them with @todo puzzles)

• should I create separate puzzle for each method (test)? or one for all?
• what should be the scope for these puzzles? implement business logic inside RtHooks?

I'm just trying to find out definition-of-done to have less reworks.

Thank you for your patience!

[yegor256]

Questions are welcome! Let me explain by example. How I would do it, step by step.

I need to start with a unit test. Here it goes:

class RtRepoTest {
  void providesAccessToHooks() {
    RtRepo repo = new RtRepo(
      Mockito.mock(Github.class),
      new FakeRequest()
    );
    MatcherAssert.assertThat(
      repo.hooks(),
      Matchers.notNullValue()
    );
  }
}

Commit. Does it work? No. Do I have enough time to fix it? Well, yes, I've spent just a minute so far, but I have 29 more minutes. OK, let's go deeper, let's fix the test. In order to do it I should create an interface Hooks, and add a method hooks to Repo interface. Here it comes:

public interface Hooks {
}

and the method (also in the Repo interface, of course):

final class RtRepo {
  public Hooks hooks() {
    return new RtHooks(this.entry, this);
  }
}

Commit. Run unit tests again. Do they pass? No, I can't even compile, since RtHooks is absent. OK, do I have time? Yes, still have time. I'm in TDD domain, so let's start with a test:

class RtHooksTest {
  void canFetchEmptyListOfHooks() {    
    MkContainer container = new MkGrizzlyContainer().next(
      new MkAnswer.Simple(HttpURLConnection.HTTP_OK, "[]")
    ).start();
    Hooks hooks = new RtHooks(
      new JdkRequest(container.home()),
      Mockito.mock(Repo.class)
    );
    MatcherAssert.assertThat(
      hooks.iterate(),
      Matchers.emptyIterable()
    );
  }
}

Commit. Does it pass? No, since there is no class RtHooks and there is no method iterate() in it (and it's not in the interface). OK, do I have more time? Yes, I do, let's create it. Here it goes (and define it in the interface):

final class RtHooks {
  RtHooks() {
    // ...
  }
  public Iterable<Hook> iterate() {
    return Collections.emptyList();
  }
}

Commit. Tests pass? No, not yet. I don't have MkHooks. OK, let's start with a test too. Let's create MkHooksTest, with a simple scenario:

class MkHooksTest {
  void canFetchEmptyListOfHooks() {    
    Hooks hooks = MkHooksTest.repo().hooks();
    MatcherAssert.assertThat(
      hooks.iterate(),
      Matchers.emptyIterable()
    );
  }
}

And I have to create MkHooks class and return an empty list of hooks from its iterate() method.

Commit. So, do we compile and pass now? Yes, the entire code base compiles and passes all tests. Good. What now? Now I'm running out of time, since I need to send it all through code review and it may be bounced. So, let's reserve some time for it. Besides that, I didn't cover the entire scope, since Hooks should have many more features, as the ticket explains. Ok, let's get back to the unit test and add some puzzles:

class RtHooksTest {
  void canFetchEmptyListOfHooks() {    
    // ...
  }
  /**
   * @todo #122 RtHooks should iterate multiple hooks. Let's implement
   *   a test here and a method of RtHooks. The method should iterate
   *   multiple hooks. See how it's done in other classes with GhPagination.
   *   When done, remove this puzzle and Ignore annotation from the method.
   */
  @Ignore
  void canFetchNonEmptyListOfHooks() {    
  }
  /**
   * @todo #122 RtHooks should be able to get a single Hook. Let's implement
   *   a test here and a method get() of RtHooks. The method should fetch a single
   *   hook. See how it's done in other classes, using Rexsl request/response.
   *   When done, remove this puzzle and Ignore annotation from the method.
   */
  @Ignore
  void canFetchSingleHook() {    
  }
}

Commit. I would add more test methods there, one per each usage scenario (not just per method, but per scenario, like it's done with fetching empty list and non-empty list). No need to do the same for MkHooksTest, since when new methods will be introduced to MkHooks they automatically will be tested. No need to make extra tasks/puzzles for them. OK, am I done? There should be also an integration test. OK, let's create one:

class RtHooksITCase {
  /**
   * @todo #122 RtHooks should be able to fetch a list of hooks from a real
   *   Github repository. a single Hook.  
   *   When done, remove this puzzle and Ignore annotation from the method.
   */
  @Ignore
  void canFetchAllHooks() {
  }
}

Commit. Looks like I'm ready to wrap up. Did I cover the entire scope of the ticket? Looks like it. OK, I push and make a pull request.

[longtimeago]

Wow! Thank you very much for this explanation! This post should definitely be saved in HOWTO. Many cases become clear. I propose you to commit this task by yourself, because you did almost all for it :)

[yegor256]

Nah, it's just a sketch :) You go on and implement it. Will see how will work in your case. I hope you get the idea :)

[longtimeago]

How do you think, where Smart implementation should be mentioned? Should it?

[yegor256]

I think it's not required at the moment at all. When/if we see in the future that it may help, we'll create a new ticket, explain why and what should be done, and do it there. For this task I don't see where you need such a decorator.

[yegor256]

thanks, done!

[dmarkov]

@longtimeago Much obliged! I've added 30 mins to your account in payment "DA2085861"