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 finish porting all of the node tests and to clean up the mess!

Day 22: The Last Node Test Porting, Ever

Is it possible to port the remaining tests today? I hope so!

The first is for nodeletgroup. It only needs to override test_extends() (and can inherit it). With that in place, 334 tests run and all of them pass. I’m sure the suite all passes too, but it’s always good to check.

While they’re running, I noticed that I had forgotten to check in an updated Build.PL with a dependency on a newer version of SUPER. That’s commit #867. 6078 tests run now and all of them pass. The new test is commit #868.

The next node class to test is htmlsnippet. It extends htmlcode, so it can probably just inherit the tests directly. I made that happen and 210 tests run with no failures. Looking at the diff, this actually removes code. The test suite grows, but there’s less code in the repository.

Everything all passes again (6284 tests), so this is checkin #869.

The next node class to port is permission. This is a lot of boilerplate, but I’m so close to the end that it’s really not worth automating it. (I normally would, but here’s a free insight for the day: if it takes five minutes to automate and five minutes to do and you’ll never do it again, just do it.)

permission extends htmlcode. Again, 210 tests run and all of them pass. I expect no less. I run the tests though and the entire suite just to make sure. (While the entire suite runs, I prepare the checkin.) Again, this checkin actually removes more code than it adds. Now 6490 tests run and pass at #870.

(Some of you might ask why I checkin these so frequently now when I checked in much larger chunks earlier. Checkins are basically free with Subversion, so the smaller the unit of work, the easier it is to see what changed and when. Also, the smaller the change, the less work it is to revert if I ever get into a spot where I can’t make the tests pass.)

There are two node classes left for which to port tests. First is theme. It extends nodeball, so there should be a test_extends() override. (I wonder if I forgot those for the other tests… I’ll check after this.)

Oddly, the theme tests get in an infinite loop in one of the hateful XML methods in nodeball. I wonder if importing SUPER() will help. Explicitly using Test::More in the test code doesn’t. Explicitly using SUPER doesn’t either. There’s deep recursion in applyXMLFix() in the test_apply_xml_fix_no_fixby() method in the node tests. Ugh.

This is the source of the uninitialized value warning in the nodeball tests. Why? I wonder if fixing that would fix the test. I fixed the warning with an exists test in nodeball’s applyXMLFix() (checkin #871).

The infinite loop is yet another SUPER bug, I bet. Ugh.

Actually, it isn’t. It’s because of the evil, icky way that nodeball calls applyXMLFix() as a setting function. There’s no good way to fix that without making nodeball contain a setting node, and that’s a bigger refactoring than I can do now.

For now, I’ll just stub out test_insert_access(), test_insert_access_restrictions(), test_insert_restrict_dupes(), and test_apply_xml_fix_no_fixby_node() in the theme tests. I don’t like it, but it’s progress and it’s something I know I have to fix later.

This is a good place to use TODO tests, actually. Now 290 tests run and the four TODO tests fail, as expected, so everything passes. (Expected, temporary failures are okay. Marking them as TODO tells the test suite to keep track of them but not count them against you.)

How’s the full suite? 6776 tests now run, with no unexpected failures. (The harness reports everything as passing, which is nice.) This is checkin #872.

Finally, it’s time for the opcode tests.

They port quickly, as you might expect. 211 tests run and all pass (after I remembered to use Test::More in the new file). The entire suite now has 6983 tests. Everything passes. Here’s checkin #873.

My last task today is to check that every node that needs it has a test_extends() method.

Hm, I don’t see tests for container. Weird. I’ll add that too, while I’m at it. 212 tests run and all of them pass. (I had to port the test_dbtables() code.) This is commit #874.

What’s missing inheritance tests? htmlsnippet.

Argh, javascript is missing a test too. With test_dbtables() added, 212 tests run. Now the entire suite has 7400 tests. This is commit #875.

mail also needs test_extends(). Now 213 tests run and pass there. I missed permission earlier. 211 tests run and pass there. restricted_superdoc gets the method. 213 tests run and pass there. superdoc is the same. usergroup needs the method, and now runs and passes 322 tests. workspace now runs and passes 211 tests.

I missed symlink last time, for some reason. It runs and passes 212 tests, after porting. Now 7615 tests run in the entire system. I checked in the symlink tests as #876 and the inheritance tests as #877.

That’s it for today. What should I do next time? It’s probably time to untangle the database code.