4 ms·
Calling out DI is a bit of a misdirection as most have come to know DI I think. What he's really arguing against here is more basic: Composition. Pragmaticall
by ssmoot 14y ago
Calling out DI is a bit of a misdirection as most have come to know DI I think.
What he's really arguing against here is more basic: Composition.
Pragmatically the examples makes some sense. Why not stub out any global like this though? Because it breaks encapsulation and makes the program harder to understand.
Instead of lifting dependencies up to the level of method parameters with good composition, now you have to truly grok the entire method before you can have any confidence you even know what to stub out, and wether that's actually going to produce the expected results.
So while I fully endorse this specific, localized, easy to understand example, the poor composition of many Ruby libraries is what tends to make working with Ruby code so damn hard (IMO, see: Bundler). It's not enough to read method signatures. There are no explicit interfaces. Only implicit ones you devine by understanding the full breadth and scope of how parameter A is used in method B, all the way down the stack trace.
Fundamentally here DHH isn't talking about "Dependency Injection". He's talking about Composition and a pragmatic example of breaking Encapsulation. While sure there are 101 ways in which breaking Encapsulation can be a useful, pragmatic technique to employ for the seasoned code-slinger in the small, in the large it makes for more difficult to understand, and therefore less maintainable code.
I find many of these recent posts by DHH a bit ironic considering the subtext of a guy who read the PoEAA, then went off and wrote an MVC web-framework, packed with a nice Table Data Gateway, then proceeded to confuse Unit for Integration tests, and soap-box on the evils of Design Patterns in general.
[EDIT]
PS: The obvious example for making it easy to test, without breaking encapsulation, would simply be to avoid globals and use a default parameter.
def publish!(current_time = Time::now)
self.update published_at: current_time
end
TA-DA. So trivial examples might show how a very shallow example of monkey-patching can be a nice convenience, you also have simple "fixes" that actually take _less_ code to implement.
You could easily come up with deeper stacks, presenting more difficult problems, but then you're not really making a great case for the beauty and simplicity of monkey patching if I have to have such a deep understanding of the side-effects in your code before I can even start making sense of your tests.
- swanson 14y agoRegarding your example using default params - this is the same argument (http://weblog.therealadam.com/2013/01/03/design-for-test-vs-design-for-api/ http://weblog.therealadam.com/2013/01/03/design-for-test-vs-...) that DHH is addressing in this blog post. Adam's post does raise an interesting point about the "mass" of the function. Since `publish!` now has parameters, the public API is more complex. Whether or not this trade-off is worth it or not will probably have to be evaluated on a case-by-case basis, methinks.
- pacala 14y agoThe APIs are exactly the same. One is explicit, reading its signature is enough to understand what it does, the other one is implicit, requiring reading the method body. The implicit version readability mass increases fast as it makes further transitive calls, which are now all required reading. The explicit version mass is always constant: the signature.
- twerquie 14y ago> It's not enough to read method signatures. There are no explicit interfaces. Only implicit ones you devine by understanding the full breadth and scope of how parameter A is used in method B, all the way down the stack trace. Unit tests are meant to explicitly define the code's interface, and Ruby makes it very easy to write tests. Unfortunately, in practice this doesn't always happen and what you're describing rings a painful bell.
- jdminhbg 14y agoI don't like adding that parameter at all. When someone else comes across your method, will they know whether #publish! is meant to have a changeable time in real use? You don't want test-specific functionality leaking into real code.
- yummyfajitas 14y agodef publish!(current_time_TEST_USE_ONLY = Time::now) self.update published_at: current_time_TEST_USE_ONLY end Another way to do it (what I do) is simply run the test, and compare obj.published_at to Time::now. If they differ by more than 100ms, something has gone terribly wrong (if you use Ruby, maybe replace 100ms with 1s).
- rlpb 14y ago> If they differ by more than 100ms Please don't do this. It'll work fine for you, but if your tests ever get integrated into a larger system, then tests may start breaking randomly. I might be running tests concurrently, for example. On a virtual machine whose host is a bit more heavily loaded than normal. Running a large test suite is one situation where I don't care about interactivity and can just go away and come back later. Except that with a test that makes assumptions about running time, I can't. And this problem has actually happened to me, so isn't just hypothetical.
- tiziano88 14y agothe perfect recipe for a flaky test!
- jufo 14y agoIf you have to do this, you could check that the published time was >= the time at which you started the test, and <= the time at which you are doing the check.
- jfager 14y agoI can't count the number of times I've had to re-kick a build because a timing-dependent test like that failed (busy build server, inopportunely timed gc, etc). One of my top programming annoyances.
- deleted 14y ago[deleted]
- sbov 14y agoI see what you did here. Now write the code to test calling publish! without an argument provided for current_time.
- ssmoot 14y agoWhy? What's the point? Really? People shouldn't be losing sleep over testing their stdlib. FWIW, I've used a Time::warp function on several occasions. I really don't have a problem with this code. It's a straw man of an example.