Java generation: allow non-final generation for classes

[carojkov]

Allow mocking stubs - don't make them final

I'd love to be able to mock stubs

private HelloServiceGrpc.HelloServiceBlockingStub stub;

public void hello(String message) {
    stub.hello(message); <-- I should be able to make a unit test that verifies stub.hello() is called and with the right value.
}

@artnaseef

[artnaseef]

+1 - being able to mock these stubs would make clean unit testing far easier.

[panchenko]

It's possible to mock final classes with Mockito.
In 5.* versions it just works, in previous versions need the org.mockito:mockito-inlinedependency

[ejona86]

We do not support mocking stubs. They are final for the sole purpose of preventing mocking (as a private constructor prevents extension, but you need final to prevent mocking). Yes, there may be ways to defeat it, but you are completely on your own.

It caused support issues early in gRPC's life, and so we add final anywhere we possibly can. Users found ways to even break anonymous classes!
#1469 (comment)

[carojkov]

I am sure improper use of the library can cause support issues, but I think a library should be catering to a broader spectrum of use-cases. Mocking is a an important use case which should be addressed. There are more ways to do that then not making it final e.g. introducing a generated interface that a Stub implements.

That, I think, would be an acceptable compromise.

[ejona86]

Mocking without letting using withDeadline() is too much of a toy to consider. And mockers will mess up the with* methods. Pretty much in every way mocking stubs is a dead-end.

Alternative ways to mock services would be fine. The main point is to have in-process transport in the mix. I think the ideal solution here would be "have a blocking service API." That isn't as far away as it once seemed, but probably isn't that close either.

[ejona86]

Put another way: mocking stubs prevents you from using gRPC properly and from knowing you use it improperly. You can blame the heavy use of mocks internally at Google for teaching us this lesson early. In Google, when we upgraded we'd have to fix everyone's broken, garbage, useless tests. So many useless tests, that tested nothing and made us debug impossible-in-production situations. Bad tests provide negative value. We've lived in that alternate universe, and it was a bad one.

[artnaseef]

@ejona86 I started a long response to your comments, but decided to try a slower approach here.

What kind of mocking are you referring to that includes timeouts?

From my perspective, mocking enables PURE UNIT TESTING where the unit is 1 java class (my own class), and mocking enables me to wire up fixed responses to method calles into classes that otherwise might do a lot of work. So the mock just immediately returns a fixed value in one specific test case, enabling me to ensure my code correctly handles that specific return value.

mocking stubs prevents you from using gRPC properly and from knowing you use it improperly

Correct. The point of the mock is to enable my pure unit testing of my code, not to verify how it works with other classes. That's an integration test.

[ejona86]

What we found was that "pure unit testing using gRPC" was not actually testing. If a substantial percentage of Google engineers's tests that mock gRPC didn't actually verify anything useful, then it was simply harmful and not useful for the masses.

We have checks in our code to make sure you are using APIs correctly. By using our implementation you get additional assertions for free, for things you weren't likely to check yourself. There's basically no cost to using In-process transport with directExecutor(). You get deterministic results that are fast and reflect reality. As a bonus, if gRPC happens to have a regression, you'll find out when testing instead of production.

You don't mock out ArrayList in your "pure unit tests." Don't mock out gRPC's stubs.

[artnaseef]

You didn't really respond to me. "Pure unit testing" of MyClassThatUsesGrpc does NOT call GRPC and does NOT verify it is correctly using GRPC. That's INTENTIONAL.

What you are talking about is integration testing. Using mocks for integration testing is no-bueno - just don't do it. (see https://github.com/savoirtech/black-box-system-test).

As far as a substantial number of your colleagues doing it wrong - that's not your problem, and the solution to prevent that is unfortunately getting in the way of others who are trying to do something that is not only reasonable - it's best practice. That is, thoroughly unit testing their own code.

We have checks in our code to make sure you are using APIs correctly

That's great. And during integration testing, that's wonderful to have. Thank you. During unit testing, that is not important - the integration tests are the ones that will tell me when I've used GRPC incorrectly. The unit tests won't tell me that because they are not integrating with GRPC.

[artnaseef]

BTW...

What we found was that "pure unit testing using gRPC" was not actually testing

That sounds 100% like grey box tests. How do you pure unit test "using gRPC"? In my mind, that's just not possible.

[artnaseef]

Did you write your own "mock gRPC"?

[carojkov]

@ejona86 Hm, the argument that making the classes final helped with the correctness may be flawed. That may have happened due to people acquiring more experience and having better examples for grpc.

In my unit test I want to test a specific and small concern - the stub is called with the value I expect - that's all. Having to stand up the receiving end increases cognitive load on both writers and readers of the test.

Just imagine that you'd have to start in process servlet container in order to test that HttpServletResponse sends the right data. Sounds horrible, right?

[artnaseef]

It's possible to mock final classes with Mockito. In 5.* versions it just works, in previous versions need the org.mockito:mockito-inlinedependency

That is true. However, it's not an ideal solution here. First, Mockito is no the only mock framework. Second, in my experience, Mockito sometimes breaks when using that feature (i.e. tests that were working stop working when enabling the feature that allows mocking of final classes).

[ejona86]

Just imagine that you'd have to start in process servlet container in order to test that HttpServletResponse sends the right data.

Have you seen our example tests in the helloworld example?

In my unit test I want to test a specific and small concern - the stub is called with the value I expect - that's all.

That's all today. And if we needed to add a deadline (which is very common), it'd start breaking. The tests then prevent you from doing things or enabling features that you should. All code starts simple, but it generally doesn't stay that way.

As far as a substantial number of your colleagues doing it wrong - that's not your problem

