Test enhancement #1
Conversation
tests/ImportResolverTest.php
Outdated
| { | ||
| $resolver = new ImportResolver(new \ReflectionFunction('Brick\Reflection\Tests\reflectedFunc')); | ||
|
|
||
| $this->assertNull($resolver); |
There was a problem hiding this comment.
This statement will never be reached, you can remove it (and remove the assignment above).
tests/ImportResolverTest.php
Outdated
| { | ||
| $resolver = new ImportResolver(new \ReflectionProperty(ReflectionTarget::class, 'foo')); | ||
|
|
||
| $this->assertResolve(ReflectionTarget::class, 'ReflectionTarget'); |
There was a problem hiding this comment.
You're not testing against $resolver here, you need to use assertSame(..., $resolver->resolve(...
There was a problem hiding this comment.
You're right. The assertResolve will not be tested the correct test.
I use the assertSame instead of the assertResolve.
tests/ImportResolverTest.php
Outdated
| { | ||
| $resolver = new ImportResolver(new \ReflectionMethod(ReflectionTarget::class, 'publicStaticMethod')); | ||
|
|
||
| $this->assertResolve('Brick\Reflection\Tests\publicStaticMethod', 'publicStaticMethod'); |
There was a problem hiding this comment.
Same here, the first line is unused.
tests/ImportResolverTest.php
Outdated
| ReflectionTarget::class, 'privateFunc', | ||
| ], 'str')); | ||
|
|
||
| $this->assertResolve('Brick\Reflection\Tests\privateFunc', 'privateFunc'); |
tests/ReflectionToolsTest.php
Outdated
| @@ -19,6 +19,85 @@ public function testGetMethodsDoesNotReturnStaticMethods() | |||
| $this->assertCount(0, $methods); | |||
| } | |||
|
|
|||
| public function testGetReflectionFunction() | |||
| { | |||
| $reflectionFunc = function() | |||
There was a problem hiding this comment.
You don't have to provide a body, just = function() {}; will be shorter.
tests/ReflectionToolsTest.php
Outdated
| }; | ||
| $functions = (new ReflectionTools)->getReflectionFunction($reflectionFunc); | ||
|
|
||
| $this->assertCount(0, $functions->getParameters()); |
There was a problem hiding this comment.
It would actually be better to check that $function (without s, please) is an instance of ReflectionFunction and that getName() returns the proper value!
tests/ReflectionToolsTest.php
Outdated
| { | ||
| $functions = (new ReflectionTools)->getFunctionParameterTypes(new \ReflectionFunction('Brick\Reflection\Tests\reflectedFunc')); | ||
|
|
||
| $this->assertCount(0, $functions); |
There was a problem hiding this comment.
Redundant with the test below it, you can remove this line.
tests/ReflectionToolsTest.php
Outdated
|
|
||
| public function testGetFunctionParameterTypesShouldReturnEmptyArray() | ||
| { | ||
| $functions = (new ReflectionTools)->getFunctionParameterTypes(new \ReflectionFunction('Brick\Reflection\Tests\reflectedFunc')); |
tests/ReflectionToolsTest.php
Outdated
| $functions = (new ReflectionTools)->getFunctionParameterTypes(new \ReflectionFunction('Brick\Reflection\Tests\reflectedParameterFunc')); | ||
|
|
||
| $this->assertCount(1, $functions); | ||
| $this->assertSame('string', $functions['arg'][0]); |
There was a problem hiding this comment.
Can you test the actual array returned with assertSame() instead of these 2 lines?
tests/ReflectionToolsTest.php
Outdated
|
|
||
| public function testGetFunctionParameterTypesShouldReturnTypesArray() | ||
| { | ||
| $functions = (new ReflectionTools)->getFunctionParameterTypes(new \ReflectionFunction('Brick\Reflection\Tests\reflectedParameterFunc')); |
There was a problem hiding this comment.
Should be called $types as well.
tests/ReflectionToolsTest.php
Outdated
|
|
||
| public function testGetParameterTypesShouldReturnTypeArray() | ||
| { | ||
| $parameters = (new ReflectionTools)->getParameterTypes(new \ReflectionParameter([ |
There was a problem hiding this comment.
Should be called $types as well.
tests/ReflectionToolsTest.php
Outdated
| ], 'str')); | ||
|
|
||
| $this->assertCount(1, $parameters); | ||
| $this->assertSame('string', $parameters[0]); |
There was a problem hiding this comment.
Same here, please replace with assertSame([...], ...
tests/ReflectionToolsTest.php
Outdated
|
|
||
| public function testGetPropertyTypesShouldReturnEmptyArray() | ||
| { | ||
| $properties = (new ReflectionTools)->getPropertyTypes(new \ReflectionProperty(ReflectionTarget::class, 'foo')); |
tests/ReflectionToolsTest.php
Outdated
| { | ||
| $properties = (new ReflectionTools)->getPropertyTypes(new \ReflectionProperty(ReflectionTarget::class, 'bar')); | ||
|
|
||
| $this->assertCount(1, $properties); |
tests/ReflectionToolsTest.php
Outdated
|
|
||
| public function testGetPropertyTypesShouldReturnTypeArray() | ||
| { | ||
| $properties = (new ReflectionTools)->getPropertyTypes(new \ReflectionProperty(ReflectionTarget::class, 'bar')); |
tests/reflectedFunc.php
Outdated
| /** | ||
| * The Target Reflection function without parameter. | ||
| */ | ||
| function reflectedFunc() |
There was a problem hiding this comment.
Can you merge these 2 functions into one file, functions.php?
tests/ReflectionTarget.php
Outdated
| private $bar; | ||
|
|
||
| /** | ||
| * @var \\Exception $barWithType |
tests/ReflectionTarget.php
Outdated
| class ReflectionTarget | ||
| { | ||
| /** | ||
| * @param string $foo |
There was a problem hiding this comment.
Should be @var, and no variable name
There was a problem hiding this comment.
Ah, just noticed that you're actually testing that it returns no types. Anyway, I don't think it's a good idea to put an invalid docblock in here. Maybe the best thing to do is to remove the docblock altogether?
There was a problem hiding this comment.
I think it's not the good way to remove the $foo docblock bcause it will output the following error message.
1) Brick\Reflection\Tests\ReflectionToolsTest::testGetPropertyTypesShouldReturnEmptyArray
TypeError: preg_match() expects parameter 2 to be string, boolean given
/home/lee/reflection/src/ReflectionTools.php:174
/home/lee/reflection/tests/ReflectionToolsTest.php:56
It will be failed when excuting the preg_match function.
I think we just keep the invalid docblock to test whether it returns the no types.
There was a problem hiding this comment.
This was a bug in ReflectionTools, I never realized that getDocComment() could return false! I just fixed it, and reported the documentation mistake on php.net.
You can now safely remove the docblock in the test!
| private $foo; | ||
|
|
||
| /** | ||
| * @var string $bar |
There was a problem hiding this comment.
Should be @var string, without variable name
tests/ReflectionTarget.php
Outdated
|
|
||
| /** | ||
| * @param string $str | ||
| * @return string $str |
There was a problem hiding this comment.
Should be @return string, no variable name
tests/ImportResolverTest.php
Outdated
| { | ||
| $resolver = new ImportResolver(new \ReflectionMethod(ReflectionTarget::class, 'publicStaticMethod')); | ||
|
|
||
| $this->assertSame('Brick\Reflection\Tests\publicStaticMethod', $resolver->resolve('publicStaticMethod')); |
There was a problem hiding this comment.
This test is actually weird, you're attempting to resolve a class called publicStaticMethod which is very confusing. Let's just call it 'Foo'?
There was a problem hiding this comment.
I write the wrong assertions here and it should be ReflectionTarget class.
There was a problem hiding this comment.
Ok, FYI it can be any class name, even if it does not exist!
tests/ImportResolverTest.php
Outdated
| ReflectionTarget::class, 'privateFunc', | ||
| ], 'str')); | ||
|
|
||
| $this->assertSame('Brick\Reflection\Tests\privateFunc', $resolver->resolve('privateFunc')); |
There was a problem hiding this comment.
Same remark as above, please choose another name like 'Foo' to remove any confusion.
There was a problem hiding this comment.
It's same as the publicStaticMethod.
tests/ReflectionTarget.php
Outdated
| */ | ||
| public static function publicStaticMethod() | ||
| { | ||
| return 'publicStaticMethod'; |
There was a problem hiding this comment.
You don't need to return anything here. You've declared it @return void, let's remove the body.
tests/ImportResolverTest.php
Outdated
| * @expectedException \InvalidArgumentException | ||
| * @expectedExceptionMessage Cannot infer the declaring class from the given ReflectionFunction | ||
| */ | ||
| public function testConstructorWithReflectedFunctionShouldThrowInvalidArgumentException() |
There was a problem hiding this comment.
Very long name, please rename to testConstructorWithReflectionFunctionThrowsException
tests/ReflectionTarget.php
Outdated
| } | ||
|
|
||
| /** | ||
| * @param string |
There was a problem hiding this comment.
Now this should be @param string $str!
/**
* @param string $str
*
* @return string
*/@param mentions the parameter name
@return does not
BenMorel
force-pushed
the
master
branch
2 times, most recently
from
65ad42b to
9d2f509
Compare
Changed log