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 maintainability. 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 a race against the clock to get just one single test running against the live, not a mocked, database. Can chromatic do it? His reputation is on the line!

Day 30: Endgame

When I ended day 29 (and it’s been a few days, with Real Life and Day Job intruding), I was halfway to being able to run one node test out of the database. When I stopped, I had the entire test suite still passing, despite moving around some database functions to achieve a clear separation of concerns. Clearer, anyway.

My first place to start is running the test suite again. 7657 tests run and all pass. I still have only some confidence in the results, but tests broke before when I broke the code, so I know they catch some errors. Besides that, if this works, the tests will get a lot better very quickly.

With the very bare-bones setup I have at the moment, the only nodetypes in the test database right now are node, nodetype, and setting. The node test class is the largest node test class, but it’s also the most widely inherited, so porting one test there (node creation?) to run against the testing database might be useful.

That’s test_insert(). Deep breath.

Before this works, I need to modify the test fixture to create the test database. I probably also need some sort of handle to its DBI object so I can run queries against it. This isn’t a big change from the current system; that’s basically what the mock database does. I’m not going to get rid of the mock database, however — it’s still useful to simulate failures.

The fixture setup needs two parts. The first is per test class, for copying the standard empty testing database to the right location and inserting the test class’s base nodes. The second is per test, clearing out all of the cruft from the previous run. This makes me think a two-step approach is best; moving files around is easy.

In the test file, I’ll assume that the build process has built t/ebase.db. It might be the wrong assumption, but for now it’s the simplest idea I have. (I can also change it in one place here.)

