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 nodegroup?

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 $field parameter is group.

With a quick port run, 293 tests run and 15 fail. One of those is due to the change in 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 nodegroup’s implementation 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 SUPER() 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-Test::MockObject 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.

The getNodeKeepKeys() test method ports almost instantly and inheritance works flawlessly. 331 tests run and 1 fails.

The final method to port is for conflictsWith(). Fortunately, 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 Test::Class object 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.