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 starting to port a child node to Test::Class. Hooray!

Day 11: The Second Great Node Test Port

Having finished the grand migration of the node tests to Test::Class, today’s work is to port the nodetype node tests. Yes, I should port the nodegroup tests, but that’s a big file and it intimidates me today.

I anticipate doing a bit of research today, mostly to figure out how Test::Class handles inherited tests. Many of the nodes override their parent methods, then call them again with SUPER(). I hate to duplicate the tests, but I don’t know exactly how Adrian’s distribution will handle that call.

Skimming the documentation, it looks like calling the parent test method within the test class will work. My only concern was how that affects the test count, but prefixing the + character to the test count in the :Test() attribute should work. Great!

I started by copying the nodetype test to lib/Everything/Node/Test/nodetype.pm and adding a short header:

  package Everything::Node::Test::nodetype;

  use strict;
  use warnings;

  use base 'Everything::Node::Test::node';

  sub node_class { 'Everything::Node::nodetype' }

That should be enough to run the basic tests. The good news is that the test file ran. The bad news is that 28 out of 213 tests failed. That’s fine, though – this node behaves differently.

I need a new inheritance test, so should I override startup() and add a method? How does that work?

  sub startup :Test( +1 )
  {
      my $self = shift;
      $self->SUPER::startup();

      isa_ok( $self->node_class()->new(), 'Everything::Node::node'; );
  }

I’m not entirely happy with that, but it’s not awful. I mistrust the constructor somewhat, but it works for now so I can live with it.

It seems like the rest of the problem is to go through the nodetype code, method by method, and override what I need to override. Hopefully I don’t have to modify too many node tests to make this work. There are only 13 methods in this class.

Unfortunately, one of the first batches of failures is in startup(), where node loads several modules and nodetype doesn’t. I could split this into a test_imports() method, but now I’m not sure of the value of testing those imports. If I need to, I’ll do it later.

(I think my original idea was that I wanted to catch missing imports by hardcoding the use of the exporting modules in the tests, though now I realize that that makes the code much more brittle to test and thus to code. Delete!)

I might have to mock the Everything::Security model in the test, but I’ll figure that out later. Only 28 tests fail now.

The first test is for the dbtables() method. This is mostly a data method, so it’s difficult to factor properly between the tests and the code… but for now I can override the test method altogether. 27 tests fail.

The next method is construct(). I realized that I wasn’t looking at all of the commented-out procedural test code. Perhaps I shouldn’t be programming this afternoon. Oh well; I have version control.

I do need to flip back and forth between the node tests and the old test code to see what I can keep and what I can ignore. test_construct() there merely returns true. I can override this test method completely.

Moving the procedural code into a method gives me a 92-line method. Yuck. That’s okay though. With a few short transformations (mindful of the mock database/mock node distinction), I ran the test to discover… I forgot to update the test count. Then 27 tests failed out of 233 and test_construct() died somewhere in the middle. I probably missed a mock somewhere.

Apparently so — there’s another getType() error. That’s fine. I missed at least one node/mock db swap. Two. Now only 26 tests fail and 45% of the code is in the new style. That’s progress!

The test code is ugly, but it works. The next method to test is destruct(). It took 30 seconds to convert and passes. 25 tests fail and 48% of the code has migrated.

insert() is next. It has a SUPER() call, so test_insert() could be slightly more difficult. (If I just mocked SUPER(), it won’t be. Hey, I did! Great!)

The test method took only a couple of minutes to migrate and I forgot to change the test count again. That’s fine. I did have a couple of typos to fix though. After fixing the count, I noticed that the test died, probably due to yet another unmocked method.

Yes; the test didn’t follow the API of insert() properly; it didn’t pass in a $USER parameter. That’s no good.

There’s another problem; SUPER() doesn’t work within the nodes. That’s pretty bad. I don’t really miss it, but I’m not quite sure I should get rid of it. On the other hand, I really don’t want to mock up what it would take to make it work, so it’s back to the SUPER module.

Adding that to Everything::Node made three more tests pass.

Now test_insert_restrictions() and test_insert_access() fail. Why? Probably because I am not skipping the nodetype extension check, of course! Adding some code to set the nodetype properly makes test_insert_access() pass, but test_insert_restrict_dupes() still dies… because I overrode test_insert_restrictions() instead. With all three overridden appropriately, 18 tests fail out of 237 and 62% of the tests are in the new style.

The next method to test is update() and hopefully it will go more quickly. I definitely learned something though.

test_update() works just fine, with a quick fix. test_update_access() is a little trickier; it also needs a stub. Oh, and the behavior of SUPER() has changed; it no longer sends along arguments. That’s fixable. With that fixed, I don’t even need the stub. Great!

That leaves eight methods untested, 240 tests running, and 17 failing. I want to get all of them passing again, but I doubt I have time today. I’ve migrated 2/3 of the nodetype tests to the new style and have learned a lot from the experience. That’s a valuable day. Now hopefully all of the system’s tests pass.

They don’t, because of the changes to SUPER(). Oops. I should perhaps localize that to Everything::Node::nodetype for now, and within the test class. That’s kind of icky, but it works. All of the tests pass (and this new test isn’t any worse than it was), so this is checkin #843 and the end of another day.