Hunting the Shmoo

Screencasts and blog posts on workflow, productivity, tools, Mozilla and whatever else tickles my fancy.

Part 2: How to deal with IFFY requirements

My last post was basically a very long winded way of saying, “we have a problem”. It kind of did a little dance around “why is there a problem” and “how do we fix it”, but I want to explore these two questions in a bit more detail. Specifically, I want to return to the two case studies and explore why our test harnesses don’t work and why mozharness does work even though both have IFFY (in flux for years) requirements. Then I will explore how to use the lessons learned to improve our general test harness design.

DRY is not everything

I talked a lot about the DRY principle in the last article. Basically the conclusion about it was that it is very useful, but that we tend to fixate on it to the point where we ignore other equally useful principles. Having reached this conclusion, I did a quick internet search and found an article by Joel Abrahamsson arguing the exact same point (albeit much more succinctly than me). Through his article I found out about the SOLID principles of object oriented design (have I been living under a rock?). They are all very useful guidelines, but there are two that immediately made me think of our test harnesses in a bad way. The first is the single responsibility principle (which I was delighted to find is meant to mitigate requirement changes) and the second is the open/closed principle.

The single responsibility principle states that a class should only be responsible for one thing, and responsibility for that thing should not be shared with other classes. What is a responsibility? A responsibility is defined as a reason to change. To use the wikipedia example, a class that prints a block of text can undergo two changes. The content of the text can change, or the format of the text can change. These are two different responsibilities that should be split out into different classes.

The open/closed principle states that software should be open for extension, but closed for modification. In other words, it should be possible to change the behaviour of the software only by adding new code without needing to modify any existing code. A popular way of implementing this is through abstract base classes. Here the interface is closed for modification, and each new implementation is an extension of that.

Our test harnesses fail miserably at both of these principles. Instead of having several classes each with a well defined responsibility, we have a single class responsible for everything. Instead of being able to add some functionality without worrying about breaking something else, you have to take great pains that your change won’t affect some other platform you don’t even care about!

Mozharness on the other hand, while not perfect, does a much better job at both principles. The concept of actions makes it easy to extend functionality without modifying existing code. Just add a new action to the list! The core library is also much better separated by responsibility. There is a clear separation between general script, build, and testing related functionality.

Inheritance is evil

This is probably old news to many people, but this is something that I’m just starting to figure out on my own. I like Zed Shaw’s analogy from Learn Python the Hard Way the best. Instead of butchering it, here it is in its entirety.

In the fairy tales about heroes defeating evil villains there’s always a dark forest of some kind. It could be a cave, a forest, another planet, just some place that everyone knows the hero shouldn’t go. Of course, shortly after the villain is introduced you find out, yes, the hero has to go to that stupid forest to kill the bad guy. It seems the hero just keeps getting into situations that require him to risk his life in this evil forest.

You rarely read fairy tales about the heroes who are smart enough to just avoid the whole situation entirely. You never hear a hero say, “Wait a minute, if I leave to make my fortunes on the high seas leaving Buttercup behind I could die and then she’d have to marry some ugly prince named Humperdink. Humperdink! I think I’ll stay here and start a Farm Boy for Rent business.” If he did that there’d be no fire swamp, dying, reanimation, sword fights, giants, or any kind of story really. Because of this, the forest in these stories seems to exist like a black hole that drags the hero in no matter what they do.

In object-oriented programming, Inheritance is the evil forest. Experienced programmers know to avoid this evil because they know that deep inside the Dark Forest Inheritance is the Evil Queen Multiple Inheritance. She likes to eat software and programmers with her massive complexity teeth, chewing on the flesh of the fallen. But the forest is so powerful and so tempting that nearly every programmer has to go into it, and try to make it out alive with the Evil Queen’s head before they can call themselves real programmers. You just can’t resist the Inheritance Forest’s pull, so you go in. After the adventure you learn to just stay out of that stupid forest and bring an army if you are ever forced to go in again.

This is basically a funny way to say that I’m going to teach you something you should avoid called Inheritance. Programmers who are currently in the forest battling the Queen will probably tell you that you have to go in. They say this because they need your help since what they’ve created is probably too much for them to handle. But you should always remember this:

Most of the uses of inheritance can be simplified or replaced with composition, and multiple inheritance should be avoided at all costs.

