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 to port the remaining node tests as quickly as possible. Then it’s on to greater things!

Day 21: How Quickly Can I Port the Real Remaining Tests?

Day 20 ended very successfully with me porting three very small tests very quickly. (I also fixed the bug in SUPER and checked in a fix to the code there as #861. Yes, that’s cheating, but I shouldn’t have had that bug in the module, so it totally counts.)

The first test to port is for nodemethod. It’s pretty small and it extends node. This should go quickly.

With the bare minimum porting (and, as usual, a custom test_dbtables(), 212 tests run. One fails.

The failure is in the test for getIdentifyingFields(), the only other method in nodemethod. Overriding that test (not inheriting it!) fixes the failure.

That’s it. That’s all this test needs. I can delete the rest of the test code. Hopefully the remaining tests will be this easy (or easier).

Hm, there’s a problem with the user test. Weird. It looks like a SUPER() problem. Yucky. After a few moments of debugging, it turns out that there’s yet another bug in SUPER, due to some old, undeleted code. Deleting that fixes everything.

4816 tests run, with one skipped (nodemethod had six of the seven skips) and everything else passing, so this is checkin #862.

The next port target is htmlpage. It also inherits from node.

With the initial porting done and test_dbtables() fixed, 212 tests run and 4 fail. There are also a lot of skips. This is because htmlpage overrides insert().

The initial insert() test is pretty easy; there are no new method calls for new behavior — all of the new behavior in the overridden implementation depends on the presence of a node attribute. That’s easy to fix.

There’s also a SUPER() call that needs to pass data, but it’s easy to test that and fix the failing test.

Now how does inheritance work? With the attribute, calling the parent test method works just fine. That leaves the other test_insert_*() methods to adapt.

That’s easy, but there are some failures — because insert() clears out node attributes, including the database. After adding that back in, 217 tests run and all of them pass with no skips. It’s time to run the suite again.

With 5024 tests now, there is only one skip (in workspace). Everything else passes. This is checkin #863.

I still have more time today. The next test to port is for workspace, coincidentally. (I thought it might be soon.) I hate the workspace code, as it’s a mess. At least the test is short.

The workspace node inherits from the setting node, so there may be complications here. There’s only one method though: nuke().

The initial porting is quick and the one skip in the entire test suite magicallyh goes away. (It was in the pre-Test::Class dbtables() test. Inheriting this from the setting test class means I can delete code!)

230 tests run and 23 fail. That’s a lot, for such a small class. What’s the problem?

nuke() is very different here. I don’t think it’s inheritable. That’s fine.

The new test_nuke() has seven tests (including a new SUPER()-takes-arguments test). 210 tests run and 1 fails.

There’s duplicate code in nuke() itself; why does it need to check for delete access? If the parent nuke() fails, so will this one. I rearranged the logic in the node class and that made everything easier to test. Now everything passes.

It’s time to check the entire suite again. 5225 tests run, with no skips and all passes. Here’s checkin #864.

The last set of tests to port today is for image. (I have seven remaining, including this one. That might take up all of day 22.)

The only code to test here is a dbtables() method. It ports immediately. I want to change the SUPER::dbtables() call in the class though. The tests still pass.

Now the entire suite has 5433 tests, which all pass. This is checkin #865.

I still have a few minutes. What’s next? usergroup, which extends nodegroup and might have a bit of work there. There are only two methods though, and they both return false. (That’s it. That’s all they do.)

With the quick porting, 333 tests run and 11 fail. It’s override time!

Overriding test_conflicts_with() (which runs one test, yawn!) fixes five of the failures, running only 328 tests. Overriding test_update_from_import() fixes the remaining failures. 321 run and all pass.

Running the whole suite reveals that now 5748 tests run. All of them pass. This is checkin #866.

The entire test suite takes 45 seconds to run on my laptop. That’s not awful, but it could be faster. I like the increased coverage (especially as it has helped me to remove some code), but there’s a point of diminishing returns.

I think that moving the database access code elsewhere will speed up the suite and reduce the number of tests. Running tests against a testing database will also reduce the number of tests and add simplicity too.

I’ll figure that out when I need to, next time.