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 and setting tests!

More 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. Weird.

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 getDefaultTypePermissions(). Wow, 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 getNode(). Done!

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 now.

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 construct() and 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.

Without calling 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 solution.

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.

With the 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 perhaps.)

The remaining test failure is in applyXMLFix(), specifically 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 SUPER() 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.

Oddly, the themesetting, user, and 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 Everything::Node, not the 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.