Rework shutdown no-op tests to avoid mocks

[DaveCTurner]

In #85914 we adjusted the shutdown metadata actions to avoid triggering
unnecessary cluster state publications. This commit reworks the
corresponding tests to use a real cluster service which decouples them
from the cluster service API.

[elasticmachine]

Pinging @elastic/es-core-infra (Team:Core/Infra)

[rjernst]

What is the problem with the mocks? This change seems to do the opposite of the stated intention: it makes the tests more coupled to the cluster service api. These are unit tests, so there should be no need for them to have a real cluster service, it's far too heavyweight for just confirming whether a cluster state update task was submitted.

[DaveCTurner]

These tests are not just saying that a task was or wasn't submitted, they're specifying exactly how it's submitted too. That means we can't change the cluster service API without also changing these tests, which is an obstacle for in-flight work such as #85751. To give another example, these tests had to be changed in #86018 even though the no-op checks tested here don't have much to do with batching.

I don't think the cluster service is especially heavyweight either.

[rjernst]

I don't think the cluster service is especially heavyweight either.

Currently the test creates a dummy object that accepts the cluster state update task. This is all the test cares about. It doesn't care about how cluster state updates are actually submitted in production across threads, etc. This change now create executors/threadpools/threads on every test. That is relatively heavyweight.

we can't change the cluster service API without also changing these tests, which is an obstacle for in-flight work such as #85751

The cluster service API should be separate from the implementation. All this test cares about (and I anticipate many other tests care about the same thing) being able to submit an update, then "run" the update to see their listener gets called. It looks like the referenced change essentially creates a task queue for the executor, which is then submitted to instead of submitting the task and executor together (I like that change!). While I understand how mocking here makes that more difficult (because every use of a method increase the cost of refactoring), I still think we should maintain mocks because they are lightweight. Note that I don't mean that needs to be Mockito mocks, only something that mimicks the api, in a controlled and introspectable way, so that the test can confirm it interacted with the external object in an expected way.

Long term, I think we need to extract out the cluster service public api from the implementation, so that we can have an interface that is very easily mockable, eg with a test implementation that doesn't do anything. That is, it just captures the calls made to it, like the mocks we use currently here, but without mockito argument capture craziness. I realize though that is a longer term ask, so could we keep the mocks, or have a helper method for tests to create the current mockito capturing so that the tests are more easily changeable for that referenced PR?

[DaveCTurner]

This change now create executors/threadpools/threads on every test.

These things are are all lightweight fakes in this test. Admittedly creating a ThreadPool does start a single extra thread, ThreadPool#cachedTimeThread, which is immediately stopped. I expect we could fix that. Everything else happens on the main test thread. On my machine this implementation seems to run slightly faster than the mocks-based one in current master.

I'm ok to rework these tests to continue to use mocks within #85751 instead, but it is worth noting that the only tests that need this kind of rework are these ones plus IndexLifecycleRunnerTests and IndexLifecycleServiceTests that also make heavy use of mocks which isn't so easily eliminated.

I'm also on board with (eventually) extracting an interface for the cluster service to allow this kind of capture without involving Mockito. Especially if working with the real service (with fake executors) is a big drag on test performance, but I'm not seeing evidence to support that yet.