[buddypress-trac] [BuddyPress] #4470: BuddyPress Singleton

[buddypress-trac at lists.automattic.com]

#4470: BuddyPress Singleton
------------------------------+------------------------------
 Reporter:  johnjamesjacoby   |       Owner:  johnjamesjacoby
     Type:  enhancement       |      Status:  new
 Priority:  normal            |   Milestone:  1.7
Component:  Core              |     Version:
 Severity:  normal            |  Resolution:
 Keywords:  has-patch commit  |
------------------------------+------------------------------

Comment (by foxly):

 >> Why a unit test would want to "tamper" with variables outside of the
 boundaries BuddyPress
 >> internally allows defeats the purpose of that test, since it will
 clearly always fail;
 >> rightfully so.

 The reason its imperative that unit tests be able to access and modify
 private methods and variables is so that you can get an acceptable level
 of test coverage (80%) without having to use an astronomically complicated
 test rig.

 Listed below is the class 'misery()' from one of my second year CE
 courses. It's laid out how you're proposing to structure your new BP
 classes: private methods and variables for things that other developers
 aren't supposed to 'mess with'.

 So to write a unit test for this class, all you have to do is run it
 through each possible input state, and check the output is valid. Therein
 lies the name: three of the class's methods are co-dependent (alice, bob,
 and mallory) private methods, and we can't modify the class' state
 variables ($foo, $bar, and $baz).

 http://en.wikipedia.org/wiki/Karnaugh_map

 Thus, we would need to check '''385 different states''' in the unit test,
 and '''8 of those are impossible to validate''' due to aliasing in the
 output.

 {{{
 class misery {

   private var $foo;
   private var $bar;
   private var $baz;

   public function ed($arg){

       $this->foo= $arg;
   }

   public function joe(){

       $result = '';

       if($this->foo){
          $result .= $this->foo;
       }

       if($this->bar){
          $result .= $this->$bar;
       }

       if($this->baz){
          $result .= $this->$baz;
       }

       return $result;

   }

   private function alice() {

      $this->foo++;

      if($this->bar >= 5){
         $this->bar = 0;
      }
      else {
        self::bob();
      }
   }

   private function bob(){

      $this->bar++;

      if($this->bar >= 7){
         $this->bar = 0;
      }
      else {
         self::mallory();
      }
   }

   private function mallory(){

      $this->baz++;

      if($this->baz>= 11){
         $this->baz= 0;
      }
   }

 }
 }}}

 Misery is a simple class used to illustrate a point. In the real world,
 the situation is usually *much* worse.

 None of misery's private methods accept arguments, each of which adds
 another degree of freedom to the equation. Throw in some methods that call
 assorted methods from outside classes and don't use dependency injection,
 and you can end up with a class that's literally *impossible* to test.

 In addition to high complexity and long run times, '''test results from a
 class like misery provide little insight into what's causing a problem'''.
 Imagine misery's three private methods were each 500 lines long. The
 results would show you had a problem "somewhere" in those  1,500 lines of
 code. Or something those lines of code interacted with. Good luck finding
 it.

 If I hadn't intervened in this this ticket, I'm pretty sure there would be
 all sorts of misery-like situations in BP within a few months.

 Now, if you simply didn't set those vars and methods as private, the
 entire class could be tested in just nine operations:

 1) Set $foo to 0 and check the state variables
 2) Set $foo to 4 and check the state variables
 3) Set $bar to 0 and check the state variables
 4) Set $bar to 6 and check the state variables
 5) Set $baz to 0 and check the state variables
 6) Set $baz to 10 and check the state variables
 7) Null the sate variables, run $ed, check the state variables
 8) Set all state variables to 0 and run joe()
 9) Set all the state variables to 2 and run joe()

 And these tests would show *precisely* which method isn't working
 properly.

 '''And that's why you don't set your vars and methods to private.'''

 But really, why take our word for it. Did you know WP doesn't use *any*
 private methods or vars in their classes *either*? ...seriously, just
 ctrl-shift-f 'private var' and 'private function' and be enlightened.

 {{{
 **
  * Helper class to remove the need to use eval to replace $matches[] in
 query strings.
  *
  * @since 2.9.0
  */
 class WP_MatchesMapRegex {

         /**
          * store for matches
          *
          * @access private
          * @var array
          */
         var $_matches;

         // ...

         /**
          * do the actual mapping
          *
          * @access private
          * @return string
          */
         function _map() {
                 $callback = array(&$this, 'callback');
                 return preg_replace_callback($this->_pattern, $callback,
 $this->_subject);
         }

 }
 }}}

 Instead, they mark private methods and variables with a '_' prefix
 $_private_var and _private_method(){}

 I'll give you one guess as to why they're doing that:

 http://unit-tests.trac.wordpress.org/browser/trunk/


 The reason things have to be done this way is because PHP lacks two
 important OOP paradigms: Friends,
 http://en.wikipedia.org/wiki/Friend_function and Multi-inheritance,
 http://en.wikipedia.org/wiki/Multi-inheritance.

 If PHP had friends, we could set vars and methods as protected with
 impunity, then add the unit test rig as a 'friend'. Or if PHP had multi-
 inheritance, we could partition a super-class' methods into sub-classes
 containing atomic groups of methods, unit-test the sub-classes, then
 multi-inherit them into a working super-class.

 But PHP has neither of these, so if we start limiting the visibility of
 things, the only thing we're really doing is making our own life
 difficult.

 The purpose of unit tests is to test the smallest individual units of code
 in an app *one at a time*. In all but the simplest of classes, this means
 testing the individual class methods.

 If you make it impossible to test individual methods by making a class'
 vars and methods private, you are no longer unit testing, you're black-box
 testing http://en.wikipedia.org/wiki/Black-box_testing and you do *not*
 want to be taking on that level of complexity for an open-source project.

 -F

-- 
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/4470#comment:7>
BuddyPress <http://buddypress.org/>
BuddyPress