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
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
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
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
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
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
methods to adapt.
That’s easy, but there are some failures — because
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
With 5024 tests now, there is only one skip (in
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.
workspace node inherits from the
so there may be complications here. There’s only one method though:
The initial porting is quick and the one skip in the entire test suite
magicallyh goes away. (It was in the pre-
dbtables() test. Inheriting this from the
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.
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?
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
With the quick porting, 333 tests run and 11 fail. It’s override time!
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.