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 the porting of yet another node test worked almost flawlessly. Am I getting good at this?
Day 14: Finally Porting without Debugging
The problem in the previous day’s work was because I started porting tests
out of order. To fix that, I need to port
setting and probably
Somehow I removed all of the tests in t/Node/setting.t when I
changed the test harness there, but that’s why there’s source control. After
recovering the previous version, now I can revise
After creating the test subclass and overriding
appropriately, 212 tests run and 8 fail. (There are plenty of skips
test_extends() adds another test. Great!
SUPER() to the
Everything::Node::setting namespace, 213 tests run and 5 fail.
The next two tests are for
destruct(), but they do the same thing as the parent class’s test
methods, so I don’t even need them.
getVars() test is a bit tricky, but it ports easily. 215
tests run. 5 fail. The same goes for
hasVars(). The latter test makes one failure go away, so now 217
run and 4 fail.
fieldToXML() is next. The parent test class exercises this (and
all of its tests pass), but there’s more to test here now. Specifically, the
setting node has to export the
vars field to XML, and
that logic is in
By adding a
SUPER() call in the test method, everything just
works out. 222 tests run and 4 fail. This covers 46% of the test file.
xmlTag() tests are similar. The same technique
makes 228 tests run and still only 4 fail. I didn’t even look at the code to
see if there’s duplication… and there is. Also, this is the source of three
The problem here, as usual, is that there’s an extra method call in the
xmlTag() that the mock in the parent test method
doesn’t understand. The solution for now is to check the
call, but not actually to make it. With that in place, 219 tests run and only 1
fails. 61% of the test file is in the new style.
The next test method is the hideous
applyXMLFix(). It’s a big
chunk of code.The right way to figure out what this should test is to examine
what the overridden method does.
The first batch of tests can extend
test_apply_xml_fix_no_fixby_node(). That simplifies things. Of
course, the tests were buggy because
applyXMLFix() didn’t return
undef… but that’s an easy fix and I’m glad I remembered it.
Another test failed because the old
setting test overrode the
logging on its own. Porting that to the new approach fixed the test. 224 run
and 1 fails.
The rest of
applyXMLFix() will hopefully be easier. Nope. 230
run (it runs six more) and five fail.
Of course, this is because the fixby node doesn’t contain the right fields; I moved that setup into the previous test. With the setup code added back, two tests fail.
A lot of tests skip too, because I didn’t add code to call the parent test method. With that in place, and with a couple of database mocks replaced with the database mock object, 230 tests run and 2 fail. One of the failures is in the parent method. Calling the parent test method as the first action — and clearing out the logged error data — fixes that failure. 230 tests run. 1 fails. 94% of the code is in the new style.
getNodeKeepKeys() is next, with one test. 231 run. 1 fails.
The final test in the file is for
Coincidentally, this is the one with the test failure. Adding a logging mock
fixes it, but that’s without calling the parent test method. Will it pass? Will
It fails, as I expected. The problem is that there’s an undefined reference somewhere in the parent test method. There’s really no easy way to test this, so I decided to leave the test overriding but not calling the parent method. 230 tests run. Everything passes.
How’s the whole suite? 2232 tests pass in 54 files and there are no failures. That’s good for a day of work. After setting the properties in the new test file and adding it to MANIFEST, this is checkin #847.