KittehOrg / KittehIRCClientLib

An IRC client library in Java
https://kitteh.dev/kicl/
MIT License
146 stars 34 forks source link

Create a super interface for each of the event types #251

Open mechjacktv opened 5 years ago

mechjacktv commented 5 years ago

I was trying to test code that depends on ChannelMessageEvent tonight and there was no easy way to fake ChannelMessageEvent without extending the concrete class. My code depends on several of the interfaces it implements, but because there is no super interface that pulls them together for ChannelMessageEvent to implement I'm extending.

I'd like to see all of the event classes roll up the interfaces they implement into one super interface, one for each of the event types so future testing isn't so hacky.

bendem commented 5 years ago

can't you instantiate the ChannelMessageEvent class? There is no need to extend it since there is no behavior in it.

mechjacktv commented 5 years ago

Absolutely and ultimately that’s what I’m doing, but I had to extend it first to get the behavior I needed for testing.

This class lives at the edge of my application and I want to write unit tests that validate that I’m calling it with expected arguments. This class doesn’t expose the message that was passed into sendReply or validate that sendReply was called at all (nor should it), but I want to write tests against my code that check for both.

It’s not about just having an instance to pass in as a dependency but that I have an instance that exposes how my code interacted with it. If I had an interface to code against I could easily do that and not have to worry about providing ChannelMessageEvent’s dependencies which are irrelevant to my tests.

mbax commented 5 years ago

This week, I'm quite busy and I don't have the time to put in the thought for considering this. So, in the mean time: Have you considered mocking it (and if so, why does it not meet your goals)? I've had a lot of fun with mockito for testing things within the library.

mechjacktv commented 5 years ago

That’s exactly what I want to do. It’s easier to mock an interface than a concrete class (you have more options). Moving from v4 of KICL to v5 also broke Mockito (I think, but am not sure, it’s related to the module support added, which is fine because I prefer mocking with dynamic proxies anyway).

bendem commented 5 years ago

Can you show a concrete example of what you are trying to do?

mechjacktv commented 5 years ago

What I have to do today:

private static class TestableChannelMessageEvent extends ChannelMessageEvent {

  private String replyMessage;

  public TestableChannelMessageEvent(Client client, List<ServerMessage> originalMessages,
      User user, Channel channel, String message) {
    super(client, originalMessages, user, channel, message);
    this.replyMessage = null;
  }

  public void sendReply(String message) {
    this.replyMessage = message;
  }

  public String getReplyMessage() {
    return this.replyMessage;
  }

}

@Test
public void myTestMethod() {
  Client client = (Client) Proxy.newProxyInstance(getClass().getClassLoader(),
      new Class[] { Client.class }, (proxy, method, args) -> null);
  User client = (User) Proxy.newProxyInstance(getClass().getClassLoader(),
      new Class[] { User.class }, (proxy, method, args) -> null);
  Channel channel = (Channel) Proxy.newProxyInstance(getClass().getClassLoader(),
      new Class[] { Channel.class }, (proxy, method, args) -> null);
  TestableChannelMessageEvent event = new TestableChannelMessageEvent(client,
      new ArrayList<ServerMessage>(), user, channel, "Arbitrary-1");
  SubjectUnderTest subjectUnderTest = new SubjectUnderTest(event);

  String testInput = "Arbitrary-2";
  subjectUnderTest.doSomething(testInput);

  assertEquals(String.format(SubjectUnderTest.MESSAGE_FORMAT, testInput),
      event.getReplyMessage());
}

What I would like to do:

@Test
public void myTestMethod() {
  final String[] replyMessage = new String[1];
  ChannelMessageEventInterface event = (ChannelMessageEventInterface) Proxy
      .newProxyInstance(getClass().getClassLoader(),
          new Class[] { ChannelMessageEventInterface.class }, (proxy, method, args) -> {
              if("sendReply".equals(method.getName())) {
                replyMessage[0] = args[0];
              }
              return null
          });
  SubjectUnderTest subjectUnderTest = new SubjectUnderTest(event);

  String testInput = "Arbitrary-1";
  subjectUnderTest.doSomething(testInput);

  assertEquals(String.format(SubjectUnderTest.MESSAGE_FORMAT, testInput), replyMessage[0]);
}
mbax commented 5 years ago

KICL tests with mockito so it can't be broken for that reason. My guess is that you don't have checker framework in your dependencies, which totally will make mockito fall over (found that out myself while testing tonight!).

This code worked fine for me as an example "does my event listener reply":

    public void test(ChannelMessageEvent event) {
        event.sendReply("YAY");
    }

    @Test
    public void testpls() {
        ChannelMessageEvent event = Mockito.mock(ChannelMessageEvent.class);
        this.test(event);
        Mockito.verify(event, Mockito.times(1)).sendReply("YAY");
        Mockito.verify(event, Mockito.times(0)).sendReply("BOO");
    }
mechjacktv commented 5 years ago

You are right that I don't have checker framework as a dependency because I'm not using directly. If that's the solution I still submit something about the module implementation is broken.

If your answer is that I have to use a tool like Mockito or extend your code to add testing behavior to write tests against your code that's fine. I would just like to not depend on concrete implementations if I don't have to thus the issue being opened.

mbax commented 5 years ago

I wonder if changing from provided to compile on checker-qual would make things behave?

[not available for dev stuff until maybe Sunday]

mechjacktv commented 5 years ago

To be clear, make Mockito work was not the goal of my issue. Mockito breaking certainly put things on my radar, but at the end of the day, to use this library requires directly depending on concrete implementations and not interfaces. This will always have an adverse impact on testability.