grpc / grpc-java

The Java gRPC implementation. HTTP/2 based RPC
https://grpc.io/docs/languages/java/
Apache License 2.0
11.47k stars 3.85k forks source link

Make GrpcServerRule more reusable #4171

Closed ensonic closed 6 years ago

ensonic commented 6 years ago

See #2160 for some context. All the linked examples assume one mocks the service that is tested, which sounds weird to me (see https://github.com/grpc/grpc-java/blob/master/examples/src/test/java/io/grpc/examples/helloworld/HelloWorldClientTest.java) My grpc service talks to other grpc services and I'd like to mock out those. My Dagger module injects those secondary grpc service stubs and hence in the Test dagger module I'll need to provide mocked stubs and this is when things seem to fall apart.

I understand the suggestion to mock the impl instead (again see #2160), but I am not sure how to actually make this work. I have this code:

@Module
public class XyzTestModule {
    protected static final AbcGrpc.AbcImplBase abcImpl =
      mock(AbcGrpc.AbcImplBase.class, delegatesTo(new AbcGrpc.AbcImplBase() {}));

  @Provides
  BindableService provideXzyGrpcService(Datastore datastore) {
    GrpcServerRule grpcServerRule = new GrpcServerRule().directExecutor();
    // FIXME: need to somehow invoke before(), but this is a hack
    try {
      Method before = grpcServerRule.getClass().getDeclaredMethod("before");
      before.setAccessible(true);
      before.invoke(grpcServerRule);
    } catch (IllegalAccessException | InvocationTargetException | NoSuchMethodException e) {
      e.printStackTrace();
    }
    grpcServerRule.getServiceRegistry().addService(abcImpl);
    AbcGrpc.AbcBlockingStub abcClient =  AbcGrpc.newBlockingStub(grpcServerRule.getChannel());

    return new XyzService(datastore, abcClient);
  }
}

bur of course the use of reflection is a terrible hack. It looks like I'll have to replicate most of what is in https://github.com/grpc/grpc-java/blob/master/testing/src/main/java/io/grpc/testing/GrpcServerRule.java, right? So maybe the code in GrpcServerRule can be made more resuable. E.g. Datastore has a LocalDatastoreHelper, where I can start()/stop() in my tests @BeforeClass/@AfterClass helpers.

ejona86 commented 6 years ago

I need to look at this more, but does Dagger really require you to do the test injection with a static? There seems like there should be a way to avoid that. I'm not going to enough Dagger to provide a better pattern.

In any case, I see no reason to use reflection. I'd suggest a call closer to grpcServerRule.getServiceRegistry().addService(XyzTestModule.abcImpl); from within the @Test or @Before of your test method. You could let XyzTestModule.abcImpl be injected into the test class as well, if that's possible in Dagger.

ejona86 commented 6 years ago

I read it again, and I don't think I see anything else to do here. It would be trivial to allow calling getServiceRegistry() before @Before, but it seems that's not really the problem here.

ensonic commented 6 years ago

FYI: I did this as a GrpcTestService

public class GrpcTestService extends GrpcServerRule {
  public void start() throws Throwable {
    super.before();
  }

  public void stop() {
    super.after();
  }
}

and then this as a GrpcTestModule:

@Module
public class GrpcTestModule {
  @Provides
  @Singleton
  GrpcTestService providesGrpcServerRule() {
    return new GrpcTestService();
  }
}
ejona86 commented 6 years ago

@ensonic, there should be no need to extend GrpcServerRule. I didn't realize it was non-final. It should be final, and when we make it so it will break you.