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 start porting the nodegroup tests. This will take a couple of days. It’s painful, but it’s good for me. That’s what I keep telling myself.

Day 16: Nodegroup is Complex

Today is the first day of the second half of this project. I didn’t anticipate spending so much time reworking the node tests, but it’s the biggest need I saw as I went along, I’ve fixed some bugs, and I’ve learned a fe things. These changes will also make future changes easier — and those future changes will reduce a lot more technical debt.

As you might imagine, today’s task is to port yet another node’s tests to the new Test::Class style. I’m starting to reach the point where all of these will be small, but the next node on the list is nodegroup, the second-largest node class.

I’ll have to do it eventually, so why not now? As usual, the first step is to make a new test class for it.

I’m getting sick of overloading node_class() every time, especially because the test class names correspond to the node class names. Here’s a new version of the method in the node test class:

  sub node_class
  {
      my $self =  shift;
      my $name =  blessed( $self );
      $name    =~ s/Test:://;
      return $name;
  }

That ought to work for all test classes. It’s time to remove the overloaded methods to see. If everything still passes, it works correctly. It does. Here’s checkin #852. That’s one fewer worry when porting yet another test. That feels good.

How does the nodegroup test fare? 212 tests run (as I expect) and 48 fail. There are plenty of skips. That’s fine.

The first method to test is construct(). It’s short and easy to port. Great! 212 tests run and 47 fail.

The next method is selectGroupArray(). The tests seem to port well, after untangling some mock database and node confusion, but they die with a method call on an unblessed reference. This usually means that the order of values returned from a mock call is wrong. I can’t immediately see where that is though — wait, there it is. The mock database call to return the cursor returns the node, not the mock database object (the latter of which mocks the fetchrow() method). That should fix it. Now 220 tests run and 47 fail. I’ve added eight passing tests, just as I intended. (That’s 9% of the test file ported. Ugh.)

The next method to test is destruct(). This is simple enough it can call the parent test method. 222 tests run and 47 fail.

insert() comes next. This is interesting; the code here checks for create access, but calls SUPER() which also checks for create access. That’s deletable code. The rest of the test method can be very short; it just needs to check that the inserted node keeps its group parameter and that the node calls updateGroup(). With that done, 226 tests run and 44 fail. I wonder if there are other related failures though — there are. The child test doesn’t mock updateGroup(), as I expected.

That didn’t help; it just moved the death in tests elsewhere. I wonder if it’s because the $USER parameter is so simple and wrong? No, it’s just as bad in the node test.

Reordering the SUPER() call in the test fixes everything. 226 tests run and 43 fail. There aren’t as many skips either.

update() looks almost as simple. With a quick port, 229 tests run and 44 fail. Oops. Unlogging the SUPER() mock fixes it; now only 42 tests fail.

A lot of the remaining failures are in updateFromImport(), the next method to test. I forgot to mock updateGroup() first, but with that done 229 tests run and 41 fail. Can this call SUPER()? Yes, with the proper mocking and unmocking. I also fixed a test description typo in the node test class.

233 tests run. 41 fail. Several skip. I’ve ported 21% of the test file.

I hate the updateGroup() method, as it’s huge. It’s also next on my list. No core node overrides it, so there’s not a lot of sense in splitting it out into separate methods unless they make the code more readable. I think they might. The first method is test_update_group_access(), with two tests. They both pass. Great!

There are 18 tests in the test_update_group() method (I didn’t find any other good place to split it) and plenty of mixed mock and mock database code. How’s the first run? A compilation error and a syntax error. That’s fine. With that fixed, 253 tests run and 46 fail. That’s not too bad – 18 more tests and only three failures. I bet some of them are counting errors too.

The fixes look like tweaks to the expected input and output. Making one test more accurate about what it expects fixes one. Further unconfusing the mock and the mock database fixes another. Yet one more is because a SQL statement regex is too strict.

There was a larger problem in the tests too, namely that I moved some initialization code to come after code that relied on it. Oops. After fixing that, everything passes.

I really don’t trust the coverage on this code (and I extremely distrust the code being tested; it’s too long and complex), but it’ll do for now until there are better tests. 254 tests run. 41 fail. I have 31% of the old test in the new form. That’s slow, but it’s the end of my day.

After running the whole test suite, the normal, unported nodegroup tests failed. That’s because I made some code changes. I decided not to delete these tests until I ported the entire test file, so as to keep my changes minimal. I don’t want to keep the bugfixes out of the code though, so I took a few minutes to fix up the old tests, just to keep everything passing.

Everything looks good. This is checkin #853 and the end of another day.