On Sun, Sep 1, 2019 at 12:40 PM Aryeh Gregor <a...@aryeh.name> wrote:
> On Fri, Aug 30, 2019 at 10:09 PM Krinkle <krinklem...@gmail.com> wrote: > > For anything else, it doesn't really work in my experience because > PHPUnit > > won't actually provide a valid implementation of the interface. It > returns > > null for anything, which is usually not a valid implementation of the > > contract the class under test depends on. [..] > > In my experience, classes that use a particular interface usually call > only one or two methods from it, which can usually be replaced by > returning fixed values or at most two or three lines of code. [..] It could sometimes not be worth it to mock for the reason you give, > but in my experience that's very much the exception rather than the rule. > [..] > Consider a class like ReadOnlyMode. It has an ILoadBalancer injected. > The only thing it uses it for is getReadOnlyReason(). When testing > ReadOnlyMode, we want to test "What happens if the load balancer > returns true? What about false?" A mock allows us to inject the > required info with a few lines of code. While I do prefer explicit dependencies, an extreme of anything is rarely good. For this case, I'd agree and follow the same approach. And I think there are plenty of other examples where I'd probably go for a partial mock. Doing so feels like a compromise to me, as I have often seen this associated with working around technical debt (e.g. the relationship between those classes could've been better perhaps). But I think it's fair to say there will be cases where the relationship is fine and it still makes sense to write a partial mock to keep the test simple. In addition to that, I do think it is important to also have at least one test case at the integration level to verify that the higher-level purpose and use case of the feature works as intended - where you'd have to construct the full dependency tree and mock or stub only the signal that is meant to travel through the layers. Thus ruling out any mistake in the (separate) unit tests for ReadOnlyMode and LoadBalancer. But, you wouldn't need to have full coverage through that - just test the communication between the two. The unit tests would then aim for coverage of all the edge cases and variations. Thanks for writing this up. > 5) In some cases you want to know that a certain method is being > called with certain parameters,[ ..] Maybe the bug changed the > WHERE clause to one that happens to select your row of test data > despite being wrong. > Yep, this is important. I tend to go for lower level observation instead of injected assertions. E.g. mock IDatabase::select/insert etc. to stash each query in an array, and then toward the end assert that the desired queries have occurred exactly as intended and nothing more. Similar to how one can memory-buffer a Logger instance. I like the higher confidence this gives and I find it easier to write than a PHPUnit mock where each function call is precisely counted and params validated, while being more tolerant to internal details changing over time. Having said that, they both achieve the same goal. I suppose its a matter of taste. Certainly nothing wrong with the PHPUnit approach, I'd just not do it myself as much. Thanks for reminding me of this. > 6) [..] > One way to help catch inadvertent performance regressions is to test > that the means of ensuring performance are being used properly. For > instance, if a method is supposed to first check a cache and only fall > back to the database for a cache miss, we want to test that the > database query is actually only issued in the event of a cache miss. Yep, that works. I tend to do this differently, but it works and its important that we make these assertions somewhere. I don't disagree :) -- Timo _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
