PHPUnit

Some feedback on 20120416_1819_patch_36107.diff:

Very nice unit tests! Good work!

===================================================================
--- trunk/Tests/Unit/Fixtures/ProtectedClass.php    (revision )
+++ trunk/Tests/Unit/Fixtures/ProtectedClass.php    (revision )
@@ -0,0 +1,35 @@
+<?php
+/**
+ * Test class.
+ */
+class Tx_Phpunit_Tests_Fixtures_ProtectedClass {
+
+    /**
+     * @var integer $propertyOne
+     */
+    protected $propertyOne = 1;

Generally, please do not put empty lines below an opening brace. Please change this in all files added by this patch.

+    /**
+     * Protected test function which returns true when processed.
+     *
+     * @return boolean
+     */
+    protected function methodOne() {
+        return TRUE;
+    }
+
+    /**
+     * Public test function which returns true when processed.
+     *
+     * @return boolean
+     */
+    public function methodTwo() {
+        return TRUE;
+    }

Please always uppercase TRUE in comments as well.

===================================================================
--- trunk/Classes/TestCase.php    (revision 60750)
+++ trunk/Classes/TestCase.php    (revision )

For changes in this size, please add yourself as @author to the class documentation comments (also in the test case test).

@@ -50,5 +50,72 @@
      * @var boolean
      */
     protected $backupStaticAttributes = FALSE;
+
+    /**
+     * Returns a mock object which allows for calling protected methods and access of protected properties.
+     *
+     * @param string $originalClassName
+     * @param array $methods
+     * @param array $arguments
+     * @param string $mockClassName
+     * @param boolean $callOriginalConstructor
+     * @param boolean $callOriginalClone
+     * @param boolean $callAutoload
+
+     * @return PHPUnit_Framework_MockObject_MockObject
+     */
+    public function getAccessibleMock($originalClassName, $methods = array(), array $arguments = array(), $mockClassName = '', $callOriginalConstructor = TRUE, $callOriginalClone = TRUE, $callAutoload = TRUE) {
+        $AccessibleMockObject = $this->getMock($this->buildAccessibleProxy($originalClassName), $methods, $arguments, $mockClassName, $callOriginalConstructor, $callOriginalClone, $callAutoload);
+
+        return $AccessibleMockObject;
+    }

Please make sure that all changed or added lines are below 130 characters (the current limit in the TYPO3 coding guidelines).

Please add the type hinting for $methods.

In the description: Returns -> Creates

Please add (short) descriptions to the @param and @return.

In the @param, please add the types within the arrays: array -> array<string>

+    public function getAccessibleMock($originalClassName, $methods = array(), array $arguments = array(), $mockClassName = '', $callOriginalConstructor = TRUE, $callOriginalClone = TRUE, $callAutoload = TRUE) {
+        $AccessibleMockObject = $this->getMock($this->buildAccessibleProxy($originalClassName), $methods, $arguments, $mockClassName, $callOriginalConstructor, $callOriginalClone, $callAutoload);
+
+        return $AccessibleMockObject;
+    }

Please always use lowerCamelCase for method and variable names: AccessibleMockObject -> accessibleMockObject

You can inline $AccessibleMockObject (i.e., directly return it instead of putting it in a local variable).

+    }
+
+
+    /**
+     * Creates a proxy class of the specified class which allows for calling even protected methods and access of protected properties.
+     *
+     * @param $className
+     *
+     * @return string Full qualified name of the built class
+     */
+    protected function buildAccessibleProxy($className) {

Generally, please use only one blank line between functions, not two.

Please add the type (string) and some short description to the @param.

Please document that the return value will not be empty.

Please lowercase the "Full" as this is no full sentence.

+    protected function buildAccessibleProxy($className) {
+        $accessibleClassName = uniqid('Tx_Phpunit_AccessibleProxyTestCase');

Why the "TestCase" suffix here? I think it should just be an ...AccessibleProxy.

+        eval('
+            ' . $abstractModifier . 'class ' . $accessibleClassName . ' extends ' . $className . ' {
+                public function _call($methodName) {
+                    $args = func_get_args();
+                    return call_user_func_array(array($this, $methodName), array_slice($args, 1));
+                }
+                public function _callRef($methodName, &$arg1 = NULL, &$arg2 = NULL, &$arg3 = NULL, &$arg4 = NULL, &$arg5= NULL, &$arg6 = NULL, &$arg7 = NULL, &$arg8 = NULL, &$arg9 = NULL) {
+                    switch (func_num_args()) {
+                        case 0 : return $this->$methodName();
+                        case 1 : return $this->$methodName($arg1);
+                        case 2 : return $this->$methodName($arg1, $arg2);
+                        case 3 : return $this->$methodName($arg1, $arg2, $arg3);
+                        case 4 : return $this->$methodName($arg1, $arg2, $arg3, $arg4);
+                        case 5 : return $this->$methodName($arg1, $arg2, $arg3, $arg4, $arg5);
+                        case 6 : return $this->$methodName($arg1, $arg2, $arg3, $arg4, $arg5, $arg6);
+                        case 7 : return $this->$methodName($arg1, $arg2, $arg3, $arg4, $arg5, $arg6, $arg7);
+                        case 8 : return $this->$methodName($arg1, $arg2, $arg3, $arg4, $arg5, $arg6, $arg7, $arg8);
+                        case 9 : return $this->$methodName($arg1, $arg2, $arg3, $arg4, $arg5, $arg6, $arg7, $arg8, $arg9);
+                        default : throw new InvalidArgumentException(\'The accessibleProxy of class ' . $className . ' does currently not support methods with more than ten arguments.\', 1334536392);
+                    }
+                }
+                public function _set($propertyName, $value) {
+                    $this->$propertyName = $value;
+                }
+                public function _setRef($propertyName, &$value) {
+                    $this->$propertyName = $value;
+                }
+                public function _get($propertyName) {
+                    return $this->$propertyName;
+                }
+            }
+        ');
+

For the evaled code, please use single lines concatenated by dots instead of multiline strings.

Please add the PhpDoc for the methods.

Please drop the space before the : in the cases.

Please fix the line lengths to be below 130 characters.

Instead of the early returns, please put the return value in a local variable and return it at the end of the _callRef function. (We'll need break statements then.)

Please document that buildAccessibleProxy works exactly like that function in the extbase base test case.

Please also add unit tests for the number of arguments and the exception.

I think InvalidArgumentExceptions when $methodName is === '' or when $propertyName is === '' would be very helpful, too.

===================================================================
--- trunk/Tests/Unit/TestCaseTest.php    (revision 60750)
+++ trunk/Tests/Unit/TestCaseTest.php    (revision )
 class Tx_Phpunit_TestCaseTest extends Tx_Phpunit_TestCase {
+
     /**
+     * @var Tx_Phpunit_Tests_Fixtures_ProtectedClass $fixture
+     */
+    private $fixture;
+
+    /**
+     * @var Tx_Phpunit_BackEnd_Module|PHPUnit_Framework_MockObject_MockObject $mock
+     */
+    private $mock;
+
+    /**
+     * @var Tx_Phpunit_BackEnd_Module|PHPUnit_Framework_MockObject_MockObject $accessibleMock
+     */
+    private $accessibleMock;

Just to make the values explicit, please give these fields NULL as default value.

+    public function setUp() {
+        $this->fixture = new Tx_Phpunit_Tests_Fixtures_ProtectedClass;
+        $this->mock = $this->getMock(
+            'Tx_Phpunit_Tests_Fixtures_ProtectedClass', array('dummy'), array(), '', FALSE
+        );
+        $this->accessibleMock = $this->getAccessibleMock(
+            'Tx_Phpunit_Tests_Fixtures_ProtectedClass', array('dummy'), array(), '', FALSE
+        );
+    }
+

I think the last three parameters are not needed for these two calls as we do not need to skip the constructor if I'm not mistaken.

+    /**
+     * @test
+     */
+    public function accessProtectedPropertyForClassReturnsFalse() {
+        $this->assertFalse(in_array('propertyOne', get_object_vars($this->fixture)));
+    }
+

I don't get the point of this test. What is the exact behavior which you're testing here?

If it is whether a protected property is accessible, I'd name the corresponding tests something like "protectedPropertyIsNotDirectlyAccessible" (as this is not about a particular function or a particular return value). Ditto for the other similar test functions.

Generally, for assertFalse, assertTrue etc., please put the checked value on a separate line.

+    /**
+     * @test
+     */
+    public function getPublicPropertyForClassGetsPublicProperty() {
+        $this->assertSame(2, $this->fixture->propertyTwo);
+    }

Generally, please put the expected value and the actual value on a separate line each.

I'd prefer the properties to be strings with texts like "This is a public property." and have names like "publicProperty" and "protectedProperty" so the intent is clearer.

I think it would be great to also add an interface that contains the methods added via eval. This will allow better code inspection and code completion.

What do you think?

Nicole, would you like to take care of porting this back to extbase (including the new interface and the bug fix), or do you want me to do it?

So this is than extbase related and has to change in the Git/Gerrit repository of extbase, right? Shall I open a new ticket for that?

Nicole Cordes wrote:

So this is than extbase related and has to change in the Git/Gerrit repository of extbase, right? Shall I open a new ticket for that?

Yes, this would be extbase-related and needs a separate ticket, a separate patch and a push to Gerrit. If you could do this, this would be awesome. (IIRC, you'll still need to sign the CLA and e-mail it to info at typo3 dot org to be able to push extbase patches.)