This 30-day project explores the refactoring of a legacy system. The Everything Engine is an aging software project that powers Perl Monks, Everything 2, and a few other websites. It suffers from poor design and maintainiability. Learn what it’s like to look over the shoulder of an experienced developer as he refactors, redesigns, and updates the code.
Today’s task is removing around a thousand lines of code while improving test coverage. Really? Really!
Day 26: Fixing NodeBase Tests
Ahh, you probably thought the days of writing about porting tests were over.
Workspace means that
Workspace needs tests. If I can steal those from
NodeBase, so much the better.
That means that I have to have confidence in the
in the first place. This is sort of fulfilling and sort of not fulfilling work.
I like the results, but it’s kind of dull, at least until I hit the point where
I can change just a few lines of code and get another few dozen tests to
Fortunately, I can delete a lot of the tests that refer to methods that have
Everything::DB. Don’t worry; I can move that code into
tests for that module when I write it.
The first test method to port is
test_get_fields(). I can be a
little less specific about how it works, so I’ve cut out two unnecessary
getRef() test ports quickly. Now I can remove an
unnecessary use of
$_ in the code itself.
test_get_id() is also quick. I like having lots of small
methods. They’re easy to read, to document, and to test. I wish more of the
code were this simple. One of my refactoring goals is to make this actually
I don’t really like
test_has_permission(), but I don’t really
hasPermission() so I can live with the commutative
fix_node_keys() belongs in
That was at the end of the file, but I have more methods to test. I don’t have any particular guideline about where methods appear within a module, but I try to keep the tests in the same order in the test file. That’ll make a bigger diff in this case, but that’s fine.
The next two methods worth testing are
rebuildNodetypeModules(). The former has sufficient ugliness that
I don’t like the test (anytime you have to localize a typeglob, you’ve scared
me). The latter is easy.
I want to do more work on
loadNodetypeModule(), but I’m not
sure that code belongs here. I don’t have any better ideas though, so for now
it’ll stick around. After trying for five minutes to trigger the warning case,
I couldn’t find an easy way. That’s fine.
test_reset_node_cache() is one of the first uses of the
storage attribute today. It’s odd how little use it has been so
test_get_cache() is even simpler.
There were no tests for
getNodeById(), so I added them.
test_new_node() took moments to port.
test_get_node() was missing a few test cases at the start, so I
added three. I also found some dead code at the end of
so I removed that too. Sadly, there aren’t any tests for the rest of the code.
That needs fixing, but I’ll port the rest of the file first. (Why? I want to
get the code and tests back to where they were before I improve the coverage.
the code will provide plenty of other opportunities in the future. It’s just
beyond the scope of what I want to do here. My goal isn’t to make the code
perfect, just a little bit better than what it was when I started. That’s much
I do add tests for
getNodeZero() because they’re trivial to
write. That’s a minor perfection. (It does sound like I’m avoiding work, but
I’m trying to get through this code as quickly as possible while still making
Of course, there already were tests for that. Oops. At least the new ones are in the new style.
The last test to port is for
getNodeWhere(). Again, it’s
The only missing test is for
getNodetypeTables(). It’s easy to
write, but it takes a few minutes. That’s fine. Now 134 tests run and all of
them pass. how is the suite? Everything passes (7526 tests). From my reading of
the diff, this removes nearly a thousand lines of code, while adding more
tests. Very nice. This is checkin #882.
With that done, it should take just a moment to inherit those tests for
Workspace. That’s seven lines of code. Of course, they need more
work, but that’s good for tomorrow. (I also found two unused warnings in
How does the entire suite look? 7526 tests run. Hmm. Due to the way Build.PL works here, I have to run it again to pick up new test files. That’s fine. That’s really good to know.
After doing that, 7660 tests run and all pass. That’s excellent, and a good place to start tomorrow. Here’s checkin #883.