Provide JUnit 5 GrpcTestExtension

[mmichaelis]

What version of gRPC are you using?

1.18.0

What did you expect to see?

In order to be able to test with JUnit 5 I would like to have a comparable solution like GrpcCleanupRule but as JUnit 5 extension.

Example

In my PoC project mmichaelis/poc-grpc you may see an example, which is very similar to your GrpcCleanupRule (because that was my intention :smile:): In module grpc-test you will find the class GrpcTestExtension and an example usage in HelloServiceImplTest.

Help wanted?

I may assist in adding this to the grpc-java project, but there are several questions:

• First of all: Do you like the extension?
• Where to place the JUnit 5 extension? Into testing module? Or an extra module?
• And if it is an extra module: How to share code? My intention would be to refactor GrpcCleanupRule, so that GrpcTestExtension and the JUnit 4 rule share as much code as possible.

[dapengzhang0]

This seems need to add junit 5 dependency in io.grpc.testing. @carl-mastrangelo are we able to add that dependency internally?

[creamsoup]

personally i would like to use junit5. but, internally we only have junit 4.12. junit5 requires pretty big change, so unfortunately it won't happen any time in near future.

[mkobit]

JUnit 5 is also Java 8+. I'm not sure if the grpc-testing module is still under of the umbrella of https://github.com/grpc/grpc-java/issues/4671

[Torbacka]

Just wondering how this issue is going? Is there anything I can do to help?

[dapengzhang0]

@Torbacka , basically it is blocked by #4671. grpc-java is a library that still has to support Java 7.

[Torbacka]

@dapengzhang0 thank for the update.

[bahrigencsoy]

IMO the current design of GrpcCleanupRule is unnecessarily dependent on Junit4 TestRule api. It basically calls teardown method after each test case.

So, I guess, if you can just separate the core cleanup utility from the TestRule extension, we can just call tearDown() method inside @AfterEach method. Or use the engine to create a Junit5 AfterEachCallback extension.

[asarkar]

I created an extension for JUnit 5, that does what the rule did, and more. https://github.com/asarkar/grpc-test

[marcphilipp]

@asarkar Awesome! Could you please add it to https://github.com/junit-team/junit5/wiki/Third-party-Extensions?

[asarkar]

@marcphilipp Added to the "JUnit Jupiter Extensions" section.

[xmlking]

any update on this?

[asarkar]

@xmlking What is stopping you from using https://github.com/asarkar/grpc-test?

[xmlking]

Unfortunately my corporate nexus blocking some external dependencies

[asarkar]

Not sure I understand how grpc-java would be allowed, but grpc-test wouldn't be, given both are available on Maven Central. Good luck.

[ejona86]

I just came across this issue. Even if we assume Java 7 support was dropped today, I'm not sure grpc-testing in the appropriate place to put JUnit 5 stuff. It seems fair to make a separate artifact for it (e.g., grpc-testing-jupiter). If it is a separate artifact, then it is free to require Java 8 before the rest of grpc does.

Dropping Java 7 support will be very soon now, but I don't think there's any need to wait on it. Unless there's disagreement that another artifact is appropriate.

