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 finish porting the
nodetype Test Porting
Today’s task is to finish porting the
nodetype tests that I
started on day 11. Two thirds of them work, so this should be fairly quick.
Well, I’ve fixed two thirds of the file anyway.
The next untested method is
nuke(), which removes a node from
the database. It’s a short method, but apparently I never tested it.
This one could be slightly trickier, as it introduces a new
getNode() call on the database at the start of all calls. I’m not
sure if that affects the mock in the parent test method; that could be a
problem. Regardless, running only the two tests of this method is fine.
I note with as much grim irony as usual that running these tests against a test database and not mocking database calls would solve this problem very conveniently.
For now, I’ll use a dirty trick and mock the
SUPER() call. I
don’t particularly like it, but it’s the simplest solution that comes to mind
right now. If
nuke() breaks in
node, its own tests
should catch it. Interestingly, now all of the 217 tests pass.
I idly wonder if adding a new type of mock object that associates mocks with packages based on the caller would solve this problem nicely without introducing really weird calling syntaxes.
Next up is
getTableArray(). After the short transformation,
four more tests pass and 72% of the test file has migrated.
The next method to test is
look at all that CamelCase. You may have noticed that my test methods use the
underscore and lowercase naming conventions instead. I find it easier to read.
Someday, maybe I can purge CamelCase from Everything entirely.
The method to test is only three lines of code. The test will be much longer, thanks to the method name.
Hm, when verifying that the first transformed test there failed, I noticed a
warning coming from
test_nuke_access() (inherited from the
node test). Though the tests pass, I do need to override this
method to set a mock on
Now setting the node field appropriately in the type permission test fixes that too, so 224 tests pass and 77% of the file is in the new style.
getParentType() is another short method, but it has a branch in
it, so it needs a bit of code in the test. First I forgot to update the test
count for this method, so three extra tests ran. Then I noticed that I forgot
to change the mock method checker to pull from the mock db object, not the
node. Now 228 tests pass and 82% of the test file is new.
hasTypeAccess() is slightly longer than the previous two
methods, but the tests are about the same length. There is no real control flow
here, which is why. While performing the transformation, I added another test
of the return value. I again forgot to switch one mock instance to the database
mock object. 233 tests pass and 86% of the file is in the new style.
Each new test method takes a couple of minutes to port; I’m into the short ones now and finding a rhythm. Having a nice automatic refactorer could speed this up even more, though I have a couple of Vim macros that make the job easier.
I swapped the order of the two tests for
isGroupType() to avoid
a bit of initialization code. I’m lazy. Two more tests pass. Still 86% of the
code is in the new style.
derivesFrom() is the longest method in quite a while. It has a
few points of control flow, making the test more complex. It should port
quickly though. It has six tests and one custom mock (using
mock(), not one of the convenience methods), but it worked on my
first try. 241 tests pass and 96% of the file is in the new style.
The remaining test is for
getNodeKeepKeys(). Though it
overrides a parent method and calls
SUPER(), it filters the
results of that call, so there’s no sense in running the parent tests for it.
With a quick translation, 236 tests pass and everything is in the new style
This means that t/Node/nodetype.t can now become a short test runner. Does everything still pass? Yes, and now there are 1973 tests. I think that’s a win. This is checkin #844.
I still have about 15 minutes left in my hour. The next largest node test to
port is for
setting. Why not?
$ cp t/Node/setting.t lib/Everything/Node/Test/setting.pm
After adding some test header (and porting 5% of the test file to the new style, ha ha), 212 tests run and 9 fail. Of course, lots get skipped because of exceptions. That’s fine.
The first test to fix is for
dbtables(). That’s quick. Now only
8 tests fail. (By the way, there are 9 methods in this node. That also ought to
be quick to port, but I probably won’t get it all done today.)
The next test is for
getVars(), and with it in place two more
tests run and still 8 fail.
setVars() is almost as quick. It just needed wrapping in a
method and a new mock. 216 tests pass and eight fail. I’ve migrated 20% of the
test file in five or six minutes.
hasVars() is even shorter and faster. It doesn’t add any tests,
but it replaces a failing parent test method, so now only 7 tests fail.
I have tests for
destruct(), but I
don’t need them.
Next come XML methods. Yuck.
fieldToXML() is pretty easy to migrate. It adds five tests and
I don’t want to mess with it. (I still hope this alternate representation goes
into the appropriate NodeBase subclass sometime, probably in a role.) Making
the test method call the parent test method isn’t correct, though; this node
also needs a
SUPER() upgrade. After that fix in place, 222 tests
run and 12 tests fail. I’ll probably fix them as I go on.
The next method to test is
xmlTag(). It’s also long. It looks
like it should work, but it fails when
node tries to call
parseBasicTag(). It’s not immediately obvious why this happens,
but I can debug it.
SUPER() within the test method, all six tests
pass. It’s only when calling the parent test that things go wrong. Again, the
problem is that the child method consumes one of the mocks… this probably
will come up again and again. For now, mocking
SUPER() is the
Now 219 tests run and 11 fail. I’ve ported 59% of the test file and may be able to finish today. Three methods remain, and the latter two are very short. The next one is another XML method however.
test_apply_xml_fix() method running, lots of skips
occur, but 230 tests run and only 3 fail. This is very nice. I think the
exception problem is because of the use of
TieOut instead of the
error logging I built into the node tests a couple of days ago. After fixing
that, 9 tests of 232 fail.
Some of those tests are due, again, to method calls in the child implementation of the tested method that affect the mocks of the parent testing method. Again, this means that I will skip the parent test method for now. 224 tests run and 2 fail. I’ve ported 94% of the file. There are two methods left to test.
getNodeKeepKeys() is another method for which running the
parent test doesn’t make sense (at least as I’ve coded it). Porting the test
was quick, and 2 tests of 216 fail. 96% of the file is in the new style.
Adding a test for
updateFromImport() fixes all but one of the
test failures. (This is another overridden test method that cannot call
SUPER(). I need to reconsider how this all works together
The remaining test failure is in
the test for no fixby node in the
node test. Hm, it looks like the
behavior of the
setting node is actually in error; should it
return the fixby node in case of an error or nothing?
According to the documentation of
nodegroup, it should return
the fixby node. Hey, this test uncovered a bug! Fixing the test causes another
three tests to fail. Now to fix the code.
After fixing the code, all 215 tests pass. That’s good for another checkin. Again I went slightly over my allotted time, but now I have three tests in the new form and am making progress.
Over the weekend I should have time to consider the
problem. Maybe it’s not a big deal. Maybe it is. I’m sure that moving to
database-driven tests will help a lot of that go away though.
workspace tests fail now. The diff should explain why… but it
didn’t. All three fail because of a
SUPER() problem disallowing
the use of class methods. This is a problem in
SUPER module. For now I can just disable those tests. I do
wonder why I’m only seeing those failures now, though.
Oh right; these three must inherit from
setting. Skipping the
dbtables tests should fix everything. The rightest thing
to do is to fix everything to use
SUPER() properly, but that’ll
take too long right now and I’ll modify these tests anyway.
Now 2143 tests run and pass (except for the skips) and I can check in everything as #845. See you next time.