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.

Really Refactoring the Node Tests

I really dislike the way node attribute access all goes through hash members, not accessors; it’s difficult to maintain. I’d love to fix that soon. Of course, that’ll break a lot of ecore, but that’s fine — I can fix that too.

It seems like the easiest way to fix all of this is to migrate all of the node tests to Test::Class. Now that their inheritance is correct, sharing behavior in the classes should be much easier.

I haven’t done this much, so it’ll be a good opportunity to learn for me too.

The biggest test is the one for Everything::Node::node, so that’s probably the place to start. I thought to start by copying its test file to Everything::Node::nodeTest, but I hate the name. Everything::Node::node::Test means creating even more subdirectories, so how about Everything::Node::Test::node? Besides, this is Subversion, so I can move anything.

  $ mkdir lib/Everything/Node/Test/
  $ cp t/Node/node.t lib/Everything/Node/Test/node.pm

Perhaps I’ll put everything under Everything::Test someday.

Moving this all doesn’t work right off the bat; t/Node/node.t needs to load the test class and run the tests. It’s a small file now:

  #!/usr/bin/perl

  use lib 't/lib';
  use TieOut;

  use Everything::Node::Test::node;
  Test::Class->runtests();

I can probably get it shorter, but I’ll wait to see what happens. Making the test module run means wrapping just about everything in:

  sub test_all :Test( 228 )
  {
      # ... 
  }

Not everything passes (okay, 29 out of 228 pass), but it’s a good start. It compiles and runs at least.

The real trick in making these tests better is in grouping them along functional units. I usually use one test method for each method in the actual node being tested. That’s pretty easy to manage.

The initial loading and import-checking tests go into a method named test_load():

  sub test_load : Test(2)
  {
      my $self   = shift;
      my $module = $self->node_class();
      my %import;

      my $mockimport = sub { $import{ +shift }++ };

      for my $mod (qw( DBI Everything Everything::NodeBase Everything::XML))
      {
          $self->{mock}->fake_module( $mod, import => $mockimport );
      }

      use_ok($module) or exit;
      is( keys %import, 4, "$module should use several modules" );
  }

That seems sensible. Most of the rest of the work of translating this file follows that pattern — wrap each related group of tests in a method, extract out anything shared with the rest of the test module, and make everything work. Because Test::Class uses subroutine attributes to count the entire number of tests in a test file, there’s no single plan number set anywhere. This means that I can comment out the remainder of the file with __END__ right above code I haven’t migrated yet and test the entire code I have migrated. This is very useful, even if it’s an accidental benefit.

Oh, I need to add Test::Class to build_requires in Build.PL too. That’s also a good time to update MANIFEST and run the add file script.

After adding test_dbtables() and watching it fail, I rememberd that the order in which the tests run is not the order in which the methods appear in the file. That is, test_load() might not run before test_dbtables(). I moved test_load() to load_module() and called it explicitly from the startup() method.

Now it fails… but I can fix it. The problem is that I kept the attribute on load_module, so Test::Class saw it as a test to run. That’s a problem. I explicitly made load_module separate to allow for overriding. I’ll put it in startup() for now and perhaps create the mock object in create_fixture(), a method to run before each test method. Now all four (of 228) tests pass again.

For construct() and destruct(), I actually need a node. That means I need the fixture method. That’s fine. It’s:

  sub make_fixture :Test(setup)
  {
      my $self      = shift;
      $self->{mock} = Test::MockObject->new();
      $self->{node} = $self->node_class()->new();
  }

That fails because there’s no new() method. Interesting. (It’s actually in Everything::NodeBase, I believe. I know why this is, but I forgot that it is.) The fix is to add such a method to Everything::Node::node and to test it, of course. I don’t really like testing new() on every time through make_fixture(), but it’s there or in startup()… okay, startup() is better. Now 8 tests pass. Progress!

The next method is for insert(). Wow, there are a lot of tests here. There are probably several different types of tests, implying multiple test methods. The first set of tests checks that insert() checks user access. That seems like test_insert_access() to me.

Of course, the first time I ran the tests, they failed. This is because Everything::Node::node doesn’t inherit hasAccess() from Everything::Node. That clearly means a code change and a new test in startup:

  ok( $module->isa( 'Everything::Node' ),
      "$module should extend Everything::Node" );

I think that this is correct. Time will tell.

The test still fails, mostly because execution runs into Everything::Node where it tries to see if the user has access. That’s too much work. I need to mock something else here, probably hasAccess(). I’ve never used the Test::MockObject::Extends technique like I’m about to describe yet, but I think it will work. Here’s a quick change to make_fixture():

  my $node      = $self->node_class()->new();
  $self->{node} = Test::MockObject::Extends->new( $node );

Now in test_insert_access() I can write:

  $node->set_false( 'hasAccess' );

That worked. Very nice. I’ll have to remember this trick. This works by wrapping the object of the class being tested in a Test::MockObject::Extends instance. The wrapper object works like a normal mock object in that you can tell it to mock certain methods. It passes everything else through to the wrapped object. Because each individual test method gets a fresh object, there are very few statefulness issues to worry about — at least, if you keep your test methods small.

I think that’s all I want to test in this method, so it’s time to make test_insert_restrictions():

  sub test_insert_restrictions :Test( 2 )
  {
      my $self = shift;
      my $mock = $self->{mock};
      my $node = $self->{node};

      $node->set_true( 'hasAccess' )
           ->set_series( restrictTitle => 0, 1 );
      is( $node->insert( $mock ), 0,
          'insert() should return 0 if node title is restricted' );

      $node->{node_id} = 5;
      is( $node->insert( $mock ), 5,
          'insert() should return node_id if it is positive already' );
  }

That should be fairly obvious now. I like how much cleaner the code looks, too. Despite the fact that reading Test::Class code requires knowing how it works, knowing what the tests mean is much easier here. (I think it’s due, in part, to having decent method names as well as short test methods.)

I don’t have a lot of time left in my hour, so I don’t want to finish insert() or do update() or nuke(). There are a lot of shorter node methods I can do though: test_is_group(), test_get_field_datatype(), test_has_vars(), test_clone(), and test_commit_XML_fixes().

That’s not as much progress as I had hoped to make today, but some days are like that. This is good progress, even if it is slow. I learned a new technique that will make the rest of this coding work much better, and these refactored tests will be very useful in the near future. They should allow me to reduce a tremendous amount of duplication in the existing node tests as well as to refactor the code much more quickly.

Previous days showed how tedious it was to make changes to multiple node objects at once. Hopefully most of that trouble will disappear soon.

Instead of breaking the tests (as I didn’t finish migrating this one), I’ll leave the Test::Class code commented out in t/Node/node.t. That way I’ll still have it (and you can see it in the diff), but it won’t break anything if anyone downloads this revision.

Unfortunately, it did break the setting node tests. The problem there is the inheritance from Everything::Node which complicates life with an AUTOLOAD(). That’s fine; the fix is to make the mock object in the test handle the method it should handle. (The real fix is to get the hateful XML code out of the nodes, but that’s another day.)

Now all tests pass and I can commit. This is commit #838.