It definitely is our problem. If we break a test in Google, we have to fix it. And even in OSS that is our problem if you can't upgrade to a new version of gRPC because your tests break. You then plead to us to release patches to old releases.

That sounds 100% like grey box tests. How do you pure unit test "using gRPC"? In my mind, that's just not possible.

I added the "using gRPC" a bit later. The code under test was using gRPC, and the test was mocking gRPC out. So "using gRPC APIs" may be more apt.

[carojkov]

And if we needed to add a deadline (which is very common), it'd start breaking.

I am not sure I understand how you mean it will start breaking.

It might say the mock didn't expect the withDeadline() to be called. Is that what you mean by 'it'd start breaking' ?

[ejona86]

I am not sure I understand how you mean it will start breaking.

Well, by default it'd return null and you'd get a NPE. But how would you fix that? when(mock.withDeadline()).thenReturn(mock) is broken and wouldn't notice this important bug:

stub.withDeadline(1, SECONDS);
stub.someRemoteMethod(req);

And when(mock.withDeadline()).thenReturn(mock2) is broken when called multiple times, which many tests would need to be careful about. Yes, I know how to resolve that, but actually implementing behavior that would catch bugs in code-under-test by mocking the stub is not practical. Having initial test writing using stub mocks means you'd need to rewrite them the moment you need to do something "real," or you'd hack the mock so badly it wouldn't be testing the implementation.

We did consider having a mock Channel, so you'd still use the normal stub which is relatively thin. But that's essentially what in-process transport is, and in-process has the added benefit that it is production-worthy/tested. You can still use mocks, but you mock the service.

[panchenko]

The challenges with mocking are more generic and not specific to gRPC.
It's normal to mock only those methods which are called, etc.
Depending on the particular case - one way or another could be slightly more convenient.
For example, a JUnit5 extension for in-process server would definitely help with promoting better practices, but that's a different issue.

Back to this topic: I don't see a problem with stub classes being final, as it's still possible to mock.
Why such a long discussion?

[carojkov]

I think when(mock).withDeadline(expectedDeadline).thenReturn(mock) is exactly what you want.
It will verify that expected value for the deadline is set.

wouldn't notice this important bug:

stub.withDeadline(1, SECONDS);
stub.someRemoteMethod(req);

Hm, I don't see the bug that slipped in here due to our mocking above. Can you explain?

[ejona86]

Hm, I don't see the bug that slipped in here due to our mocking above.

stub is immutable. When you make a config change it returns a new instance. So that code snippet didn't actually set any deadline, because it discarded the result.

[carojkov]

Edit:

I see, returning a new instance for with methods is an interesting choice.

Perhaps that [the api] is the root of the problem with the observed failures to write proper tests?

[artnaseef]

Hm, I don't see the bug that slipped in here due to our mocking above.

stub is immutable. When you make a config change it returns a new instance. So that code snippet didn't actually set any deadline, because it discarded the result.

As I mentioned earlier, making sure the code-under-test is correctly using the gRPC APIs is NOT the intent of the test. That problem will get caught later in integration testing. The POINT of a pure unit test IS to verify that the code works the way the developer intened, with or without the developer making mistakes in using other libraries. SO, the scenario of not correctly using the return result of the stub.withDeadline(...) call will NOT be caught by unit testing with mocks. That's correct. That's intentional. It's not a problem.

As far as a substantial number of your colleagues doing it wrong - that's not your problem

It definitely is our problem. If we break a test in Google, we have to fix it. And even in OSS that is our problem if you can't upgrade to a new version of gRPC because your tests break. You then plead to us to release patches to old releases.

Do you mean that you/your team are/is responsible to fix tests that you don't write for code you don't maintain???

[artnaseef]

BTW, my 2-cents on the Builder Pattern - builder patterns are kinda handy, but often super ugly - because they "pretend to be" a langauge construct when they are not, and the Java language itself does not provide great tooling to write them. We just have to take the good with the bad when using them.

[artnaseef]

Edit:

I see, returning a new instance for with methods is an interesting choice.

Perhaps that [the api] is the root of the problem with the observed failures to write proper tests?

I think a lot of builder patterns use an explict build method so avoid the problems an immutable builder creates like this. Using an explicit build method with a mutable builder, that does nothing but build, together with an immutable end-result eliminates this confusion.

For methods that change state, returning new instances is the correct way to build immutable objects.

[artnaseef]

The challenges with mocking are more generic and not specific to gRPC. It's normal to mock only those methods which are called, etc. Depending on the particular case - one way or another could be slightly more convenient. For example, a JUnit5 extension for in-process server would definitely help with promoting better practices, but that's a different issue.

Back to this topic: I don't see a problem with stub classes being final, as it's still possible to mock. Why such a long discussion?

So first, that stated reason for final classes:

We do not support mocking stubs. They are final for the sole purpose of preventing mocking

Second, not all mocking frameworks will work with final classes. And third, given "1st", why escalate the "war" in the first place?

Now if there is a desire to stay with making stubs final (yuck) then perhaps a good way to support a community that does things differently from your teams/company, an option on the code generator for those of use conviced we don't want final classes and put all kinds of warnings, "WE DON'T RECCOMMEND YOU USE THIS".

Also, perhaps there is a chance this discussion will lead to learning and coming to common understanding. That's always a good thing.

[panchenko]

If using Gradle the generated files can be easily modified before compilation.

tasks.named('generateProto').configure {
    doLast {
        // modify generated files here
    }
}

[carojkov]

@ejona86 thanks for your time and explanation. I now understand that final isn't the problem. My current thoughts on this is that grpc java API does not properly address unit-testing use-case. I and a few others on this thread and bug reporting system consider this use-case important. I hope that java api changes to address this use-case.