The service container is used in many of our tests. However, it is not used in any of the tests in the unit subfolder.
Is it acceptable to use the service container in unit tests or should the dependencies (always?) be mocked? The documentation has not yet been written on the subject.
@dbarratt Thanks for rasing this question.
In short, I think the answer is "No", in so far that core's default/global service wiring code should not be used in our "unit unit" tests. The "unit unit" tests do not require MediaWiki to be installed, can be executed from a composer command, and have as the general model that they are not integration tests (roughly in line with the "test pyramid" convention).
In my view, something is a "good" unit test if it explicitly instantiates the subject class and fully owns the all its (runtime/injected) dependencies. In other words, the subject class does not use global state, and the dependencies are constructed explicitly within the test suite. Alternatively, as you mention, you can also stub some or all injected dependencies with a PHPUnit Mock object. However, in my opinion that is not required for it to be a unit test, and there are plenty of cases where such stubbing would imho be counter-productive (adding maintenance cost, and significantly decreasing the value of the test, double loss).
As such, the answer to your question is no because:
Thanks @Krinkle !
My thinking with mocking was if you had a long dependency chain like:
Class A → Dependency B → Dependency C → Dependency D → Dependency E
and you were testing Class A it would be a lot easier to simply mock Dependency B than to mock the whole chain.
Does that make sense? Or would it be better to instantiate the entire dependency chain?
I think that's a decision best left as a judgement call. Sometimes there's good value to be had by mocking it, sometimes by not mocking it.
If a dependency is not providing data required by the subject (e.g. a service merely sync to, write to, or fallback to), then mocking it seems the quickest way to get it done – without compromising anything else. I will note that if a dependency is indeed optional in this manner, one could argue that perhaps it shouldn't be a mandatory dependency of the subject. The subject may want to allow re-use without it in production as well, not just in test. Either by making the constructor parameter optional, or by having the dependency provide a production-grade placeholder, such as NullLogger, LCStoreNull, PoolCounterNull, EmptyBagOStuff, JobQueueMemory, etc.
If a dependency plays an important role to the subject's core function, you'll probably want to explicitly instantiate it as otherwise you'll either end up a devalued test because the subject is used in a way that is very unlike production, or end up having to effectively re-create the business logic of the dependency through length mock-building boilerplate. I personally find that an anti-pattern because 1) it's a lot of work with minimal or no added value 2) it's complex enough to imho classify as –untested– source code, and 3) it devalues the test because we've now hardcoded a copy of our business logic that already is or likely will become outdated or wrong at some point. Thus allowing bugs to creep in that are hard ot catch. An integration test might some of that at some point, but I think that should be reserved for a handful of cases only to verify the end-to-end story as powered by service wiring. The deeper connection between a subject and its "logic dependencies" is ideally fully covered by a unit test. In such case, I'd rather it use the real one. I'm not aware of there being a downside to doing that, either.
Except mabe if the logic dependency is more than that, e.g. if it is a kitchensink that also makes db queries and such. Such class should probably be split to allow stubbing that part only.