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
Day 10: The Final Porting of
At last count, two-thirds of
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,
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.
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
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
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
methods on the database mock object, so calling
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
The remaining test fails because the code tries to call
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
Everything::Node catching something
Disabling inheritance temporarily (don’t ask) shows that the real unmocked
incrementGlobalVersion() on the nodecache. That’s
fixable. Now there are two unmocked methods,
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
it, but couldn’t run any more tests and I didn’t update the test number
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.
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
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
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
Test::Class and start migrating other nodes on my next
All tests pass, so this is checkin #842 and the end of day 10.