Problem

Currently AbstractHealthIndicator class has the final modifier on its (the only) public health() method. This makes the child classes hard to mock in unit tests.

Example

Say we have a composite health check based on DataSourceHealthIndicator and RabbitHealthIndicator statuses (simplified for brevity):

public class MyAppHealthProbe {
  // dataSourceHealthIndicator & rabbitHealthIndicator are constructor-injected fields of this class
  public boolean isReady() {
    return UP.equals(dataSourceHealthIndicator.health().getStatus())
        && UP.equals(rabbitHealthIndicator.health().getStatus());
  }
}

And we’d like to test the isReady method with the following code:

  @Test
  void isReadyDesired() {
    // given
    var rabbitHealthIndicatorMock = mock(RabbitHealthIndicator.class);
    var dataSourceHealthIndicatorMock = mock(DataSourceHealthIndicator.class);
    var sut = new MyAppHealthProbe(rabbitHealthIndicatorMock, dataSourceHealthIndicatorMock);
    when(rabbitHealthIndicatorMock.health()).thenReturn(Health.up().build());        // fails because of 'final'
    when(dataSourceHealthIndicatorMock.health()).thenReturn(Health.up().build());        // fails because of 'final'
    // when
    boolean isMyAppReady = sut.isReady();
    // then
    assertTrue(isMyAppReady);
  }

But we cannot do that because Mockito is unable to mock final methods:

Somewhere in your test you are stubbing final methods. Sorry, Mockito does not verify/stub final methods.

Other solutions

As a workaround, we have to introduce additional implementations of each indicator class in the tests, override the doHealthCheck method in them and then mock it with Mockito#doAnswer method. But this seems a too complicated and non-readable solution.

Considerations

At the same time, we don’t see any clean reasons why the AbstractHealthIndicator#health is declared final. There was a suggestion to remove the modifier but it was declined with an unclear explanation. I suppose that the modifier is intended to prevent indicators’ developers from accidentally implementing the wrong method (health instead of doHealthCheck).

Suggestion

Nonetheless, I’d like to offer to make it non-final because the testability is also very important.

P.S.

If this is an acceptable change, I can submit a PR with it.

I also would be glad to see other alternative solutions.

Comment From: snicoll

Thanks for the suggestion. I know you've simplified the probe for brevity but can you share why you need to inject a concrete health indicator type? All you do is calling the method of the interface.

There was a suggestion to remove the modifier but it was declined with an unclear explanation.

The issue you've referenced and your request are completely different. Rather that declining to remove the modifier, we discussed why they needed to do that as part of AbstractHealthIndicator and they agree they could look for another option.

Comment From: Toparvion

@snicoll , thank you for quick response and for the clarification on the mentioned issue.

can you share why you need to inject a concrete health indicator type?

In that particular case we don’t use any of concrete implementation’s methods, but the need for them initially arose from the injection ambiguity since we have two beans of the same type:

@Autowired
public MyAppHealthProbe(HealthIndicator rabbitHealthIndicator,
                        HealthIndicator dataSourceHealthIndicator) {

Of course, we could use @Qualifier("rabbitHealthIndicator") annotation but we preferred to inject concrete classes instead because:

  1. There is no public rabbitHealthIndicator nor dataSourceHealthIndicator string constant in Spring Boot code so that we cannot reference it in our code and hence have to duplicate it (that seems quite brittle)
  2. Injection of concrete indicators gave us a strong link to the indicators’ sources in the IDE, i.e. it became easy to navigate to the indicator source code with just one click rather than searching for the bean name through the libraries code. (I know that IDE should be able to find beans code by the qualifier but unfortunately it works far not always.)

Is it a framework misusing or we should consider other approaches?

Comment From: snicoll

Thanks for the feedback, using the constructor as an injection point is the information that I needed.

Is it a framework misusing or we should consider other approaches?

It isn't really but you could consider another approach indeed. Rather than making MyAppHealthProbe a @Component where you need to tell the container what you want to use, you could defer that to a @Configuraiton, something like

@Bean
public MyAppHealthProbe myAppHealthProbe(DataSourceHealthIndicator dataSourceHealthIndicator,
        RabbitHealthIndicator rabbitHealthIndicator) {
    return new MyAppHealthProbe(dataSourceHealthIndicator, rabbitHealthIndicator);
}

That way, MyAppHealthProbe doesn't need to tell about the specific types and you can mock that easily. Perhaps the individual indicators aren't important for it to serve its purpose and all your implementation needs is a List. In that case you could modify the contract of MyAppHealthProbe to be a bit more explicit.

Given that your implementation doesn't need to know about the particular type (if I understood correctly) and you want to mock the contract at the same time, it feels to me that it's an acceptable option.

I am flagging this for team attention to see what the rest of the team thinks about the testability argument.

Comment From: Toparvion

Perhaps the individual indicators aren't important for it to serve its purpose and all your implementation needs is a List

The original code of the isReady method contained a kind of “togglers” for each indicator to control its contribution into the aggregating probe’s result:

  @Override
  public boolean isReady() {
    return super.isReady()
        && (!readinessProbeProperties.isDbCheckEnabled()
            || UP.equals(dataSourceHealthIndicator.health().getStatus()))
        && (!readinessProbeProperties.isRabbitCheckEnabled()
            || UP.equals(rabbitHealthIndicator.health().getStatus()));
  }

Unfortunately, those “togglers” cannot be replaced with Actuator’s built-in ones (that prevent the beans from registering in the context). Therefore we had to distinguish individual indicators somehow and couldn’t collapse them into a single list.

But after your suggestion to use provider-like @Configuration class and after some internal discussion, we’ve come to extracting that “toggling” logic outside from the aggregating probe as well. This allowed us to use the list of indicators and make the probe more simple.

So, for this particular case I think we’ve found an acceptable solution. Thank you very much @snicoll!

As of testability argument in general, I’d like to keep the issue open (for a while) to hear your teammates’ opinion.

Comment From: wilkinsona

IMO, the health method being final is a key part of AbstractHealthIndicator being designed for extension. Changing this would introduce confusion for anyone extending AbstractHealthIndicator as the need to override health vs implementing doHealthCheck would become less clear. There's a small trade-off in terms of testability, but until there's a situation that is untestable with the current arrangement, I don't think a change can be justified.

The code above could also have been made testable by extracting package-private methods for the DataSource and Rabbit health:

public boolean isReady() {
    return UP.equals(dataSourceHealth().getStatus()) && UP.equals(rabbitHealth().getStatus());
}

Health dataSourceHealth() {
    return this.dataSourceHealthIndicator.health();
}

Health rabbitHealth() {
    return this.rabbitHealthIndicator.health();
}

dataSourceHealth and rabbitHealth can then be overridden in the test-only subclass of MyAppHealthProbe and the public API is unaffected.

Comment From: Toparvion

@wilkinsona , thank you for the detailed explanation! You've confirmed my assumptions (in the initial comment) and it helps me to understand the framework better.

dataSourceHealth and rabbitHealth can then be overridden in the test-only subclass

This is quite similar to what we did before another solution has been found in this issue. While generally acceptable, we found this approach too verbose comparing to a single-line when(...).thenReturn(...); Mockito expression.

Closing the issue with big thanks to all of you for your patience and comprehensive answers.