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 still more work porting the node tests to Test::Class. When will it all end?

Day 09

As usual, today’s job is to continue porting the test for the node class to Test::Class. My last realization was that the remaining tests to port all handle database operations and all have to test code branches pertaining to workspaces.

Eventually these tests will go in their own class, but not today, so today I have to test them. Ugh.

The first method to test is nuke(), which removes a node from the database. I wish I had a test database… but soon. The first test method to write is test_nuke_access(). It looks much like its corresponding update() test method.

Everything passed on my first try. That’s nice.

Looking at nuke() again, most of the rest of the code has no particular branch conditions. There are only a couple of loops to erase data in other tables. That’s nice, at least for testing. Of course, it means that the nuke() testing method may be long, unless I find a good place to split it. (Arguably, I should wait until a node subclass needs to override something. I think I will, unless something really jumps out at me.)

Along the way, I noticed that whenever I need to use my mock database object, I always have to write $node->{DB} = $db. Why not do that in the setup fixture? Yep! Then I can delete the remaining assignments. Everything should still pass… and it does.

Making the remaining tests pass is trickier — I hit an infinite loop somewhere in Everything::Node. I think this means that there’s an unmocked method somewhere running afoul of the AUTOLOAD() there. It’s just a matter of tracking it down.

Again, a lot of the work here is in untangling the reuse of one mock object in many circumstances. It’s difficult to tell which is the node object and which is the database object, at least without editing the code and running the tests again. In this case, I really do want to see test failures, as they’ll tell me I’m on the right track.

After organizing that as far as it seems to make sense, the first call to nuke() still causes an infinite loop. This means I have to figure out where things go wrong within that method… oh well.

(This would be much easier with triggers in the database. There’d be almost no code in this method then. The method is long enough that I don’t have a lot of hope of anyone successfully recreating it the first time within another program that wants to use this database, so it seems like a clear benefit to put as much of the delete logic in the database, with proper foreign key support, as possible. I realize that it’s heresy in web development communities today to suggest taking advantage of database smarts, but do any of the people arguing that position actually try to test and maintain their code? I’m a bitter guy today.)

Tracking down the loop is much more difficult than I want to imagine right now. Somehow, the flow control necessary to loop over all of the joined tables for all of the nodetypes this node represents looks correct. Yet something somewhere is wrong.

My false laziness bit me again. Instead of going straight to the AUTOLOAD() in Everything::Node to see the method being called and redispatched, I tried to trace it down. Oh well — what does it show? getParentType(). Hm, where is that call? It’s nowhere in nuke(). It’s another place to look though. Adding a caller() check reveals that it’s in Everything::Node line 397 — getNodeMethod().

Uh oh. That code hits the database, looking for a method defined not in a .pm file.

Aha. I stepped back for a minute to check my assumptions and realized that I’d moved one mock from $node to $db inappropriately. Moving it back should fix the infinite loop. It does. Great. Too bad I wasted 20 minutes figuring that out. One test still fails though, because I had another error in that mock.

Oops.

I wonder now if Test::MockObject::Extends should have an option to ignore a parent AUTOLOAD(). Then again, it’s only the fact that Everything has its own method dispatch system (at least for now) that makes this problematic.

After editing a few more test calls to use the right mock object, they pass too. So far so good. I like to work on batches of test calls — a handful at a time.

One of the next batch of tests failed because there was no node_id set on the node. This test apparently expected the node_id set in the update() tests to have persisted. That obviously didn’t work here. That’s fine. I took the chance to change the expected node_id value, just to be sure it only passes because I want it to pass.

The rest of the test porting was just untangling the different mock object uses. Now 127 tests pass. This was a long and involved method.

What’s next? xmlTag() is fairly short, as far as the tests go. This should go much more quickly.

After writing the method framework and changing $mock to $node, only one test fails. How nice! (I also noticed that I don’t have to override Everything::logErrors() specifically in this code, as the startup code already does it. I can update another test method I saw on the way to work with this too, after I get this method passing.)

Hm, the failing test is the one that uses the error log. That’s odd. Is the setup incorrect? It is — the setup fixture doesn’t clear it between tests. That’s fixable.

With that fixed (and one more piece of data set correctly for the test), another 9 tests pass. That was much faster. Now to fix the other logging, in test_restrict_title(). Done! That’s the only other use of logErrors() in a test, and that gets rid of a couple of lines of scary test code. Great!

I’ve converted 65% of the test file so far. Wow, this is taking much longer than I had in mind. That’s okay. I’m making progress, which is important.

That took up my hour. It looks like perhaps two more days of work should finish this file. Hopefully the remaining node tests will be much easier to write. Almost every node class is a third the size of this one — or less, with the exception of nodegroup, and only twelve of the 28 nodes have any code worth considering. Things should move much more quickly now.

After undoing the test launcher (so that everything passes and uses the old tests, as the behavior hasn’t changed), everything still passes. Great. It’s time to check in again.

Hm, SVK gives the same error as last time. That’s disturbing. It seems to be a problem with authentication on the SourceForge server. I’ll fix that later.