Monday, October 31, 2011

Chasing Technology

Maybe I'm just getting old, but keeping up with technology seems to be getting harder and harder. As I've mentioned before, I am a big fan of learning your libraries, languages, and tools. But how are you supposed to do that, when the libraries keep upgrading and there are constantly new tools to use? Just this past week there was a post on slashdot suggesting that tech skills have a 2 year half life. The comments on that thread seem to mostly disagree with the hypothesis, but there is definitely some truth to it.

The two languages that I use the most at work did not exist when I started college. However, languages are easy to keep up with. They tend to move slowly and are very well documented. Tools, libraries, and frameworks, on the other hand, are a nightmare to chase. They are constantly being upgraded, and often times the best documentation are the question/answers out there on the web at places like stackoverflow. The problem with these sites is that they are only useful after a tool has been out long enough for a critical mass of people to use.

So what's a person supposed to do?

Well, first, as many commenters from the slashdot article stated, the most valuable skills a software engineer has is knowing how to design, develop, and debug software. These skills don't go away just because you are using a new language or library. So play to your strengths. If you design and write clean code, it'll be easier to fix/update as you learn the language/library/tool better.

Second, choose your upgrades carefully and intentionally. Just because there is a newer version of something out there, doesn't mean you need to use it. Are the benefits worth it to upgrade now? Does it make more sense to wait until it has been popularly adopted? Or maybe it makes sense to skip a version or two.

Third, don't become an expert in everything. Most of us have finite amounts of energy and time. So, while I'm a big believer in being an expert in the tools you are using, that isn't always practical. If a particular tool/library/framework is bound to be upgraded/replaced in the near future, it's probably ok if you use the existing library in a less than optimal way. Focus your energies on the timeless (design and coding skills) or the long lasting (languages).

constantly learnFourth, and most importantly, constantly learn. If your employer doesn't provide opportunities for professional growth, take it anyway. Just as you don't ask for permission for every single test that you write, or every whiteboard design that you draw, you don't need permission to explore new technologies and tools. Whether they know it or not, your employer pays you to be a good and competent software developer. This requires you to be constantly learning new and better ways of doing things. If you are not taking ten to twenty percent of your time exploring and learning, you are shortchanging yourself and making yourself obsolete. I realize that it seems there is never time to do this, but like other aspects of good software engineering (appropriate design, testing, documentation), in the long run you will be worse off if you don't make the investment now.

Monday, October 10, 2011

Vexing Bugs

While bugs are a part of development, there are a few types of bugs that I find particularly vexing: intermittent bugs, library bugs, and bugs that only happen in production.

Intermittent Bugs
It's annoying to perform the exact same set of steps 4 times, have it work 3 of the times, and fail once. When Murphy has his way, it works when you, perform the steps or are watching, but fails when the user does it on their own. How do you diagnose a bug that you can not reliably reproduce?

Library Bugs
The majority of library bugs aren't actually bugs in the library, but rather a bug in how you are using the library. Either way, having cryptic error messages coming out of the bowels of a library, when there is no apparent connection between the error and your code, is not fun. I find the process of googling error messages or library usages to be much more frustrating than tracking down errors that are entirely in my own source code.

Production Bugs
While we strive to have our development environment similar to the production environment, there are certain discrepancies that always creep in. Things like debug information, optimization level, database source, etc. When in development, you don't want to be messing with production data, and your willing to give up some performance to better track the code. But since it isn't the same, code that works fine in development doesn't always work in production. Ugh.

This Week
So what prompted this post? Well, this week I had a bug that was at the intersection of these bugs. We had deployed a new version of our application, and suddenly I started getting server emails about errors. It was some cryptic error happening within the library we use for making Web Service calls. And, of course, the errors only happened sometimes. Even more frustrating, when trying to reproduce the error on my local box using WEBrick in development mode, the error never happened.

After setting up a development server on my local box that was configured just like the production box, I was able to intermittently reproduce the error. Using my standard debugging technique of adding printfs, I eventually noticed output that looked like:

Starting Web Service Call 1
Starting Web Service Call 2
Exception Caught from Web Service Call 2
Ending Web Service Call 1

Unfortunately, it didn't catch my eye right away, but I eventually noticed that every time there was an error, the web service calls were getting interleaved. i.e. Web Service Call 2 was starting before Web Service Call 1 started. And now it suddenly made sense. 

