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.

Fixing Failing Tests

Today the goal is to fix the failing tests. Who knows what I’ll find? I’ve tried this a couple of times and usually ended up frustrated that the tests aren’t as nice as they would be if I’d written them today. (Of course, writing the tests as they were a few years ago taught me what I know today, so there’s a circular argument in there somewhere.)

What fails? I forget. Run the tests again.

  $ ./Build test

Only:

  t/ehfpermissionmenu.t    2   512    26    2   7.69%  22 24
  t/ennode.t             255 65280   226  388 171.68%  33-226

First things first — the permission menu test. Oh — the test files have names corresponding to what they test. Someday I’d like to unflatten the hierarchy and store this under t/Everything/HTML/FormObject/permissionmenu.t or something. Maybe tomorrow, or even today if I fix the tests in time.

  perl -Ilib t/ehfpermissionmenu.t

There are uninitialized value warnings probably getting in the way and two failed tests. Looking in the test file, I see a few bits I don’t like, so I might as well perltidy them. Why not just perltidy all of the tests? Makes sense to me.

  for i in `find .`; do echo $i; perltidy -b $i; rm "$i.bak"; done

Yes, this won’t work on directories, giving an error. That’s fine. I’m too lazy to look up the right invocation in the find manpage before issuing the command (find . -type f, actually). This takes longer than I anticipate, but that’s okay.

This huge commit is #826. Committing takes a while too.

Okay. The permission test still fails. I’m really not sure what’s going on and FakeNode bothers me, so I’ll try porting a few of the tests to use Test::MockObject instead. (Yes, I wrote the latter after realizing how much nicer the interface to the former could be.)

Oops. After five minutes of that, it’s really not working and I don’t care to stumble through the tests to figure out why. I plan to port the tests to Test::Class anyway someday (probably as or after turning the node objects into proper, Perlish objects), so it’s a low priority. Back to the failing tests.

Ahh, looking at the code in Everything::HTML::FormObject::PermissionMenu, the problem is that the expected field uses a double-colon separator. This was to fix an old bug reported on SourceForge where fields with colons in them already had big problems — so we moved to a double-colon separator instead of URI-encoding the form data appropriately. No one updated the test to use double colons in the mock data. Changing the one line (the getBindField() sub definition for FakeNode) fixes both failing tests. Nice. This is commit #827.

I also ran into something weird where the path appears wrong for the code when running perl -Ilib t/test_file.t, but I fixed that by using -Mblib instead. I’ll look into that later.

Thinking more about this bug, it would be nice to only care in one place about the encoding of multiple Everything values in a single form value. That’s definitely a code smell here. I’ll fix that later too.

The other failing test looked awful. Running it reveals only that it already uses Test::MockObject and an unsuccessful mock (or incomplete) is causing the error around test #33. That means most of the 194 failing tests will probably pass. (Everything::Node::node is a big file that I would like to see shrink. It will.)

Telling the mock object to handle the canWorkspace() method without logging it or even doing anything bypasses the error and the test file completes, with only one failing test. Excellent! Now the real debugging begins.

(It’s experience with Test::MockObject and making silly mistakes that let me fix that so quickly.)

This test fails within nuke() (which removes a node from the database) within the call to sqlSelectMany(). My initial guess is because the API there changed somewhat. (You may be thinking right now, “Why is the database code all mixed up in here?” and that would be exactly my instinct too. Repeat after me. “Fix later.”)

What is the API to sqlSelectMany() and where is it? It’s part of Everything::NodeBase. It takes:

• a string list of columns to return
• the name of the table on which to select
• a string containing the search criteria
• other SQL options to pass
• an array reference of bound values for placeholders

Looking at the test, it ignores the fourth argument and tries to find the fifth in that slot. That won’t work. Changing 4 to 5 in the test makes it pass. That should fix the entire test file. It does. Now for another checkin, number #828.

You might wonder why I didn’t say anything about that API or its use or even wonder why someone made this mistake. The answer to the first should be familiar. (It’s on my list of things to fix.) The answer to the second is more abstract. I could tear through the checkin logs or annotate the code to see what changed and why. Was it the test? The API? Should I worry about similar bugs in other tests or the code (does it have enough tests?)

The error occurred so long ago that there’s not much I can learn from it now. I just wanted to get the tests to work to have some baseline from which to work.

