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
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
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
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
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
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
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
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
nodegroup tests still pass. So do the other tests. Great!
This is checkin #854.