The two web service calls were being made as the result of two separate AJAX callbacks from a web page. Since the the two web service calls were being done in two different web requests, they were being handled in parallel resulting in a race condition. As it turns out, the web service library we were using is not thread safe. However, apparently WEBrick in development mode was serializing these requests (i.e. not processing 2 until 1 was completely handled), and so in development there were no threading issues. 

As with most bugs, once the problem had been diagnosed, it wasn't that hard to fix. In this case, we put the web service calls in a critical section, forcing them to be serialized. For our particular use case, the higher latency of serializing the web service calls was acceptable, allowing for a fairly simple solution.

Moral
This post wasn't really intended to have a moral. It was mostly just me talking about my week, but I suppose there are a couple good ideas.
  • If you think its a library bug, you are probably misusing the library. (i.e. Learn your libraries)
  • Intermittent errors are indicative of threading issues.
  • Know the differences between your development and production environments.
    oh, and maybe most importantly of all
  • When frustrated, take a break. I didn't actually track down the bug until I walked away from it for a while and then came back to it with fresh eyes.

Monday, September 26, 2011

Learn Your Libraries

Story 1
I still remember the month long final project from my first ever CS class. Actually, I remember almost nothing from the project except for a bug that caused me to have to throw away my first three weeks of work. I had just learned about enumerations in Pascal and thought they were the neatest thing since sliced bread. I designed my entire project around enumerations. Users would enter their menu choices via enumerations. I would display the enumerations as options. All of this depended very heavily on my understanding of enumerations which, as it turned out, was in no way related to what enumerations actually are.

Unfortunately, I didn't find this out until I had spent two weeks writing the entire program and tried running it. It didn't work at all. Like any good developer I randomly changed various lines of code until it worked. A week later, with the project deadline looming, my project was not any closer to working. It was time for drastic action. Since I couldn't seem to get the enumerations to do what they were "supposed" to do, I actually read up on them. Imagine my dismay when I realized that all the assumptions that I had made were wrong. And not just a little bit wrong. The reality (enums are a typesafe way to have named constants) and my vision (they'll do everything I want, including cooking dinner and solving the halting problem) were completely unrelated. I spent the rest of the day griping and complaining about why would anybody create such a useless design feature. When I realized that that wasn't getting me any closer to a program that I could actually turn in, I finally sat down and rewrote the entire program using only Pascal features that I actually understood, and eschewed enumerations entirely.

Story 2
The other day a friend asked me what I knew about Java Serialization. After asking for details, it turns out that he was working on a project where they were doing bit-diddling on the Serialized output of an object. One of the assumptions that they had was that if you had an objects instance1 and instance2, and you serialize both of them, that the serialized byte data would be identical if instance1 and instance2's fields are all identical. They had run a bunch of experiments and it seemed like their assumption wasn't always holding. I think the question to me was hoping that I would point out something they had missed that would give them an easy fix.

Upon check the Serialization Spec, it turns out their assumption was not valid. My suggested solution was to not use Java's Serialization, but rather write their own conversion to bytes that would fit their specific needs. This may seem like reinventing the wheel, but as I've mentioned before, I believe that rolling your own is often the way to go.

Story 3
On a project I once worked on, the decision was made that all data was going to be handled as XML. All of the data was stored as an XML document in a single column in a database. To process individual data items, XPath queries would be made. HTML reports would be created by running giant XSLT transforms on the data. The assumption was that because XML has such great support, it would be easy to add new data (just turn it into XML and merge it with the other data), and create new reports (just write a new XSLT).

The reality is that just because you can create a general tool to handle syntactically correct XML, you still need to customize the logic for the semantics. Which meant every new data source added still required code and logic changes. As for generating the HTML via XSLT transformations, its very difficult to use modern HTML features, like Ajax, this way. And by making such a large portion of the code base XSLT and XPath, we couldn't take advantage of the IDE's ability to do easy code completion, navigation, and refactoring, or leverage the development team's OO skill to create small, easily testable, and reusable components. And then there's the performance issues, both in time and space, that were encountered with trying to manipulate and store large quantities of large XML documents.

