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 finish porting the
nodegroup tests. I finally made a decision about how to handle parent test methods.
Day 18: Can I Finish
This is the third day of porting the
nodegroup tests. Can I
finish them? I hope so. I’ve certainly earned enough experience that porting
the simple tests goes quickly and debugging the complex tests is easier.
Unfortunately, I just hit the XML tests.
Where did I leave off? 285 tests run, 17 fail, plenty skip, and 65% of the test is in the new form.
The first test to port is
test_field_to_XML(). I think I can
inherit this one, as the only difference is when the
With a quick port run, 293 tests run and 15 fail. One of those is due to the
SUPER() handling. After running the parent test method
first, 14 tests fail. Great!
While checking these tests, I noticed that the parent test for
destruct() failed because
does not return true. Fixed, so 13 tests fail.
xmlTag() is the next method to test. It looks like inheriting
will also work here (and revising the flow of control through the method itself
will make it clearer). After another quick port, 296 tests run and 13 fail. One
of those failures is another
SUPER() API change. Fixing that makes
another test pass. Adding test inheritance runs 305 tests, throws some
warnings, and makes the total number of failing tests go up to 15.
Why? Oh, there’s a method call before the
call. Mocking that should fix it… but that’s not the problem. The problem is
that a mock in the parent tries to handle multiple calls to the method with a
set_series() mock. Calling the mocked method more times than that
test anticipates causes a problem.
That’s easy to fix; change that method to
set_always() and call
it just before each trip through
xmlTag(). That makes 305 tests
run and only 12 fail.
With 80% of the test file ported, the next method is
applyXMLFix(). I can probably inherit the test for no fixby node
directly, so that might make code to delete. I can also probably inherit the
parent test with ease.
After the quick porting, 315 tests run and 16 fail. Where did I get four
more failures? One of them is from a
SUPER() call. After fixing
that, 315 tests run and only 5 fail. Wow.
Those errors seem to come from a series of calls on the database object,
which I haven’t mocked yet. Is this an easy fix? 315 tests run and 3 fail. One
failure is from this method. It’s because the mocked node doesn’t have a
group attribute. Adding that makes 315 tests run and 2 pass.
Can I inherit the parent test? I think so. After clearing the errors in one spot in the child method, 315 tests run and 2 pass. Why did 315 run before I called the parent implementation? I misspelled the test name in the child method. After fixing that, everything is good again.
The next method to test is
clone(). I don’t think inheriting
this will work, but I’m willing to try.
I found a bug in the test, where one test would always succeed. I fixed it.
After that and a few other porting cleanups, 320 tests run and 1 fail. The failure is elsewhere. Just for laughs, will test inheritance work here? Not easily. I’ll skip it for now. (It’ll be easier to make work when testing against a database.)
With 91% of the tests ported, the next method is
restrict_type(). This method uses the really old pre-
approach to mocking. Yuck. Wow, the code itself uses the non-OO database
accessors. Double-yuck. No wonder the test code sucks.
Why not clean them both at the same time?
After a few moments of work, 327 tests run and 1 fails. The code is also cleaner in the method and somewhat cleaner in the test method.
getNodeKeepKeys() test method ports almost instantly and
inheritance works flawlessly. 331 tests run and 1 fails.
The final method to port is for
that’s where the one remaining failure is. With the test ported and a
SUPER() case fixed, 333 tests run and one fails. Ugh, I’ll have
to count to figure out why.
Oh, the problem is that I made a typo and passed an extra first argument to the method. Now 333 tests run, all pass, and this entire test file is in the new form. Great!
Inheriting this test doesn’t work very well, as the parent test method
passes a newnode of its own, one lacking an attribute that
nodegroup checks. I could fix that by storing session
data in the
and using that to share information between tests, but that’s more work than I
want to do right now. I’m not sure it’s quite right either.
How do the rest of the tests work? Just fine. 2878 tests run, with only a handful of expected skips. This is checkin #855, and the end of another day.