-
Notifications
You must be signed in to change notification settings - Fork 24.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework shutdown no-op tests to avoid mocks #86236
base: main
Are you sure you want to change the base?
Rework shutdown no-op tests to avoid mocks #86236
Conversation
In elastic#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 decouple them from the cluster service API.
Pinging @elastic/es-core-infra (Team:Core/Infra) |
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. |
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. |
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.
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? |
These things are are all lightweight fakes in this test. Admittedly creating a 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 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. |
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.