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.