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 another node tests to the new system. Unfortunately, the order of porting matters quite a bit… problems abound!!
Day 13: Why Going in Descending Size Order Was a Mistake
As promised, I thought about how to solve my
I’ve decided that the fragility of testing reveals an inherent problem in the
design of the code. Of course, I already knew that it wasn’t perfect — that’s
why I’m refactoring it.
I could parametrize the tests somewhat. That is, I could add extra data methods to each test class so that I can override a few small methods and expect the parent test methods to pull in that data and test the right things there. In some cases, that might mean I duplicate those data methods between the tests and the code being tested.
I think the wisest course is not to make any big changes to the code until I get better test coverage, and then plan to refactor the tests yet again when I refactor the code. I haven’t seen much discussion about refactoring tests, but it seems wildly obvious that it’s worthwhile in some cases.
Today’s first task is to refactor the
user test. It’s short, so
this should be fast. There are only ten methods in 200 lines of code and
comments. I like that ratio.
First I change t/Node/user.t to run the test class methods. What
fails? Nothing. I forgot to override
node_class(). Now 212 tests
run, 18 fail, and plenty get skipped. Great!
The first test to override is for
dbtables(). The first two
tables should be
document. Of course, the
test failed because of the incorrect use of
That reminded me to check the inheritance, so I extracted
test_extends() from the startup tests in the
test. The new test method only checks the inheritance of the class. It’s a
short test method, but it’s overridable.
Going back to the
SUPER() failure, I’ll add the hack I used in
nodetype test to inject
SUPER::SUPER() into the
user node class. I don’t like it for now, but it works without
breaking anything else… yet.
The next method to test is
insert(). It’s short and easy, so it
should be easy to port the tests. After realizing that some of the failures are
because I forgot to use
Test::More in the
test header, 208 run and 17 fail. It’s on to the next method.
isGod() checks the access of the user. It’s another short
method, with several control points. This adds three tests, so 211 run and 17
fail. I deleted a couple of tests that checked the call order of traced, mocked
calls. If the mocks didn’t work, the following tests wouldn’t pass. I don’t
worry so much.
The next method is
isGuest(), a similar check for no access
powers for the user. I also deleted a couple of mock trace checking tests.
With four new tests, 215 pass and 17 still fail. That’s progress. Oh, 56% of
the test file is now in the new form. This is going fairly quickly, though I
worry about some of the inherited tests. Time will tell.
getNodeKeys() method helps the system to know which nodes
to export. It makes sure not to export user passwords and the time of
the user’s most recent activity in the system. After porting the tests, 220
pass and 17 still fail.
Now I wonder if doing a quick
s/:Test/#&/ in the
node test will make it easier to debug these failures…. I’ll try
that when I get this ported.
I’ve ported 67% of the test to the new style. The next method is
verifyFieldUpdate(), a short method with lots of tests for some
reason. Oh, I changed my style when writing these tests. There aren’t that
After porting this test, 222 run and only 16 fail. I made one pass somehow!
updateFromImport() both return
false; they’re easy to test. 216 tests run (there’s no reason to inherit the
updateFromImport() test method) and 13 fail. I had a feeling this
restrictTitle() may be another of those magic methods. It
SUPER() and it does some work. After migrating it,
213 tests run and 12 fail. It’s not quite comprehensive, but it’s not
bad. I’ve also made it to 81% of the file.
Of course, there’s only one method left, and that won’t fix all of the
failures. Hmm. I’ll fix the test for
getNodelets() and then figure
out the other problems. I think they’re still
getNodelets() returns the nodelets (little boxes of cut-down
node views) that the user has selected. It’s a method original to this class,
so it won’t fix any inheritance problems.
test_get_nodelets() (and fixing a typo where
$sef is not
$self), 216 tests run and 12
fail. Why do those fail?
The diagnostic messages are the well-loved “Un-mocked method ‘getType()’”
Everything::Node, which makes me think there’s a
SUPER() problem somewhere.
It looks like the real problem is that
SUPER do not work well together when the extended object
inherits from something else. I can fix this, because I maintain both
After fixing that, I noticed two problems with the
getNodeKeys() test. First, the
SUPER() call in the
module didn’t pass the parameters. Fixable! Second, the mock in the
user test mocked the wrong method. Also fixed. 216 tests run and
The next failure was in
hasVars(). I think this is a failure
because I haven’t ported
setting to the new tests yet, and that
definitely has variables. I can fix that with a short, deletable test in
user for now. 216 tests pass and only 9 fail.
test_insert_access() is the next failure. This is because
node returned false, not 0 on failure.
Now 216 tests run and only 6 fail. There’s a method calling failure here in
node because the inherited test doesn’t do anything to mock the
update() call. (When a user gets created,
performs the insert, then sets the
author_id field of the user to
itself, so that the user owns himself. Then it calls
The solution to this is to override
test_insert_restrictions() to mock
all. 216 tests run and only 4 fail.
The next failure is in
test_restrict_title(). The problem here
is that I mis-ported the tests. Oops. 216 run and 3 fail.
The only remaining failures are in
xmlTag(), of course. This
is because I haven’t ported the
setting tests to the new form, so
I’ll mock up a test method for now and remember to remove it later. Yes, it’s
cheating, but it was my mistake to go in the wrong order. Besides,
user will just inherit the tests anyway.
216 tests run.
magically skips the 9
xmlTag() tests. Everything passes.
Does the rest of the code? Yes. 2317 tests now run. Great! I’ve also updated
Build.PL to require new versions of
SUPER, so I need to
upload those soon.
This is checkin #846 and the end of a day.