Code review comment for lp:~azzar1/compiz/fix-960652

Sam Spilsbury (smspillaz) wrote :

> This region of code is very fragile, so any automated tests would have to be external (not modifying code).

I would really prefer it if we did not use this as our reason for bypassing the "you touch it, you test it" policy.

Its possible to safely modify code to get it in a test harness. See the methods used in Feathers' Working with Legacy Code[1]. If people don't have that book, I would highly recommend that they talk to their line manager and expense it and read it.

In this instance, I don't see getting this code into a test harness as that difficult. For example, if you did sprout method here on the KeyRelease handler:

triggerAllReleaseEvents (unsigned int eventMods,
                         unsigned int eventKeyCode,
                         ModifierHandler *modHandler,
                         CompOption::Vector &pptions,
                         CompOption::Vector &arguments,
                         compiz::private_screen::EventManager &eventManager)

 state = CompAction::StateTermKey;
 bool handled = false;

 foreach (CompOption &option, options)
     if (isBound (option, CompAction::BindingTypeKey, state, &action))
  bindMods = modHandler->virtualToRealModMask (
      action->key ().modifiers ());
  unsigned int modKey =
      modHandler->keycodeToModifiers (eventKeycode);

  if ((event->mods && ((eventMods & modMask) != bindMods)) ||
      (!event->mods && (modKey == bindMods)))
      handled |= eventManager.triggerRelease (action, state, arguments);

 if (handled)
     return true;
        return false;

And then it would be pretty easy to write a test method for whether or not triggerAllReleaseEvents
a) actually triggered all events and
b) set the handled flag and causes the caller to return true;

Please spend some time considering how to test code when modifying it. Not only will you get the code under test, but you'll actually make the code better in the meantime, and you save headache for everyone else.


« Back to merge proposal