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 document today.

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 Everything::Node::Test::setting appropriately.

After creating the test subclass and overriding node_class() appropriately, 212 tests run and 8 fail. (There are plenty of skips though.)

Overriding test_extends() adds another test. Great!

After adding SUPER() to the Everything::Node::setting namespace, 213 tests run and 5 fail. Even better.

The next two tests are for construct() and destruct(), but they do the same thing as the parent class’s test methods, so I don’t even need them.

The getVars() test is a bit tricky, but it ports easily. 215 tests run. 5 fail. The same goes for setVars() and 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 fieldToXML().

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.

Hopefully the 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 test failures.

The problem here, as usual, is that there’s an extra method call in the overridden xmlTag() that the mock in the parent test method doesn’t understand. The solution for now is to check the SUPER() 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 updateFromImport(). 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 fail?

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.