Moral
The moral of these stories is that you need to know your tools. Enumerations, Serialization, and XML are all great, when used appropriately. When not... well, invalid assumptions lead to bugs. Invalid assumptions about crucial components of your software lead to large scale rewrites. To avoid the large scale rewrite, we often have a period of denial where we try to make the unworkable work. The worst case is that with enough bubble gum, chicken wire, and ingenuity we do make it work, at least some of the time. This results in a maintenance nightmare keeping it working and large scale rewrites are even less likely to happen once you've got "working" code.

While we all know that assumptions can be bad, the danger is when we don't realize we are making assumptions. In the first story I thought I know what enumerations did. The developers in the second story had used Java Serialization on many projects and thought they fully understood it. In the third story the benefits of XML were considered without awareness of all of the limitations and restrictions that came along. i.e. We didn't know what we didn't know.

The conservative solution to this problem is to only use technologies you have already used successfully on all your projects. While this seems to be favored by many people, I am too much a fan of shiny objects and so like to try new things. The key to using new technologies (or existing ones in new ways) is to limit the scope until you have used it successfully. If it is going to be integral to your design, create a small throw-away prototype first to make sure you find the dark corners and sharp edges first.

The most important solution, though, is to be willing to admit you were wrong. Its impossible to get very far in life without making assumptions, and despite your best precautions, sooner or later, you will be wrong about an assumption. Rather than try to patch on hack after hack to try to make a bad assumption viable, be willing to break from the past and rewrite/redesign code given your new knowledge. Even if it means throwing away a lot of work.

Monday, September 19, 2011

RSpec Lessons Learned

Previously I showed some unit tests written in RSpec. Those test were fairly ugly, partially because I didn't really understand RSpec. In an effort to have better unit tests, I have learned a little more about RSpec.

Read the Latest Documentation
You'd think this would be obvious, but given the lack of links to the documentation on the various "here's advice on using RSpec" blogs, it must not be done a lot. Worse, somehow I had been only looking at the old documentation for version 1 of RSpec. So, if you are going to use RSpec, I suggest you read latest documentation, which as of when I am writing this is RSpec 2.6. This will definitely be helpful.

