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. No! Subclassing NodeBase with 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 NodeBase tests 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 pass.

Fortunately, I can delete a lot of the tests that refer to methods that have moved into 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 tests.

The 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 possible.

I don’t really like test_has_permission(), but I don’t really like hasPermission() so I can live with the commutative distaste.

The untested fix_node_keys() belongs in Everything::DB. Oops.

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 joinWorkspace() and 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 far.

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 getNode(), 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. Running Devel::Cover on 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 more attainable.)

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 improvements.)

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 easy.

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 Workspace. Oops.)

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.