I had never heard the (apparently popular) term “composition over inheritance”. Basically, unless you really really mean it, always go for “X has a Y” instead of “X is a Y”. Never do “X is a Y” for the sole purpose of avoiding code duplication. This is exactly the mistake we made in our test harnesses. The Android and B2G runners just inherited everything from the desktop runner, but oops, turns out all three are actually quite different from one another. Mozharness, while again not perfect, does a better job at avoiding inheritance. While it makes heavy use of the mixin pattern (which, yes, is still inheritance) at least it promotes separation of concerns more than classic inheritance.

Practical Lessons

So this is all well and great, but how can we apply all of this to our automation code base?

A smarter way to approach our test harness design would have been to have most of the shared code between the three platforms in a single (relatively) bare-bones runner that has a target environment (e.g desktop Firefox, Fennec or B2G in this case). In this model there is no inheritance, and no code duplication. It is easy to extend without modifying (just add a new target environment) and there are clear and distinct responsibilities between managing tests/results and actually launching them. In fact this how the gaia team implemented their marionette-js-runner. I’m not sure if that pattern is common to node’s mocha runner or something of their design, but I like it.

I’d also like our test harnesses to employ mozharness’ concept of actions. Each action could be an atomic as possible setup step. For example, setting preferences in the profile is a single action. Setting environment is another. Parsing a manifest could be a third. Each target environment would consist of a list of actions that are run in a particular order. If code needs to be shared, simply add the corresponding action to whichever targets need it. If not, just don’t include the action in the list for targets that don’t need it.

My dream end state here is that there is no distinction between test runners and mozharness scripts. They are both trying to do the same thing (perform setup, launch some code, collect results) so why bother wrapping one around the other? The test harness should just be a mozharness script and vice versa. This would bring actions into test harnesses, and allow mozharness scripts to live in-tree.

Conclusion

Is it possible to avoid code duplication with a project that has IFFY requirements? I think yes. But I still maintain it is exceptionally hard. It wasn’t until after it was too late and I had a lot of time to think about it that I realized the mistakes we made. And even with what I know now, I don’t think I would have fared much better given the urgency and time constraints we were under. Though next time, I think I’ll at least have a better chance.


Share

Comments

SteveFink
25/04/2014

Repeating yourself really does suck for all kinds of reasons. IMHO, it usually means that you haven't adequately analyzed the tasks at hand. Sharing code because it happens to be exactly the same (or very similar) to other code is a bad idea, true. But if you figure out *why* that code looks the same, then you can usually find that at least part of what it is doing can be thought of more abstractly and implemented once, then called by both of the original places. That boundary should be a semantic boundary, though, not a syntactic one (as in, don't share as much as possible; share just what is needed to implement some human-meaningful portion of the task.) If that means there's still a little duplication before or after the call to the shared portion, so be it.

But that doesn't mean inheritance. Inheritance isn't a horrible evil, but it requires you to pick one particular dimension to describe things with, and there are often multiple equally valid dimensions to use. "is-a" just isn't a particularly good way to break down a structure. I'm sure there are good best practices or rules of thumb for picking the right dimension, but I don't know what they are. I'm not fond of mozharness's mixins. They've even been the direct cause of bugs in my code, where the fix was to reorder the list of mixins. That doesn't smell good.

Composition isn't all roses either. You end up nouning verbs a lot -- eg instead of log("foo") and inheriting the appropriate log() behavior, you instead logger.log("foo"). So you end up with a logger and a formatter and a connector and lots of other (verb)ers. All of which need to be constructed and passed in, which leads you into the abstraction hell of factories and factory factories and configuration providers and other such "can't I just write the damn code?!" gunk. But still, it lets you use one (verb)er per dimension, which is really nice.

Avoiding repetition has real benefits. Obviously, there's the one where you only have to change one place to fix everything. But it's also easier to maintain a coherent architecture when you don't have repeated or half-repeated bits lying all over the place.

That's about as far as I'm willing to go with an abstract argument. I haven't looked at the test runner code, but for mozharness, I'd really like better separation of Mock environments, running-under-buildbot or not, actions that are exactly the same (modulo some configuration, possibly) across many jobs, and most of all, making actions independent rather than relying on certain other actions to have already been run in a certain order.


Leave a comment

Your comment has been submitted and will be published once it has been approved.