Make Tests Self Documenting
Each test (it or specify block) can take a description. Sometimes this is necessary, but any time you have documentation (and that's what this description is), you risk having the documentation be out of sync with the code. While not everyone agrees, I am a fan of letting the tests document themselves, whenever possible.

Make Use of Context
Use the "context" keyword to describe what you are doing in a "before" block to set up the state necessary for testing.
describe Thing do
  let(:thing) {Thing.new}
  subject {thing}
  context "empty object" do
    #insert tests
  end
  context "with inherited object" do
    let(:base) {Thing.new}
    before(:each) do
      base["foo"] = 5
      thing.prototype = base
      thing["baz"] = 15
    end
    #insert tests
  end
end
Make Use of Describe
Use the "describe" keyword to describe either the noun that is being tested, or the actions that are under test. i.e. if you have actions in a "before" block that are the actions being tested, you should you use "describe" rather than "context"
describe Thing do
  let(:thing) {Thing.new}
  subject {thing}
  context "empty object" do
    describe "when assigning via properties" do
      before(:each) do
        thing["foo"] = 5
        thing["bar"] = "hello"
      end
      # insert checks
    end
  end
end
Make Use of Subject
If you have multiple tests on a single object, make it the RSpec Subject and put it in a describes block. This way, all of your "should" comparisons will be implicitly on this object.
describe Thing do
  let(:thing) {Thing.new}
  subject {thing}
  context "empty object" do
    describe "keys" do
      subject {thing.keys}
      it {should_not be_nil}
      it {should be_empty}
    end
  end
end
Use Its
Often times you want to test the properties of an object. You can use the "its" method to have the implicit subject of "should" comparisons be the result of the method or array dereference specified.
describe Thing do
  let(:thing) {Thing.new}
  subject {thing}
  context "empty object" do
    its(['newProperty']) {should be_nil}
    its('newMethod') {should be_nil}
  end
end
Let and Subject are Lazy Loaded
The "variables" defined in "let" calls and the "subject" aren't actually evaluated until they are used. So if you never reference a variable specified in a "let", then that code is never executed. This also means that order isn't important.  i.e. the following will work:
let (:a) {b + 1}
let (:b) {5}
specify("show using let") {a.should == (b+1)}
Shared_examples and shared_contexts are Global
I actually haven't found this documented, so I may be doing something wrong. But I have found that if I have two different rspec files that each have a "shared_examples_for 'test this object'", this causes problems. I can test each rspec file in isolation and it is fine. But if I try to test both at the same time, I get an error saying that a shared example already exists with the name "test this object".

There are two different solutions to this problem that I have found. If the shared examples are the same, pull them into a common Module that is included. Note, this is better than having repeated code anyway. If the examples are different, then you have to be more unique with the names of the shared examples.

Final Thoughts
It seems that the approach of RSpec is to write tests such that they are self descriptive and so that each test tests exactly one thing. While in theory, this sounds good, I am finding that this results in very verbose test files. That's even with the cleaning up that I have done after learning RSpec better.  i.e. I like a lot of what RSpec does, but I am not convinced that it is the best way to write unit tests.

Rewritten Tests
Using what I have learned, I have rewritten the unit tests from before. Here is what they look like now:

require 'spec_helper'

describe Thing do
  let(:thing) {Thing.new}
  subject {thing}

  shared_examples_for "simple object" do |map, self_keys|
    describe "keys" do
      subject {thing.keys}
      it {should have(map.size).items}
      it {should include(*(map.keys))}
      it {should_not include("noSuchProperty")}
    end
    describe "self_keys" do
      subject {thing.self_keys}
      before(:each) { self_keys ||= map.keys}
      it {should have(self_keys.size).items}
      it {should include(*self_keys)}
      it {should_not include("noSuchProperty")}
    end
    describe "fields" do
      map.each do |k, v|
        its([k]) {should == v}
      end
      its(['noSuchProperty']) {should be_nil}
    end
    describe "methods" do
      map.each do |k, v|
        its(k) {should == v}
      end
      its('noSuchMethod') {should be_nil}
    end
    describe "to_hash" do
      subject {thing.to_hash}
      it {should_not be_nil}
      it {should have(map.size).items}
      it {should include(*map.keys)}
      it {should include(map)}
    end
  end
  
  context "empty object" do
    describe "keys" do
      subject {thing.keys}
      it {should_not be_nil}
      it {should be_empty}
    end
    its(['newProperty']) {should be_nil}
    its('newMethod') {should be_nil}
    describe "to_hash" do
      subject {thing.to_hash}
      it {should_not be_nil}
      it {should have(0).items}
    end
    
    describe "when assigning via properties" do
      before(:each) do
        thing["foo"] = 5
        thing["bar"] = "hello"
      end
      it_should_behave_like "simple object", 'foo'=>5, 'bar'=>"hello"
    end
    
    describe "when assigning via methods" do
      before(:each) do
        thing.foo = 5
        thing.bar = "hello"
      end
      it_should_behave_like "simple object", 'foo'=>5, 'bar'=>"hello"
    end
  end
  
  context "with inherited object" do
    let(:base) {Thing.new}
    before(:each) do
      base["foo"] = 5
      base["bar"] = "hello"
      thing.prototype = base
      thing["baz"] = 15
      thing["bye"] = "bye"
    end
    it_should_behave_like "simple object", 
      {'foo'=>5, 'bar'=>"hello", 'baz'=>15, 'bye'=>"bye"}, ['baz', 'bye']
    describe "when overriding values" do
      before(:each) do
        thing["foo"] = 25
        thing["bar"] = "hola"
      end
      it_should_behave_like "simple object", 'foo'=>25, 'bar'=>"hola", 'baz'=>15, 'bye'=>"bye"
    end
  end
end

Below is what the output looks like. As you can see, if you read it, it describes the tests that are being run more clearly than the old version of the tests.

$ rspec spec/models/thing_spec.rb 

Thing
  empty object
    keys
      should not be nil
      should be empty
    ["newProperty"]
      should be nil
    newMethod
      should be nil
    to_hash
      should not be nil
      should have 0 items
    when assigning via properties
      it should behave like simple object
        keys
          should have 2 items
          should include "foo" and "bar"
          should not include "noSuchProperty"
        self_keys
          should have 2 items
          should include "foo" and "bar"
          should not include "noSuchProperty"
        fields
          ["foo"]
            should == 5
          ["bar"]
            should == "hello"
          ["noSuchProperty"]
            should be nil
        methods
          foo
            should == 5
          bar
            should == "hello"
          noSuchMethod
            should be nil
        to_hash
          should not be nil
          should have 2 items
          should include "foo" and "bar"
          should include {"foo"=>5, "bar"=>"hello"}
    when assigning via methods
      it should behave like simple object
        keys
          should have 2 items
          should include "foo" and "bar"
          should not include "noSuchProperty"
        self_keys
          should have 2 items
          should include "foo" and "bar"
          should not include "noSuchProperty"
        fields
          ["foo"]
            should == 5
          ["bar"]
            should == "hello"
          ["noSuchProperty"]
            should be nil
        methods
          foo
            should == 5
          bar
            should == "hello"
          noSuchMethod
            should be nil
        to_hash
          should not be nil
          should have 2 items
          should include "foo" and "bar"
          should include {"foo"=>5, "bar"=>"hello"}
  with inherited object
    it should behave like simple object
      keys
        should have 4 items
        should include "foo", "bar", "baz", and "bye"
        should not include "noSuchProperty"
      self_keys
        should have 2 items
        should include "baz" and "bye"
        should not include "noSuchProperty"
      fields
        ["foo"]
          should == 5
        ["bar"]
          should == "hello"
        ["baz"]
          should == 15
        ["bye"]
          should == "bye"
        ["noSuchProperty"]
          should be nil
      methods
        foo
          should == 5
        bar
          should == "hello"
        baz
          should == 15
        bye
          should == "bye"
        noSuchMethod
          should be nil
      to_hash
        should not be nil
        should have 4 items
        should include "foo", "bar", "baz", and "bye"
        should include {"foo"=>5, "bar"=>"hello", "baz"=>15, "bye"=>"bye"}
    when overriding values
      it should behave like simple object
        keys
          should have 4 items
          should include "foo", "bar", "baz", and "bye"
          should not include "noSuchProperty"
        self_keys
          should have 4 items
          should include "foo", "bar", "baz", and "bye"
          should not include "noSuchProperty"
        fields
          ["foo"]
            should == 25
          ["bar"]
            should == "hola"
          ["baz"]
            should == 15
          ["bye"]
            should == "bye"
          ["noSuchProperty"]
            should be nil
        methods
          foo
            should == 25
          bar
            should == "hola"
          baz
            should == 15
          bye
            should == "bye"
          noSuchMethod
            should be nil
        to_hash
          should not be nil
          should have 4 items
          should include "foo", "bar", "baz", and "bye"
          should include {"foo"=>25, "bar"=>"hola", "baz"=>15, "bye"=>"bye"}

Finished in 0.0831 seconds
78 examples, 0 failures

Monday, September 12, 2011

Birth of a Bug

This is a a story of a bug. It was a simple bug. Like many simple problems, on the surface it had a simple cause. Also like many simple problems, in reality it had many contributing factors. Before I tell you about the bug, let me tell you about the application.

let people configure which emails they getAt my place of employment we have various electronic forms that need to be signed by people. For example, if you want to purchase something, there are people who need to approve that purchase - probably your supervisor plus whoever has signature authority for the project you are billing the purchase to. To keep the process flowing people get emails letting them know when they have a form to sign. As it turns out, not everyone wants these emails, and sometimes people only want the emails under certain circumstances. So I wrote an app to let people configure which emails they get.

I deployed this app the other day after work, and promptly the next morning people started using it to turn off email notifications. Shortly thereafter they started sending in complaints that they were still receiving email.

Why didn't it work? How did such a fundamental bug get into the application?

Before answering that question, I'll tell you what the bug was. One of the features of the configuration is that people could turn off email notification, but add a financial exception. As an example, some people only want to get email notifications for requests that are for more than $10,000. Somewhere in the user's NotificationPreferences object is a line that looks similar to:
boolean checkFinancialOverride(Double amount) {
 return (amount >= mMoney);
}
where amount is the amount the form is for ($53.24 stapler), and mMoney is the threshold the user set ($10,000). There's a catch though - what is mMoney if the user didn't specify a threshold? Turns out that there are two possible values, depending on how the value got set. If they never have set any preferences at all, this value is NULL, but if they have set a preference for configuration, but never set a threshold, this value gets set to 0. This means that we do NOT want to send a financial override if the mMoney value is either NULL or 0. The actual code looked like:
boolean checkFinancialOverride(Double amount) {
  return mMoney != null && amount != null && mMoney > 0 && amount >= mMoney;
}
Don't bother looking for the bug in that code, as the bug isn't there. At least not yet. It didn't show up until deployment day. While the bug was simple, there were a number of "best practices" that were violated that led to the birth of this bug.

Don't Make Last Minute Changes
The plan was to deploy the code at 5PM. At 3PM, I had a coworker who suggested that "amount" was a better name for the database column for the threshold than "money" because "amount" is consistent with other projects. (and, of course, "mAmount" would be the Java variable containing the threshold value.)

Anytime you make a change, the risk of breaking something is non-zero. Make the change at the last minute and you won't have time to exhaustively test it. As you have figured out, we broke the code. Then in my "smoke testing" I only chose examples that didn't trigger the new bug. Which is how I unknowingly release code with this bug.

This wasn't a critical user bug. Heck, this wasn't anything that was going to affect the user in any way, shape, or form. I never should've agreed to make this change.

Beware of Semantic Code Changes
When making this last minute name change from "money" to "amount" we split the work up between me and the developer who recommended the change, even though all the code involving this had been written by me. This means that he was changing code that he wasn't familiar with. Here is what the updated code looked like:
boolean checkFinancialOverride(Double amount) {
  return mAmount != null && amount != null && amount >= mAmount;
}
As you can see, the condition for checking if "mAmount > 0" got dropped. Why was it dropped? I haven't asked, so I don't know. I don't know if it was accidental or intentional. Either way, there are related best practices that should have been followed.

Understand Your Changes
Ever run across code in a working system that just can't be right? Before just assuming that the bug is being hidden by other functionality and going ahead and fixing the code, make sure you understand exactly what is happening. If you have access to the original developer (in this case I was just across the hall, a slightly raised voice away), ask why the code is the way it is. It might be a bug, but you might also not be understanding it.

Leverage Refactoring Tools
If you are just renaming a variable, let your IDE rename it for you. This is the type of change that can almost always be done automatically by tools. Use those tools.

Check Your Changes Before Committing
This is something I am a big enough believer in that I may turn it into its own blog post at some point. After making code changes, no matter how large or how trivial, run diff on the code before checking it in. Verify that every change that you are committing is one that you mean. This serves as a simple and quick code review, as well as ensuring that you don't accidentally commit left in debugging statements. Not to mention catching those places where you accidentally inserted or deleted code because of careless clicks.

Run Your Unit Tests
After making any changes, no matter how trivial, make sure you rerun your unit tests. Especially before deploying. You do have unit tests, don't you? Ok, I'll admit it, we had no unit tests on this project, and this is exactly the type of bug that would've gotten caught by unit tests.

Write Understandable Code
While you can follow all the best practices under the sun, you can't force your coworkers to do the same. The best thing you can do to defend against that is to write code that is not only correct but understandable and obvious. Let's look at that query one more time:
boolean checkFinancialOverride(Double amount) {
  return mMoney != null && amount != null && mMoney > 0 && amount >= mMoney;
}
Checking for null is fairly obvious as it prevents NullPointerExceptions when the autounboxing happens. The comparison "amount >= mMoney" is the comparison that is needed. But what's with the "mMoney > 0"? If I hadn't explained it above, would you have guessed why it is here? Maybe, maybe not, either way I would argue that this is a piece of non-obvious code. I will offer up two possible suggestions for how this code could be made more obvious. The first is to add a comment:
boolean checkFinancialOverride(Double amount) {
  // if mMoney <= 0, it is not a valid value and should be ignored
  return mMoney != null && amount != null && mMoney > 0 && amount >= mMoney;
}
But since I think comments are often a sign of confusing code, my other suggestion involves changing the code:
boolean checkFinancialOverride(Double amount) {
  return isThresholdValid() && amount != null && amount >= mMoney;
}

boolean isThresholdValid() {
  return mMoney != null && mMoney > 0
}

Conclusion
Write clean code. Don't make last minute changes. Understand your changes. Write unit tests and use them. These are the steps to avoid unexpected and avoidable bugs.

Monday, September 5, 2011

Unit Testing a Simple Property Model

Previously I showed the beginning of a Ruby implementation of a simple property model. Well, I decided to create some unit tests to help verify that my property model correctly handles properties, especially with the prototype delegation.

RSpec seems to be popular in the rails world, so I used rspec to write my tests. This exercise provided me with examples of the good, the bad, and the ugly with unit tests. The good was straightforward. I don't have an application yet, just this simple library and unit tests provided an easy way to test it. Even better, at one point I changed an implementation detail that required some rewriting of the property model, and by rerunning the tests, I had confidence that I didn't break anything.

The ugly? This is definitely a good example of ugly. I have the unit testing code at the bottom of this post. I found I had a lot of duplication, so I created "shared_examples_for" to reduce redundancy. However, the resulting test code is still really long and while the shared examples helps with sharing tests, I am not sure they help with readability. Now, as Trevor commented, its possible (ok, likely) that the unit tests are ugly because they weren't written well. I will admit to being a newbie at RSpec. I am sure that they could be written better, but I am not convinced they wouldn't still be somewhat ugly.

The bad? Well, there isn't too much bad, yet. It did take time to create the tests, but I had to validate my code somehow. And it is possibly (likely?) that my tests aren't as thorough as they should be, so maybe I have some false confidence in the correctness of my code. But all in all, despite what I said last paragraph, this wasn't that good of an example of the bad. At least not yet. The bad will come when I spend time trying to clean up these tests, rather than using that time to write new code.

And without further ado, here are these unit tests.

require 'spec_helper'

describe Thing do
  let(:foo) {5}
  let(:bar) {"hello"}

  shared_examples_for "simple properties" do
    describe "keys" do
      it {thing.keys.size.should == 2}
    end
    describe "foo" do
      it {thing['foo'].should == foo}
      it {thing.foo.should == foo}
    end
    describe "bar" do
      it {thing['bar'].should == bar}
      it {thing.bar.should == bar}
    end
    describe "baz" do
      it {thing['baz'].should be_nil}
      it {thing.baz.should be_nil}
    end
    describe "to_hash" do
      it {thing.to_hash.should_not be_nil}
      it {thing.to_hash.size.should == 2}
      it {thing.to_hash['foo'].should == foo}
      it {thing.to_hash['bar'].should == bar}
    end
  end

  shared_examples_for "nested properties" do
    describe "keys" do
      it {thing.keys.size.should == 4}
      it {thing.self_keys.size.should == 2}
      it {thing.self_keys.include?('b2').should be_true}
      it {thing.self_keys.include?('bye').should be_true}
      it {thing.self_keys.include?('bar').should be_false}
    end
    describe "foo" do
      it {thing['foo'].should == 5}
      it {thing.foo.should == 5}
    end
    describe "bar" do
      it {thing['bar'].should == "hello"}
      it {thing.bar.should == "hello"}
    end
    describe "b2" do
      it {thing['b2'].should == 15}
      it {thing.b2.should == 15}
    end
    describe "bye" do
      it {thing['bye'].should == "bye"}
      it {thing.bye.should == "bye"}
    end
    describe "baz" do
      it {thing['baz'].should be_nil}
      it {thing.baz.should be_nil}
    end
    describe "to_hash" do
      it {thing.to_hash.should_not be_nil}
      it {thing.to_hash.size.should == 4}
      it {thing.to_hash['foo'].should == 5}
      it {thing.to_hash['bar'].should == "hello"}
      it {thing.to_hash['b2'].should == 15}
      it {thing.to_hash['bye'].should == "bye"}
    end
  end
 
  describe "no inheritance" do
    let(:thing) {Thing.new}
    subject {thing}

    describe "new object" do
      describe "keys" do
        it {thing.keys.should_not be_nil}
        it {thing.keys.should be_empty}
      end
      describe "properties" do
        it {thing['foo'].should be_nil}
        it {thing.foo.should be_nil}
      end
      describe "to_hash" do
        it {thing.to_hash.should_not be_nil}
        it {thing.to_hash.size.should == 0}
      end
    end
   
    describe "adding properties to object" do
      before(:each) do
        thing['foo'] = 5
        thing['bar'] = "hello"
      end
      it_should_behave_like "simple properties"
      it {thing.self_keys.size.should == 2}
      it {thing.self_keys.include?('foo').should be_true}
      it {thing.self_keys.include?('bar').should be_true}
      it {thing.self_keys.include?('baz').should be_false}
    end
    describe "adding fields to object" do
      before(:each) do
        thing.foo = 5
        thing.bar = "hello"
      end
      it_should_behave_like "simple properties"
      it {thing.self_keys.size.should == 2}
      it {thing.self_keys.include?('foo').should be_true}
      it {thing.self_keys.include?('bar').should be_true}
      it {thing.self_keys.include?('baz').should be_false}
    end
  end

  describe "inheritance" do
    let(:thing) do
      base = Thing.new
      base.foo = 5
      base['bar'] = 'hello'
      foo = Thing.new
      foo.prototype = base
      foo
    end
    subject {thing}
 
    describe "new object" do
      it_should_behave_like "simple properties"
      it {thing.self_keys.include?('bar').should be_false}
      it {thing.self_keys.include?('baz').should be_false}
    end
   
    describe "adding properties to object" do
      before(:each) do
        thing['b2'] = 15
        thing['bye'] = "bye"
      end
      it_should_behave_like "nested properties"
    end
    describe "adding fields to object" do
      before(:each) do
        thing.b2 = 15
        thing.bye = "bye"
      end
      it_should_behave_like "nested properties"
    end
    describe "adding redundant fields to object" do
      let(:foo) {15}
      let(:bar) {"bar2"}
      before(:each) do
        thing.foo = foo
        thing.bar = bar
      end
      it_should_behave_like "simple properties"
      it {thing.self_keys.size.should == 2}
      it {thing.self_keys.include?('foo').should be_true}
      it {thing.self_keys.include?('bar').should be_true}
      it {thing.self_keys.include?('baz').should be_false}
    end
  end
end


And here is the output of running the above tests:


$ rspec spec/models/thing_spec.rb

Thing
  no inheritance
    new object
      keys
        should not be nil
        should be empty
      properties
        should be nil
        should be nil
      to_hash
        should not be nil
        should == 0
    adding properties to object
      should == 2
      should be true
      should be true
      should be false
      it should behave like simple properties
        keys
          should == 2
        foo
          should == 5
          should == 5
        bar
          should == "hello"
          should == "hello"
        baz
          should be nil
          should be nil
        to_hash
          should not be nil
          should == 2
          should == 5
          should == "hello"
    adding fields to object
      should == 2
      should be true
      should be true
      should be false
      it should behave like simple properties
        keys
          should == 2
        foo
          should == 5
          should == 5
        bar
          should == "hello"
          should == "hello"
        baz
          should be nil
          should be nil
        to_hash
          should not be nil
          should == 2
          should == 5
          should == "hello"
  inheritance
    new object
      should be false
      should be false
      it should behave like simple properties
        keys
          should == 2
        foo
          should == 5
          should == 5
        bar
          should == "hello"
          should == "hello"
        baz
          should be nil
          should be nil
        to_hash
          should not be nil
          should == 2
          should == 5
          should == "hello"
    adding properties to object
      it should behave like nested properties
        keys
          should == 4
          should == 2
          should be true
          should be true
          should be false
        foo
          should == 5
          should == 5
        bar
          should == "hello"
          should == "hello"
        b2
          should == 15
          should == 15
        bye
          should == "bye"
          should == "bye"
        baz
          should be nil
          should be nil
        to_hash
          should not be nil
          should == 4
          should == 5
          should == "hello"
          should == 15
          should == "bye"
    adding fields to object
      it should behave like nested properties
        keys
          should == 4
          should == 2
          should be true
          should be true
          should be false
        foo
          should == 5
          should == 5
        bar
          should == "hello"
          should == "hello"
        b2
          should == 15
          should == 15
        bye
          should == "bye"
          should == "bye"
        baz
          should be nil
          should be nil
        to_hash
          should not be nil
          should == 4
          should == 5
          should == "hello"
          should == 15
          should == "bye"
    adding redundant fields to object
      should == 2
      should be true
      should be true
      should be false
      it should behave like simple properties
        keys
          should == 2
        foo
          should == 15
          should == 15
        bar
          should == "bar2"
          should == "bar2"
        baz
          should be nil
          should be nil
        to_hash
          should not be nil
          should == 2
          should == 15
          should == "bar2"

Finished in 0.06217 seconds
106 examples, 0 failures