by Shyam Seshadri We all have separation anxiety. Its a human tendency. We love inertia, and we don’t like change. But why should your code have separation anxiety ? Its not human (even though it might try and grow a brain of its own at times)!
I bring this up because I got the chance to work with someone who had some questions on how to test UI code. Fairly innocuous question, and in most cases, my response would have been, keep the UI code simple and free of any logic, and don’t worry too much about it. Or you could write some nice end to end / integration tests / browser based tests. So with that response set in mind, I set off into the unknown. Little was I to know was that was the least of my concerns. As I sat down to look at the code, I saw that there were already tests for the code. I was optimistic now, I mean, how bad could things be if there are already tests for it ?
Well, I should remember next time to actually look at the tests first. But anyways, there were tests, so I was curious what kinds of tests they wanted to write and what kind of help I could provide, if any. So it turns out, the class was some sort of GUI Component, which basically had some fields, and depending on whether they were set of not, displayed them as widgets inside of it. So there were, say, 5 fields, and differing combinations of what was set would produce different output. The nice thing was that the rendered data was returned as a nice Java object, so you could easily assert on what was set, and get a handle on the widgets inside of it, etc. So the method was something along the following lines :
public RenderedWidgetGroup render() { RenderedWidgetGroup group = if (this.name = null) { return group; } group.addWidget(new TextWidget(this.name)); group.addWidget( return group; } So far, so good, right ? Nice, clean, should be easy to test if we can set up this component with the right fields. After that, it should just be a few tests based on the different code paths defined by the conditionals. Yeah, thats what I thought too. So, me, being the naive guy that I was, said, yeah, that looks nice, should be easy to test. I don’t see the problem.
Well, then I was taken to the tests. The first thing I see is a huge test. Or atleast thats what I think it is. Then I read it more closely, and see its a private method. It seems to take in a bunch of fields (Fields with names that I distinctly remembered from just a while ago) and churn out a POJO (Plain Old Java Object). Except this POJO was not the GUI Component object I expected. So obviously, I was curious (and my testing sensors were starting to tingle). So I followed through to where it was called (which wasn’t very hard, it was a private method) and lo and behold, my worst fears confirmed.
The POJO object generated by the private method was passed in to the constructor of the GUI component, which (on following it further down the rabbit hole) in its constructor did something like the following :
public MyGUIComponent(ComponentId id, And of course, readData , as you would expect, is a :
Private method Looks through the POJO If it finds a field which is not null then it sets it in the Gui Component And of course, without writing the exact opposite of this method in the unit test, it just wasn’t possible to write unit tests. And sudddenly, it became clear why they were having trouble unit testing their Gui Components. Because if they wanted to test render (Which was really their aim), they would have to set up this POJO with the correct fields (which of course, to make things more interesting / miserable, had sub POJOs with sub POJOs of their own. Yay!) to be exercised in their test.
The problem with this approach is two fold :
I need tests to ensure that the parsing and reading from the POJO logic is sound, and that it correctly sets up the GUI Component. Every time I want to test the render logic, I end up testing (unintentionally, and definitely unwantedly) the parsing logic. This also adds, as I saw, obviously complicated pre test setup logic which should not be required to test something completely different. This is a HUGE code smell . Unit testing a class should not be an arduous, painful task. It should be easy as setting up a POJO and testing a method. The minute you have to perform complicated setup to Unit test a class (Note, the keyword is unit test. You can have integration tests which need some non trivial setup.), stop! There is something wrong.
The problem here is that there is a mixing of concerns in the MyGuiComponent class. As it turns out, MyGuiComponent breaks a few fundamental rules of testability. One, it does work in the constructor . Two, it violates the law of demeter , which states that the class should ask for what it needs, not what it doesn’t need to get what it needs (Does that even make sense ?). Thirdly, it is mixing concerns . That is, it does too much. It knows both how to create itself as well as do the rendering logic. Let me break this down further :
Work in the constructor If you scroll back up and look at the constructor for MyGuiComponent, you will see it calling readData(pojo) readData(pojo) Every. Single. Time ! And it cannot be mocked out. Its a private method. And god forbid if you really didn’t care about the pojo and passed in a null. Guess what ? It most probably will blow up with a NullPointerException. So suddenly, that innocuous method in the constructor comes back to haunt you when you write yours tests (You are, aren’t you ?).
Law of Demeter Furthermore, if you look at what readData(pojo)
In this case, MyGuiComponent doesn’t really care about the POJO. It only cares about some fields in the POJO, which it then assigns to its own fields. But the constructor still asks for the POJO instead of directly asking for the fields. What this means is that instead of just creating an instance of MyGuiComponent with the required fields in my test, I now have to create the POJO with the required fields and pass that in instead of just setting it directly. Convoluted, anyone ?
Mixing Concerns Finally, what could be considered an extension of the previous one, but deserves a rant of its own. What the fundamental problem with the above class is that it is mixing concerns. It knows both how to create itself as well as how to render itself once it has been created. And the way it has been coded enforces that the creation codepath is executed every single time. To provide an analogy for how ridiculous this is, it is like a a Car asking for an Engine Number and then using that number to create its own engine. Why the heck should a car have to know how to create its engine ? Or even a car itself ? Similarly, why should MyGuiComponent know how to create itself ? Which is exactly what is happening here.
Solution Now that we had arrived at the root of the problem, we immediately went about trying to fix it. My immediate instinct was to pull out MyGuiComponent into the two classes that I was seeing. So we pulled out a MyGuiComponentFactory, which took up the responsibility of taking in the POJO and creating a GuiComponent out of it. Now this was independently testable. We also added a builder pattern to the MyGuiComponent, which the factory leveraged.
class MyGuiComponentFactory { MyGuiComponent createFromPojo(ComponentId id, class MyGuiComponent { public MyGuiComponent(ComponentId id, public MyGuiComponent setName(String name) { } And now, suddenly (and expectedly, I would like to add), the constructor for MyGuiComponent becomse simple. There is no work in the constructor. The fields are set up through setters and the builder pattern. So now, we started writing the unit tests for the render methods. It took about a single line of setup to instantiate MyGuiComponent, and we could test the render method in isolation now. Hallelujah!!
Disclaimer :
It depends how complicated the parsing logic is, and whether the POJO was specifically designed for creating the GUI component, or it comes from a completely different package/subproject.
ReplyDeleteIf the parsing is simply get() operations, and the POJO is just a simple bean that was created to avoid writing constructors with lots of arguments (here is a good discussion on that subject http://googletesting.blogspot.com/2009/02/constructor-injection-vs-setter.html) I don't see anything wrong with this approach. You could even reuse the same POJO across multiple tests. It could be a variation of a Builder pattern, where your POJO is a Builder, and GUI object simply reads fields of the Builder to create itself. And I don't see any violation of the "Law of Demeter". If the POJO is a business object from a different project, written by a different team, or parsing logic is complicated, then your refactoring is very reasonable.
Not really relevant, but i guess the fifth line of the first snippet has a typo:
ReplyDeleteif (this.name = null)
I think it should be
if (this.name == null)
(woo am i a grammernazi yet?)
@sergea, The POJO has only gets() and sets(), but the problem is the amount of conditionals in the parsing logic. If it was plain vanilla assignment from one field to the other, then it might not be troublesome. But the minute the parsing logic has to check state, see if something is present, and do something different based on whether it is there or not, it adds a whole layer of complexity, both in the code as well as in your tests.
ReplyDeleteBecause now, the test has to execute that parsing logic everytime, and that means you have to setup your POJO in a correct state, which means having to know the parsing logic most likely in an intimate way. There is no reason to add that much knowledge when it can be split and completed separately.
And @fqqdk, you are correct :D. Good catch, and yes you are a grammarnazi now ;)
Natural remedies for anxiety
ReplyDeleteStressful life peer pressure, work pressure and family issues leads to anxiety. Anxiety is basically a state of mind where people experience nervousness, fear, panic and a constant sad and depressed mood.
Deep breathing exercises are excellent for anxiety and many people report positive results from meditation. Some other natural anxiety remedies to look into are St.John's Wort, SAMe, L-Theanine, and Tryptophan.