After a few minutes of hacking, I have a small patch that mostly pleases me:

  === lib/Everything/Node/Test/node.pm
  ==================================================================
  --- lib/Everything/Node/Test/node.pm    (revision 17847)
  +++ lib/Everything/Node/Test/node.pm    (local)
  @@ -5,10 +5,13 @@
   
   use base 'Test::Class';
   
  +use DBI;
   use Test::More;
   use Test::MockObject;
   use Test::MockObject::Extends;
   
  +use File::Copy;
  +use File::Spec::Functions;
   use Scalar::Util qw( reftype blessed );
   
   sub node_class
  @@ -24,6 +27,19 @@
       my $self         = shift;
       $self->{errors}  = [];
   
  +    my $blank_db     = catfile(qw( t ebase.db ));
  +    die "No testing database!\n" unless -e $blank_db;
  +
  +    my $module       = $self->node_class();
  +    my $module_db    = catfile( 't', $module . '_base.db' );
  +
  +    copy( $blank_db, $module_db )
  +        or die "No test database for $module $!";
  +
  +    $self->{base_test_db} = $module_db;
  +
  +    $self->populate_base_database( $module_db );
  +
       my $mock         = Test::MockObject->new();
       $mock->fake_module( 'Everything', logErrors => sub
           {
  @@ -32,7 +48,6 @@
       );
       *Everything::Node::node::DB = \$mock;
   
  -    my $module = $self->node_class();
       my %import;
   
       my $mockimport = sub { $import{ +shift }++ };
  @@ -49,6 +64,9 @@
       isa_ok( $module->new(), $module );
   }
   
  +# override if necessary
  +sub populate_base_database {}
  +
   sub test_extends :Test( 1 )
   {
       my $self   = shift;
  @@ -77,8 +95,23 @@
       $self->{mock_db}            = $db;
       $self->{node}{DB}           = $db;
       $self->{errors}             = [];
  +
  +    $self->make_test_db();
   }
   
  +sub make_test_db
  +{
  +    my $self         = shift;
  +    my $method_name  = $self->current_method();
  +    my $base_db      = $self->{base_test_db};
  +    my $test_db      = catfile( 't', $method_name . '.db' );
  +
  +    copy( $base_db, $test_db )
  +        or die "Cannot create test db for $method_name: $!\n";
  +
  +    $self->{test_dbh} = DBI->connect( "dbi:SQLite:dbname=$test_db", '', '' );
  +}
  +
   sub reset_mock_node
   {
       my $self      = shift;

The test still passes. The only problem is that this will leave test database all over the place. That’s no good. I like File::Temp to create temporary directories. That’s one more piece of data to store in the Test::Class object, but that’s okay. Here’s the new patch:

  === lib/Everything/Node/Test/node.pm
  ==================================================================
  --- lib/Everything/Node/Test/node.pm    (revision 17847)
  +++ lib/Everything/Node/Test/node.pm    (local)
  @@ -5,10 +5,14 @@
   
   use base 'Test::Class';
   
  +use DBI;
   use Test::More;
   use Test::MockObject;
   use Test::MockObject::Extends;
   
  +use File::Copy;
  +use File::Temp;
  +use File::Spec::Functions;
   use Scalar::Util qw( reftype blessed );
   
   sub node_class
  @@ -24,6 +28,8 @@
       my $self         = shift;
       $self->{errors}  = [];
   
  +    $self->make_base_test_db();
  +
       my $mock         = Test::MockObject->new();
       $mock->fake_module( 'Everything', logErrors => sub
           {
  @@ -32,7 +38,7 @@
       );
       *Everything::Node::node::DB = \$mock;
   
  -    my $module = $self->node_class();
  +    my $module     = $self->node_class();
       my %import;
   
       my $mockimport = sub { $import{ +shift }++ };
  @@ -49,6 +55,28 @@
       isa_ok( $module->new(), $module );
   }
   
  +sub make_base_test_db
  +{
  +    my $self      = shift;
  +
  +    my $blank_db  = catfile(qw( t ebase.db ));
  +    die "No testing database!\n" unless -e $blank_db;
  +
  +    my $tempdir   = tempdir( DIR => 't', CLEANUP => 1 );
  +    my $module    = $self->node_class();
  +    my $module_db = catfile( $tempdir, $module . '_base.db' );
  +
  +    copy( $blank_db, $module_db )
  +        or die "No test database for $module $!";
  +
  +    $self->{base_test_db} = $module_db;
  +    $self->{tempdir}      = $tempdir;
  +    $self->populate_base_database( $module_db );
  +}
  +
  +# override if necessary
  +sub populate_base_database {}
  +
   sub test_extends :Test( 1 )
   {
       my $self   = shift;
  @@ -77,8 +105,24 @@
       $self->{mock_db}            = $db;
       $self->{node}{DB}           = $db;
       $self->{errors}             = [];
  +
  +    $self->make_test_db();
   }
   
  +sub make_test_db
  +{
  +    my $self         = shift;
  +    my $method_name  = $self->current_method();
  +    my $base_db      = $self->{base_test_db};
  +    my $tempdir      = $self->{tempdir};
  +    my $test_db      = catfile( $tempdir, $method_name . '.db' );
  +
  +    copy( $base_db, $test_db )
  +        or die "Cannot create test db for $method_name: $!\n";
  +
  +    $self->{test_db}  = $test_db;
  +    $self->{test_dbh} = DBI->connect( "dbi:SQLite:dbname=$test_db", '', '' );
  +}
  +
   sub reset_mock_node
   {
       my $self      = shift;

I also broke out the initial test database initialization into its own method. I may never override this in the tests, but the setup fixture was getting too long for my comfort. I like to use method names as documentation in lieu of comments, so whenever I feel the urge to write a comment to explain a paragraph or two of code, I factor out that code into its own functional unit.

The test still passes. Great. Of course, I didn’t change any behavior, but that’s okay.

My next goal is somehow to get the node being tested to use the right database. Hmm. The obviously right solution is to change the way the node gets created. The easy way is to stick something else in the DB attribute of the node.

I like easy, especially as I only need it for just this one test right now. (Now that I know that this will be a problem I have to solve in general, eventually, I can let my subconscious brain think about it while I fix this one little piece. It’s also a lot less risky to change the behavior of just this one method.)

Of course, when I tried that, it failed badly, so I went for the second change and used the Test::MockObject::Extends wrapper trick around the actual object. I modified the per-test fixture, and all of the tests still pass. Excellent.

Another problem popped up then, namely that Everything::NodeBase didn’t forward the getFieldsHash() method to the storage object. That was an easy fix, once I figured it out.

The other problems were a bit subtler. For starters, really inserting a node into the database requires that you have the nodetype created and installed appropriately. That took me a while to track down. Fortunately, calling getType() on the nodebase with the proper nodetype fixes it.

I also had to change the way lastValue() worked for the SQLite object; somehow using the standard DBI approach didn’t work. I might fix that later.

I did remove a couple of tests, specifically the ones that check inserting a complex node with multiple tables and the caching behavior. I need to think more about how that code works.

However, that leaves three tests of insert() that actually use the database. Now that is what I wanted to do.

I just need to write a little bit of logic to create the test database during the build, and that’s that. I decided to put it in t/lib/build_test_db.pm and use it from the setup fixture after all.

With that in place, only two tests fail in the htmlpage tests. Hmm. Adding a clear() call on the node fixed it. With that in place, 7496 tests run and all of them pass.

There are some warnings, but I’m at the end of the day and I’ve had my success. This is checkin #892, the end of the day, and the end of the project. I will follow up with a retrospective soon.