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 finishing the port of the parent node to Test::Class. Hooray!

Day 10: The Final Porting of Everything::Node::node’s tests

At last count, two-thirds of Everything::Node::Test::node was in the new test format and 136 tests ran and passed. The next test to handle is test_apply_xml_fix(). It looks like there’s one branch in the method, so there will probably be two test methods.

I started by making one test method. If I can get it to run (by the mechanical transformation now) and then pass, it might be more obvious where to split it. Before I can split it, I also have to have a name for both test methods. (If I can’t really name them, there’s probably no point to splitting them.)

After the transformation, six of the eleven tests fail. That’s okay. It looks like I mishandled the error-checking code as usual, in three places. It’s possible that I should add a nice set of methods to handle this, instead of accessing a nested data structure. Of course, that probably argues for a better (and mockable) logging system anyway, which is already a goal. I’ll leave it for now.

The remaining errors are due to the previous test’s reuse of the mock object for the node and the database. After fixing that, everything passes. Now it’s time to consider the split.

The first branch condition handles an error case, where there’s no node by which to fix up this node. That seems like a separate enough case that it deserves its own method, test_apply_xml_fix_no_fixby_node(). After copying and pasting and refactoring just a bit, both methods pass.

I noticed also that another test threw a warnings and fixed that by passing slightly more data in the mocked node. (Then I saw that an error-checking regular expression was wrong, so I fixed that too.)

That went quickly. Now I have 71% of the test file in the new form.

Next is getRevision(). It looks like there are nine tests here. The transformation went quickly. What breaks when I run it? Only two tests. Things are definitely getting better! Of course, I did fix the database/node confusion when I did the transformation… except in one spot, where I didn’t update the database object. That fixes one test.

The other test failure was from missing a method call in the transformation. Oops. Fixing it doesn’t make the test pass, but it’s closer. The problem is that I assumed that xml2node() was a method, not a function from Everything::XML, and I moved it to a mock instead. That’s okay.

After adding back code to insert the function temporarily into Everything::Node::node (while retaining the mock) and fixing yet another database/node confusion, all of the tests pass. Great.

I think this is a candidate for splitting too — there’s a non-workspace version as well as the workspace version. Of course, this code will just move anyway, so there’s not a lot of point in changing it yet. 156 tests pass and 76% of the code is in the new form. I’m doing pretty well today.

The next test method is test_log_revision(). There’s a fair amount of code here too. I can already see one variant for test_revision_access(). That’s a short test method (and it allows me to delete the tests for hasAccess().

Again, the mechanical translation goes by pretty quickly. Three tests fail and there are “unmocked method” warnings. That’s a good indicator of why the tests failed. They’re sqlDelete() and sqlInsert() methods on the database mock object, so calling set_true() for them out to fix things… or at least unbreak them somewhat.

The tests didn’t pass after that, but that’s fine. They’re closer to passing. It looks like part of the problem is the method call order checking in the test. Using separate mock objects changes the methods being logged, so lots of calls to next_call() might have to change too. (They tend to make test code fragile anyway, but as I keep saying, testing against a real database should fix this.) Only two tests fail now.

Another of the failing tests fails because the node doesn’t have a node_id. Fixed!

The remaining test fails because the code tries to call toXML() on an undefined value. I think it’s getting nothing back from getNode(). That’s because I didn’t add a canWorkspace() mock on the node. Fixed. Now 170 tests pass, and 82% of the test file is in the new form. I’m getting close!

The next method is undo() and it’s a bit of a monster. There are quite a few database calls in it, so I’ll be extra careful. Again, there’s an access test, so I need an access-testing method. It passes. Great!

Of the 17 tests in the other method, 12 fail. That’s not progress, but I can fix them. Ahh, there’s the old database/node confusion again. Oh well. Now there’s an unmocked method, getType(). I remember this from a while back, when it’s not actually the problem — it’s the AUTOLOAD() in Everything::Node catching something inappropriately.

Disabling inheritance temporarily (don’t ask) shows that the real unmocked method is incrementGlobalVersion() on the nodecache. That’s fixable. Now there are two unmocked methods, now() and sqlSelect().

Wait… the problem here is that this is somehow calling the update() method, which I don’t want to happen… so I’ll just mock that instead. Okay, now ten more tests run. (Why? The tested code threw an exception. Test::Class caught it, but couldn’t run any more tests and I didn’t update the test number appropriately.)

I’ll update the test count for this method and then see what happens. Ten tests fail. At least everything is running though.

The first problem is yet another confusion. I should make a shorthand abbreviation for that: ARGH. Now nine tests fail. ARGH. Now eight tests fail.

Another common problem is juggling the logging of mocked methods. After fixing a problem like that, six tests fail. That’s definite progress.

Adding a clear() call to the mock database before another trip through the tested method fixes four tests, so only two fail — at least, that’s what I thought until I saw another death. Oops. After fixing that, the failure count is back up to four. That’s still less than six, and there’s another unmocked method to mock. Fine.

Four tests still fail, but there are no unmocked method warnings. That’s a shame, as there’s an unmocked toXML() call that makes another test pass.

I think updating method call counts will fix the remaining issues. It did. Now 198 tests pass and 93% of the file is in the new form. Wow. It’s slightly over my hour, but it’s very much worth finishing this today — I’ve spent a lot of time working on it. What’s left?

The next test method is test_can_workspace(). It’s very short. After a short transformation, four more tests pass. Great!

getWorkspaced() is next. The actual code is fairly short, as far as workspace code goes. This should also be quick.

There are six tests in the method, and after the transformation, only one fails. Being more explicit about which calls to log and check fixes it. Nice. Now 208 tests pass, with 97% of the file converted. There’s one method remaining, updateWorkspaced().

Both the test code and the code are short, so this also should be quick.

Oddly, there’s already a test_update_workspaced() method in the test file. I’ll rename this one for now and then combine them in a bit.

After a couple of ARGH moments, all of the tests pass. Now to combine them…. Actually, the existing method does nothing new, and the new method is more comprehensive, so I deleted the old one and renamed the new one and now 213 tests pass and I’ve finished migrating the whole file.

Now I can keep the changes to t/Node/node.t so that everything runs through Test::Class and start migrating other nodes on my next day. Wonderful!

All tests pass, so this is checkin #842 and the end of day 10.