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 to port another node tests to the new system. Unfortunately, the order of porting matters quite a bit… problems abound!!

Day 13: Why Going in Descending Size Order Was a Mistake

As promised, I thought about how to solve my SUPER() problem. I’ve decided that the fragility of testing reveals an inherent problem in the design of the code. Of course, I already knew that it wasn’t perfect — that’s why I’m refactoring it.

I could parametrize the tests somewhat. That is, I could add extra data methods to each test class so that I can override a few small methods and expect the parent test methods to pull in that data and test the right things there. In some cases, that might mean I duplicate those data methods between the tests and the code being tested.

I think the wisest course is not to make any big changes to the code until I get better test coverage, and then plan to refactor the tests yet again when I refactor the code. I haven’t seen much discussion about refactoring tests, but it seems wildly obvious that it’s worthwhile in some cases.

Today’s first task is to refactor the user test. It’s short, so this should be fast. There are only ten methods in 200 lines of code and comments. I like that ratio.

First I change t/Node/user.t to run the test class methods. What fails? Nothing. I forgot to override node_class(). Now 212 tests run, 18 fail, and plenty get skipped. Great!

The first test to override is for dbtables(). The first two tables should be user and document. Of course, the test failed because of the incorrect use of SUPER(). That’s fine.

That reminded me to check the inheritance, so I extracted test_extends() from the startup tests in the node test. The new test method only checks the inheritance of the class. It’s a short test method, but it’s overridable.

Going back to the SUPER() failure, I’ll add the hack I used in the nodetype test to inject SUPER::SUPER() into the user node class. I don’t like it for now, but it works without breaking anything else… yet.

The next method to test is insert(). It’s short and easy, so it should be easy to port the tests. After realizing that some of the failures are because I forgot to use Test::More in the test header, 208 run and 17 fail. It’s on to the next method.

isGod() checks the access of the user. It’s another short method, with several control points. This adds three tests, so 211 run and 17 fail. I deleted a couple of tests that checked the call order of traced, mocked calls. If the mocks didn’t work, the following tests wouldn’t pass. I don’t worry so much.

The next method is isGuest(), a similar check for no access powers for the user. I also deleted a couple of mock trace checking tests. With four new tests, 215 pass and 17 still fail. That’s progress. Oh, 56% of the test file is now in the new form. This is going fairly quickly, though I worry about some of the inherited tests. Time will tell.

The getNodeKeys() method helps the system to know which nodes to export. It makes sure not to export user passwords and the time of the user’s most recent activity in the system. After porting the tests, 220 pass and 17 still fail.

Now I wonder if doing a quick s/:Test/#&/ in the node test will make it easier to debug these failures…. I’ll try that when I get this ported.

I’ve ported 67% of the test to the new style. The next method is verifyFieldUpdate(), a short method with lots of tests for some reason. Oh, I changed my style when writing these tests. There aren’t that many.

After porting this test, 222 run and only 16 fail. I made one pass somehow!

conflictsWith() and updateFromImport() both return false; they’re easy to test. 216 tests run (there’s no reason to inherit the updateFromImport() test method) and 13 fail. I had a feeling this would happen.

restrictTitle() may be another of those magic methods. It doesn’t call SUPER() and it does some work. After migrating it, 213 tests run and 12 fail. It’s not quite comprehensive, but it’s not bad. I’ve also made it to 81% of the file.

Of course, there’s only one method left, and that won’t fix all of the failures. Hmm. I’ll fix the test for getNodelets() and then figure out the other problems. I think they’re still SUPER()-related problems.

getNodelets() returns the nodelets (little boxes of cut-down node views) that the user has selected. It’s a method original to this class, so it won’t fix any inheritance problems.

After creating test_get_nodelets() (and fixing a typo where $sef is not $self), 216 tests run and 12 fail. Why do those fail?

The diagnostic messages are the well-loved “Un-mocked method ‘getType()’” errors from Everything::Node, which makes me think there’s a SUPER() problem somewhere.

It looks like the real problem is that Test::MockObject::Extends and SUPER do not work well together when the extended object inherits from something else. I can fix this, because I maintain both modules.

After fixing that, I noticed two problems with the getNodeKeys() test. First, the SUPER() call in the module didn’t pass the parameters. Fixable! Second, the mock in the user test mocked the wrong method. Also fixed. 216 tests run and 10 fail.

The next failure was in hasVars(). I think this is a failure because I haven’t ported setting to the new tests yet, and that definitely has variables. I can fix that with a short, deletable test in user for now. 216 tests pass and only 9 fail.

test_insert_access() is the next failure. This is because insert() in node returned false, not 0 on failure. Now 216 tests run and only 6 fail. There’s a method calling failure here in node because the inherited test doesn’t do anything to mock the update() call. (When a user gets created, insert() performs the insert, then sets the author_id field of the user to itself, so that the user owns himself. Then it calls update().)

The solution to this is to override test_insert_restrict_dupes() and test_insert_restrictions() to mock update(). That’s all. 216 tests run and only 4 fail.

The next failure is in test_restrict_title(). The problem here is that I mis-ported the tests. Oops. 216 run and 3 fail.

The only remaining failures are in xmlTag(), of course. This is because I haven’t ported the setting tests to the new form, so I’ll mock up a test method for now and remember to remove it later. Yes, it’s cheating, but it was my mistake to go in the wrong order. Besides, user will just inherit the tests anyway.

216 tests run. Test::Class magically skips the 9 xmlTag() tests. Everything passes.

Does the rest of the code? Yes. 2317 tests now run. Great! I’ve also updated Build.PL to require new versions of Test::MockObject::Extends and SUPER, so I need to upload those soon.

This is checkin #846 and the end of a day.