NYCPHP Meetup

[Robert Stoll]

I am glad you coming back to me with this, I find it to be a very interesting topic

> -----Original Message-----
> From: talk-bounces at lists.nyphp.org [mailto:talk-bounces at lists.nyphp.org] On Behalf Of Gary A. Mort
> Sent: Friday, November 22, 2013 8:46 PM
> To: NYPHP Talk
> Subject: Re: [nycphp-talk] Why do unit tests not inherit?
> 
> Thanks Robert...  I may be misunderstanding something here:
> 
> On 11/15/2013 12:57 PM, Robert Stoll wrote:
> > I am not sure if we talk about the same. Just to avoid misunderstands I am going to outline a little bit more what I
> > meant. I did not mean that each method of a class has to have its one test class. But each method of a class A
should
> > have an own test method in the test class T. And if the method of class A has branches, let's say one if-statement,
then
> > the ideal case would be that you create two test methods in C which covers both cases. Once for the case that the
> > if-condition evaluates to true and once to false.
> > For example:
> >
> > class A{
> >      private $_isActive=false;
> >      function isActive(){
> >          return $this->_isActive;
> >      }
> >      function foo(){
> >          $this->_isActive=true;
> >      }
> >
> >      function bar(){
> >          if($isActive){
> >              doesThis();
> >          } else{
> >              doesThat();
> >          }
> >      }
> > }
> >
> > class T extends SomeTestFramework{
> >      public function testFoo_Standard_IsActiveSetToTrue (){
> >          // arrange
> >          // act
> >          // assert
> >      }
> >      public function testBar_IsActiveIsTrue_DoesThis(){}
> 
> I am assuming that there is no unit testing magic which is setting
> things, so this method would actually be:
> 
> public function testBar_IsActiveIsTrue_DoesThis(){
> 
> // create an object $testObject of class A
> // call $testObject->foo() to make it active
> // Test that $testObject->isActive returns true
> // Test that $testObject->bar executes doesThis()
> 
> }
> 
> 
> public function testBar_IsActiveIsFalse_DoesThat(){
> 
> // create an object $testObject of class A
> // Test that $testObject->isActive returns false
> // Test that $testObject->bar executes doesThat()
> 
> }

[Robert Stoll] 
That right, that's how my tests would look like more or less with the slight difference that I would not test if
isActive is true or false (I would cover it in another test case) but it's ok to test that as well.

> 
> It's with the above commented steps that I have an issue.  Primarily
> because in practice, if someone creates:
> 
> Class APrime extend A{}
> 
> Then they also create
> 
> class TPrime extends SomeTestFramework{
> 
>      function bar(){
>          if($isActive){
>              doesThis();
>          } else{
>              doesNOTDOThat();
>          }
> 
> }

[Robert Stoll] 
I guess you made a mistake, the new behaviour should be in APrime (as in the next paragraph) and not in TPrime:
class APrime extend A{
    function bar(){
        if($isActive){
            doesThis();
        } else{
            doesNOTDOThat();
        }
}

> In in TPrime will be:
> 
> 
> public function testBar_IsActiveIsTrue_DoesThis(){
> 
> // create an object $testObject of class APrime
> // call $testObject->foo() to make it active
> // Test that $testObject->isActive returns true
> // Test that $testObject->bar executes doesThis()
> 
> }
> 
> 
> public function testBar_IsActiveIsFalse_DoesNOTDOThat(){
> 
> // create an object $testObject of class APrime
> // Test that $testObject->isActive returns false
> // Test that $testObject->bar executes doesNOTDOThat()
> 
> }
> 
> So everything has been cut and pasted from one to the other.  The only
> difference is that APrime calls doesNOTDOThat instead of doesThat.
> Testing items where taken from one to the other, with minor editing
> changes to half of the new tests to change doesThat to doesNOTDOThat

[Robert Stoll] 
I agree, but first of all the question arises, does APrime not already break the Liskov Substitution Principle by
invoking a different method (doesNOTDOThat instead of doesThat)? 
It does not break the principle if doesNOTDOThat has the same effect as doesThat (and probably  additional side effects
which are only part of the sub-class). But it breaks the Liskov Substitution Principle if the side effects which concern
the class A are different. 
If it does not break the principle then I would reconsider the design. Why not overriding doesThat and leave the method
"bar" as it is? Shouldn't the additional behaviour of doesNOTDOThat also come into play when one is invoking doesThat
from another place?

But let's assume the principle was either broken or it wasn't and doesThat is fine as it is and thus the overriding of
"bar" was correct (seems very unlikely to me though). 
Then I would just create "public function testBar_IsActiveIsFalse_DoesNOTDOThat()" in TPrime. Let's call TPrime
APrimeTest from now on, I am sorry for the bad naming I used in the first mail. It should have been class "A" and the
corresponding test class "ATest"

Back to the problem. In addition, I would create another test class which I usually name after the schema
SubClass_ClassTest which would be APrime_ATest which extends ATest and is used to verify the Liskov Substitution
Principle or/and to state where the sub-class actually breaks the principle or behaves differently

In this case I would override the createA method which provides me the A object (unfortunately I forgot to state this
method in my earlier mail) and override the test methods which are no longer needed (since either the corresponding
method under test breaks the Liskov Substitution Principle or is implemented differently). I guess it will be clearer if
I state the code again (I used the better naming now - ATest was T above and APrimeTest was TPrime):

class ATest extends SomeTestFramework{  
    public function testFoo_Standard_IsActiveSetToTrue (){
        //same as before
    }
    public function testBar_IsActiveIsTrue_DoesThis(){
        //same as before
    }
    public function testBar_IsActiveIsFalse_DoesThat(){
        //same as before
    }

    //if the has no parameters as in this case then you could also consider to use the setUp method instead
    protected function createA(){
        return new A();
    }
}

class APrimeTest extends SomeTestFramework{
    public function testBar_IsActiveIsFalse_DoesNOTDOThat(){
        // create an object $testObject of class APrime
        // Test that $testObject->isActive returns false
        // Test that $testObject->bar executes doesNOTDOThat()
    }
}

class APrime_ATest extends ATest{
    public function testBar_IsActiveIsFalse_DoesThat(){
        // Either I would write a comment here why the sub-class breaks the Liskov Substitution principle 
        // or state why it has a different behaviour and which test methods cover it
        // Either way, due to the fact that I override the method without testing anything
        // the test will pass which is fine because I cover the case somewhere else
    }
    protected function createA(){
        return new APrime();
    }
}

This way I can reduce the code duplication but still guarantee that the Liskov Substitution Principle holds where
desired.

> Now to extend this further, let's say a year later someone goes in and
> modifies class A:
> 
> class A{
>      private $_isActive=false;
> 
>       public function __construct($active = false) {
>           $this->$_isActive = $active;
>      }
> 
> 
> Everything works pretty well for a while because no one is actually
> USING the new parameter added to object construction.  Months pass:
> 
> 
> Then the class needs to have THREE states, and is active is easily
> modified for that:
> 
>   function bar(){
>          if($isActive === true){
>              doesThis();
>          } else if ($isActive === false) {
>              doesThat();
>          } else {
> 	    doesSomethingEntirelyDifferent();
> 	}
>   }
> 
> 
> The testing class is updated with the appropriate:
> 
> public function testBar_IsActiveThirdState_DoesSomethingEntirelyDeffierentThat(){
> 
> // create an object $testObject of class A using $testObject = new A(3);
> // Test that $testObject->isActive returns 3
> // Test that $testObject->bar executes doesSomethingEntirelyDifferent()
> 
> }
> 
> And here is where the maintenance fun begins!
> 
> Class APrime was forgotten about - so it wasn't updated to use the new
> tristate logic.  APrime still passes all the tests associated with it,
> so from the Unit Tests all looks good.
> 
> The very reason we wrote unit tests, to discover when changes break
> compatibility, and because we just copy and paste tests from one test
> class to the next, the tests don't tell us all that they should and could.
> 
> 
> The more I think about it, the more I think that inheritance is not the
> answer because we just would have overwritten some of the tests anyway
> and would still end up failing.
> 

[Robert Stoll] 
If you follow my approach then you would be on the safe side because a new method in ATest will also be executed in
APrime_ATest.

> Ideally, what we should be doing is:
> When testing a subclass, after running the tests for the subclass, we
> should also load the tests for the parent class and run them. Somehow we
> need to add a blacklist of tests not to run, ie:
> For APrime we want to run:
> TPrime::testBar_IsActiveIsTrue_DoesThis,
> TPrime::testBar_IsActiveIsTrue_DoesNOTDOTHAT
> and run
> all tests from class T EXCEPT FOR TPrime::testBar_IsActiveIsTrue_DoesThat
> 
>  From a test report perspective, I want to know that:
> Class APrime is sane
> Class APrime is also a sane subclass of class A
> 
> If tests are run this way, then when the modifications I've seen happen
> such as the above are done, our unit tests will come back and say:
> Class APrime is sane
> Class APrime is not a sane subclass of class A
> 
> Because it will fail the newly added and not excluded test that
> testBar_IsActiveThirdState_DoesSomethingEntirelyDeffierentThat

[Robert Stoll] 
That's actually exactly what I try to accomplish with my approach. Nevertheless, one thing you have to consider is that
unit-test as such are not enough. Especially for cases where you want to be able to use different classes for the same
purpose. You will need integration tests which ensure that A and APrime behave in the correct way when used in
combination with other classes. And since those tests often catch errors like a break of the Liskov Substitution
Principle I actually seldom write test classes as APrime_ATest. But that's mainly because of time constraints. 

I'll try to write more tests like that though because it makes you think about the Liskov Substitution Principle in your
code design. For instance, I had lately the case that a sub-class overrode most of the methods (all important once in
fact) and I realised it because I was thinking about if I need a test class such as APrime_ATest. Finally, I removed the
extends because it violated the substitution principle too often.