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 continue porting the nodegroup tests. This is a great way to test the inheritance of test methods.

Day 17: More Nodegroup Porting

When I stopped on day 16, the new nodegroup test class ran 254 tests and 41 failed. I’d ported 30% of the tests.

The first test method to port is nuke(). It looks like inheriting the test_nuke_access() call works fine, so there’s deletable code. Seven tests run and pass (after passing the arguments to SUPER()), bringing the total to 234 tests and 19 failures. I think this can inherit from test_nuke(), but I want to try that first.

Adding SUPER() naively to the end of this test method runs 261 tests, of which 35 fail. That’s 16 more failures out of 27 more tests. What’s the problem? Mocks again. Unmocking SUPER() in the node makes 41 tests fail.

After shuffling the position of the SUPER() call, it’s clear again that the problem is that the parent and child implementations call the same methods, so the mocks don’t know what to report or where. That’s fine. Deleting the fresh code goes back to 234 tests with 19 failing.

test_is_group() takes a moment to write (but the parent implementation didn’t fail because there was no derived group table set in the mock — interesting). 234/19 again.

test_in_group_fast() is also easy to write, though I made one typo and one logical error. 238/19.

inGroup() is also straightforwardd. I like how some of these methods are still decent enough code. There are 12 tests in the test method (I counted wrongly a few times), but three of them fail. Why? Probably mismatched mock expectations.

After reading through those again, I decided to remove some of the next_call() calls — there’s no reason to check the specific order of things if I can check a couple of mocks and make sure that everything is correct. That makes 248/19, with 43% of the code ported.

selectNodegroupFlat() is longer than I like, but it seems straightforward. There are ten tests (and a missing semicolon and my when I ported it). Three of them fail. There are also uninitialized value warnings, because this set of tests depended on data being set earlier in the test file. Fixing that fixes the warnings, but doesn’t fix the three failing tests.

The problem is that this method is recursive, at least as I’ve coded it. I need another mock object. (Mocking this method on the node means that the tests will only call the mock, not the actual method being tested. That’s somewhat useless.) With the new mock, returned from the mock database, 258 tests run and 19 pass.

My first pass at tests for insertIntoGroup() failed, again because they relied on data from a previous test still being in the node. I really like having a fresh node on every test method, so I feel pretty good about finding errors like this. After adding a test for an untested call, 269 tests run and 19 fail. I’ve ported 54% of the code and still have about 20 minutes left in my hour. This is good progress.

There are some unnecessary mock logging tests in test_remove_from_group(), so I deleted them. Now 276 tests run and 21 fail. Why? One is because I forgot to mock groupUncache(). Fixed. The other is because I mocked getId() on the node, not the mock database. Also fixed.

This test lets me clean up some too-clever code in the method itself, replacing a loop and a nested splice() operation with a simple grep().

276 tests run and 19 fail. I have just under 15 minutes remaining. Can I get over two-thirds of this test ported?

test_replace_group() ports quickly (two minutes) and runs correctly the first time through.

test_get_node_keys() also ports quickly. Now 285 tests run and only 17 fail. Can I call SUPER() on this method? With a quick experiment, I can get to 289 tests running and 18 passing, but I’m not sure that there’s an easy way to make the final test pass (because nodegroup adds an extra exportable key that node shouldn’t know about). No inheritance then.

That’s 65% of the file and it’s close to my hour (and the next tests to port are the hateful XML method tests). How does everything else work? All of the old nodegroup tests still pass. So do the other tests. Great! This is checkin #854.