I will say that I don't understand JUnit 5 (it has scared me off each time I've glanced; I had issues with JUnit 3 so welcomed 4, but don't understand why we need the redesign of 5), so this will probably need community guidance on best practices for obvious stuff. I do think we liked the simple-and-get-out-of-your-way approach of GrpcCleanupRule, so I'd hope making the extension is straight-forward with few design questions. Although glancing at asarkar/grpc-test (even if I can't read Kotlin) makes me worry that isn't the case.

I'm not against copying (or similar) asarkar/grpc-test, assuming that @asarkar is fine with that and we (as a community) halfway like the approach. It has existed for years and seems like it works. But I will need someone to explain to me why, oh, why do we need field.isAccessible = true.

[asarkar]

Hi @ejona86

why do we need field.isAccessible = true

Assuming you're referring to this line, this is required to support the use cases 2 and 3 documented in the README.

Get a Resources from:

1. A test method parameter injection, or
2. An instance field, or
3. A static field.

JUnit doesn't provide access to instance or static fields, only test method parameters. The JUnit 4 rule doesn't support these use cases, and hence is relatively simpler.

I made the code available under Apache License v2.0, so if you want to fork it honoring the terms in the license, I've no problems with that. It's not clear to me though where the code will live after copying, since you seem to be recommending making a separate artifact. If that's the case, what's wrong with the the existing artifact? You, or anyone from the gRPC or JUnit team, are welcome to contribute to my repo if you choose to.

[ejona86]

JUnit doesn't provide access to instance or static fields, only test method parameters. The JUnit 4 rule doesn't support these use cases, and hence is relatively simpler.

The JUnit 4 rule supports instance field and static field, without reflection (in its code). It only doesn't support method parameter.

It seems bizarre to me that JUnit 5 would start using setAccessible(true) more heavily, but I do agree that seems to be the case. Strange that people like the idea of a private field being modified-from-a-distance, and in tests none the less. It seems JUnit 5 is built on an idea of multiple inheritance (extensions) and using reflection for everything. Magic types triggering special behavior, and external state. asarkar/grpc-test does look a lot like what is shown in https://www.codeaffine.com/2016/04/06/replace-rules-in-junit5/.

Honestly, it feels like JUnit 5 just doesn't care about resources any more. Writing an extension for every resource type is silly and seems a horrible idea (since it uses setAccessible(true) reflection). It seems like if they cared they'd have a better replacement for ExternalResource. I'm wondering if it would be best to have GrpcCleanupRule extend ExternalResource to allow using JUnit 5's ExternalResourceSupport. Then for people that want the magic-heavy JUnit 5 new-style they can use asarkar/grpc-test (and we link to it in the GrpcCleanupRule documentation or something).

[marcphilipp]

JUnit 5's use of setAccessible(true) is mostly to alleviate users from making test classes and methods public. Extensions don't use multiple inheritance (since you don't inherit any behavior) but follow the interface segregation principle. For a replacement for dealing with resources/state in extensions, please refer to https://junit.org/junit5/docs/current/user-guide/#extensions-keeping-state

[asarkar]

@jon-abdulloev-globality Java version, as in Java source code? No. Why do you ask?

[juanjcal]

i can solve the problem, we only use GrpcServerRule.java https://github.com/grpc/grpc-java/blob/9b55aed12ef51e4189b61551c6936612d8d9cf05/testing/src/main/java/io/grpc/testing/GrpcServerRule.java

when i change the next call:

@Rule 
public final GrpcServerRule grpcServerRule = new GrpcServerRule().directExecutor();

and i used now:

@RegisterExtension
public final GrpcServerRule grpcServerRule = new GrpcServerRule().directExecutor();

but for this i had to change the class GrpcServerRule, i had to create the class in my proyect in Junit5 the next way:

@ExperimentalApi("https://github.com/grpc/grpc-java/issues/2488")
public final class GrpcServerRule implements BeforeEachCallback, AfterEachCallback {
    private ManagedChannel channel;
    private Server server;
    private String serverName;
    private MutableHandlerRegistry serviceRegistry;
    private boolean useDirectExecutor;

    public GrpcServerRule() {
    }

    public final GrpcServerRule directExecutor() {
        Preconditions.checkState(this.serverName == null, "directExecutor() can only be called at the rule instantiation");
        this.useDirectExecutor = true;
        return this;
    }

    public final ManagedChannel getChannel() {
        return this.channel;
    }

    public final Server getServer() {
        return this.server;
    }

    public final String getServerName() {
        return this.serverName;
    }

    public final MutableHandlerRegistry getServiceRegistry() {
        return this.serviceRegistry;
    }

    @Override
    public void afterEach(ExtensionContext context) throws Exception {
        this.serverName = null;
        this.serviceRegistry = null;
        this.channel.shutdown();
        this.server.shutdown();

        try {
            this.channel.awaitTermination(1L, TimeUnit.MINUTES);
            this.server.awaitTermination(1L, TimeUnit.MINUTES);
        } catch (InterruptedException var5) {
            Thread.currentThread().interrupt();
            throw new RuntimeException(var5);
        } finally {
            this.channel.shutdownNow();
            this.channel = null;
            this.server.shutdownNow();
            this.server = null;
        }

    }

    @Override
    public void beforeEach(ExtensionContext context) throws Exception {
        this.serverName = UUID.randomUUID().toString();
        this.serviceRegistry = new MutableHandlerRegistry();
        InProcessServerBuilder serverBuilder = (InProcessServerBuilder)InProcessServerBuilder.forName(this.serverName).fallbackHandlerRegistry(this.serviceRegistry);
        if (this.useDirectExecutor) {
            serverBuilder.directExecutor();
        }

        this.server = serverBuilder.build().start();
        InProcessChannelBuilder channelBuilder = InProcessChannelBuilder.forName(this.serverName);
        if (this.useDirectExecutor) {
            channelBuilder.directExecutor();
        }

        this.channel = channelBuilder.build();
    }

}

[ejona86]

#9240 will improve JUnit 5 compatibility for GrpcCleanupRule. However, in JUnit 5 it won't know if the test failed, so if the test method leaves RPCs open on assertion failure (seems fairly likely) then there will be ~10 second of added time.

I poked at JUnit 5 more, and I don't think we need anything all that special or to do setAccessable(true). The equivalent of GrpcCleanupRule could be made with @RegisterExtension, BeforeEachCallback, and AfterTestExecutionCallback.

[tomhula]

Why not just use the @asarkar's solution?

[ejona86]

@Tomasan7, it is available separately. If you like it, you can use it.

[lewimuchiri]

@asarkar please share an example of how to use grpc-test with a static field and with an instance field. Thanks

[asarkar]

@lewimuchiri See this and this test.

[nddipiazza]

@asarkar and others - I struggled to understand how to replace the JUnit5 examples with existing so I started searching on github for examples and found this: https://github.com/MARSFOREVER472/fivet/blob/a0203c3dd7e01ea524b1792307e6358d2048c0ba/src/test/java/cl/ucn/disc/pdis/fivet/TestGrpc.java#L29

This is a nice example of Junit5 + Grpc