I have about 15 minutes left to work today, so maybe it’s a good place to move the test files around. I could edit them to fit more modern conventions, but that’s mostly just busy work that won’t make my life easier. Putting them in better directories allows people to map the test files to the modules they actually test more easily, though, so that’s a win.

The old t/base file is the base of all tests; I added it here before I developed a more general “start a new test” template-based Bash alias, so there’s little point in keeping it. Whack:

  $ svk rm base

I assume that people know that everything under t/ refers to Everything::*, so there’s no real need for an Everything/ directory. Oh, that reminds me. I have to keep MANIFEST up to date.

The easiest way to make these changes seems like editing a file, then running it through the shell.

  $ chdir t
  $ ls -1 >  list>
  $ vi list

This puts a list of all files in t/ into list. I edited this file, prepending svk mv to the start and putting the directory and new name at the end. I kept the MANIFEST open in another terminal to help me get the spelling right. There are a handful of untested modules here too. I think they’re in the SourceForge task list. I’ll get to them.

I need to add a couple of directories.

  $ svk mkdir HTML
  $ svk mkdir HTML/FormObject
  $ svk mkdir Node
  $ svk mkdir NodeBase
  $ sh list

That should be it, apart from editing the MANIFEST. Of course, ./Build manifest does that. (I missed a few files earlier, strangely enough.)

Do all the tests still pass? I hope so… though I haven’t edited Build.PL to find them all. Only the ones in t/*.t run. Oops. File::Find time! I figured out how to make this work with Module::Build in Games::PMM, so stealing from that:

  use File::Find;

  my @test_files;

  find( sub { return unless /\.t\z/; push @test_files, $File::Find::name },
      't/' );

  # within the Module::Build constructor call ... 

      test_files         => join( ' ', @test_files ),

  # ...

With this in place, all of the tests run, but two throw warnings and one fails. Oops. More debugging.

Hmm… this one fails because Everything::Node::theme does not inherit from Everything::Node::nodeball. That’s because the code doesn’t do it either. Weird. I assume it should, so I have to fix the code. At least, I would, if I thought that inheritance worked Perlishly here. Unfortunately it doesn’t. Then again, there’s probably no problem fixing it to do so. That means fixing the test a bit too. I hope this merges safely.

Now for the uninitialized value warnings. One’s in t/Node/node.t. Actually, there are two here. They both relate to the use of Everything::logErrors(). They test is mocking them inappropriatey – how odd. Regardless, the tests pass, mostly because of bad regular expressions. Ugh. Fixing the regular expressions (hey, don’t allow empty strings!) makes the tests fail as they should, meaning that fixing the tests is now obvious. That’s a fix.

I wonder if the other error (in t/HTML.t) is similar? Looking at the code, it’s not. It comes somewhere within the test of Everything::HTML::urlGen(). I have the sick feeling it’s in the guts of CGI. After tracking it for a few minutes, it looks like the attempt to set the script name within %ENV isn’t working – because it’s a full URL in the test, not just the relative name on the server. Changing the script name to /this.pl fixes the warnings, but breaks the tests, which don’t expect the name of the script in the URL. That’s fine. I know what to do now and can fix the tests.

Mocking things in tests makes life difficult sometimes — but until the architecture of Everything improves with regard to testability, this has to happen.

Everything should pass now without errors or warnings. It does. Here’s another big checkin, #829. Oops. The MIME type is wrong on all of the new test files. Unfun. That’s okay, that’s fixable with the script from day 1.

  $ scripts/add_text_file.sh `find t/ -type f`

Of course, having added all of these files already, I’m only running this to set their properties. That’s okay. It just takes a few moments more and gives warnings that Svk has already marked these files as added. I suppose there could be a flag that just sets the properties, but that’s more work and argument handling in the shell and I started to do it before realizing how unpleasant shell argument handling is. That’s fine.

I bet most of the files in the repository have the wrong MIME types set, so this’ll happen every time I move something. I could run this program from the root of the repository, then svk revert the changes in images/, which contains non-text files… seems like that could save some time. (I could be extra sneaky and temporarily comment out the svk add line… very clever. That’s especially handy because there’s now a _build/ and blib/ directory.) This is checkin #830.

That’s my hour (and a few minutes) for the day. See you next time.