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, yet again, continuing to port tests from the procedural form to the Test::Class form.

For the third day in a row, the task is to continue making Everything::Node::Test::node pass all of its tests in the new Test::Class system. I explained this technique at lunchtime to ORN developer Jonathan Wellons and he thought it was interesting.

When I left off, I had started migrating the insert() tests again. That seems like a good place to continue, as cherry-picking won’t get me very far again. So far, 40% of the file is the new style.

Looking at the code again, the first snippet should go in test_insert_restrict_dupes(). Oops. That’s fine.

I cut and pasted the code and modified it trivially, just to see what happened when it ran. Things failed oddly and I tracked it down to the realization that the succeeding insert() call actually modified the node in place, replacing even the database object it contained. That’s worth knowing, so I added a comment to the test to reflect it, before resetting the database.

It actually also replaced another key. That’s fine. After fixing those, the test passes. I have the temptation to undo several mocks, just to make the code shorter. Hey, why not? (The less code there, the less code to maintain. Besides that, the existing test tends to blend mocks for one test into another pretty badly. Yuck.) Now there are only four mocks, not eight or nine. Nicer.

I’m fairly confident in this test, but as I mentioned before, this is a long method and I expect to make it shorter and move it elsewhere eventually. If it goes really badly, I also expect things to fail and suggest new tests to write while fixing the bug. Time will tell if that’s false or genuine laziness.

I don’t particularly care about the next set of tests that check the node insertion and such. They know too much about the internals of the objects… but I suppose they are valid. I decided to add them to the rapidly growing method, and clearly it was a good idea, because they failed.

At least, I assume that they were a good idea. They could be a bad idea, but they seem like related tests, so if they’re not passing here, I assume that my migration is incomplete.

(Of course, then co-workers interrupted me and I lost my place, but that’s okay.)

After the interruption, I traced the calls better and realized that a couple of the mocks I deleted were necessary. I’m glad I checked. Now the tests all pass again and I have my confidence back. Of course, there’s still some code in the tests that relies on the order of calls on the mocked objects and I wish that weren’t there, but with a better test suite that can actually work with a test database, this will be refactorable even further.

For now, having this much working is progress. 79 tests pass now, out of about 200 or so. 45% of the test file is in the new form. That’s very nice.

Next up is update(), with 51 lines of test code. That’s not so bad. It’s been a few days since my last new test method, so I feel a little rusty. (I’m also in an open office with lots of co-workers, so it’s an unfamiliar environment, but that shouldn’t make a big difference. I only mention it to bring up the idea that familiarity and mood have a big difference on projects like this. It’s usually much more fun to refactor a project only as you need it as you normally work on it and see small changes you can make. Doing a big project like this is a lot less fun, usually.)

I don’t know yet if update() needs multiple test methods. Maybe. Looking at the actual code of the module, it looks like there should be access tests, workspace tests, and the normal tests. Experience writing tests makes it easier to see all of the testable code paths. I don’t know a better way to describe it.

Writing the first method, test_update_access() takes about a minute. Everything passes. Great!

The next test method is test_updte_workspaced(). I like having this as a separate method, becaues it means that this can go in the appropriate test file when the workspace code moves into a separate subclass of the nodebase.

I do still have to test it now though. Ugh. After making the code look right, I had two problems. The first was that I hadn’t changed all of the instances of $mock to $node. I decided not to do a search and replace on the code, because I was lazy — falsely lazy. Then tests failed because I’d replaced too many and wasn’t using my $self->{mock_db} object correctly. That’s fine. Those three tests now pass too and I can ignore the workspace code. (The tests aren’t great, but they exist and they pass and they appear to do the right thing. Besides, there are very few people using workspaces in practice.)

That leaves 28 lines of test code for the normal update() path. Idly I wonder if there’s no test_insert() method by default. That’s probably a problem. Today I have a short attention span – there’s so much stimulation around me. I think the last set of tests I made for test_insert_dupes() should be in its own method. I know it works now and tests the right thing because it’s part of the duplicate-restricting test. It should work on its own in its own method. I’ll do that next.

Merely splitting the test method in half (and adding the appropriate header) didn’t work. That’s fine. That’s probably because it loses the mock for methods such as hasAccess() and getId(). Adding those back makes the test compile, though three of ten fail. That’s progress!

Another missing mock is lastValue(). Adding that makes the remaining tests pass, and now I have a test_insert() method that I can inherit. That’s much better. Now back to update().

Again, the reuse of one mock object for different types of mocks (the node, the database, et cetera) is a problem here. I’ll try to avoid that mistake in the future. My first attempt at making this method pass failed miserably. After untangling what I needed to mock where, the code compiled and ran with plenty of failures and plenty of unmocked method call warnings. That’s fine.

The first set of failures went away by mocking the right methods. This is one of the dangers of reusing mock objects within the same namespace. It was too tempting for me to assume that one pass through the method being tested was sufficient, and I merged all of the mocks together. Of course, without Test::MockObject, the mocking code was verbose and ugly enough that I wanted to do that.

Some code sort of passes correctly; the mock database creation only happens in the startup method, not the fixture creation. That’s probably a mistake. I can fix it. That took a bit of work, as I reused the mock database object for a couple of things in the startup as well as to populate the global $DB variable, but it works.

Okay. That should make some of the tests easier to write, as it gets rid of other statefulness that could get in the way. After making sure that the mock object logs the methods appropriately, everything passes again.

Everything still passes — that’s 96 tests. 53% of the test file is in the new format. I hope I can finish refactoring this test in the next day. This is taking much longer than I anticipated. (Of course, having four big interruptions today meant my hour stretched out over several, but that’s okay.)

Of course, most of the rest of the tests to migrate deal with workspaces again. Sigh.

The entire system still passes, though, so I can check this in… except that the hotel wireless system won’t allow SSH/Svn access. Oh well. I’ll do it tomorrow at work.