Test enhancement by peter279k · Pull Request #1 · brick/reflection

peter279k · 2018-03-05T06:45:29Z

Changed log

enhance the tests.

BenMorel · 2018-03-05T10:51:32Z

tests/ImportResolverTest.php

+    {
+        $resolver = new ImportResolver(new \ReflectionFunction('Brick\Reflection\Tests\reflectedFunc'));
+
+        $this->assertNull($resolver);


This statement will never be reached, you can remove it (and remove the assignment above).

BenMorel · 2018-03-05T11:03:08Z

tests/ImportResolverTest.php

+    {
+        $resolver = new ImportResolver(new \ReflectionProperty(ReflectionTarget::class, 'foo'));
+
+        $this->assertResolve(ReflectionTarget::class, 'ReflectionTarget');


You're not testing against $resolver here, you need to use assertSame(..., $resolver->resolve(...

You're right. The assertResolve will not be tested the correct test.
I use the assertSame instead of the assertResolve.

BenMorel · 2018-03-05T11:04:17Z

tests/ImportResolverTest.php

+    {
+        $resolver = new ImportResolver(new \ReflectionMethod(ReflectionTarget::class, 'publicStaticMethod'));
+
+        $this->assertResolve('Brick\Reflection\Tests\publicStaticMethod', 'publicStaticMethod');


Same here, the first line is unused.

BenMorel · 2018-03-05T11:13:53Z

tests/ImportResolverTest.php

+            ReflectionTarget::class, 'privateFunc',
+        ], 'str'));
+
+        $this->assertResolve('Brick\Reflection\Tests\privateFunc', 'privateFunc');


BenMorel · 2018-03-05T11:18:28Z

tests/ReflectionToolsTest.php


+    public function testGetReflectionFunction()
+    {
+        $reflectionFunc = function()


You don't have to provide a body, just = function() {}; will be shorter.

BenMorel · 2018-03-05T11:19:51Z

tests/ReflectionToolsTest.php

+        };
+        $functions = (new ReflectionTools)->getReflectionFunction($reflectionFunc);
+
+        $this->assertCount(0, $functions->getParameters());


It would actually be better to check that $function (without s, please) is an instance of ReflectionFunction and that getName() returns the proper value!

BenMorel · 2018-03-05T11:21:25Z

tests/ReflectionToolsTest.php

+    {
+        $functions = (new ReflectionTools)->getFunctionParameterTypes(new \ReflectionFunction('Brick\Reflection\Tests\reflectedFunc'));
+
+        $this->assertCount(0, $functions);


Redundant with the test below it, you can remove this line.

BenMorel · 2018-03-05T11:21:38Z

tests/ReflectionToolsTest.php

+
+    public function testGetFunctionParameterTypesShouldReturnEmptyArray()
+    {
+        $functions = (new ReflectionTools)->getFunctionParameterTypes(new \ReflectionFunction('Brick\Reflection\Tests\reflectedFunc'));


I would call it $types here!

BenMorel · 2018-03-05T11:22:02Z

tests/ReflectionToolsTest.php

+        $functions = (new ReflectionTools)->getFunctionParameterTypes(new \ReflectionFunction('Brick\Reflection\Tests\reflectedParameterFunc'));
+
+        $this->assertCount(1, $functions);
+        $this->assertSame('string', $functions['arg'][0]);


Can you test the actual array returned with assertSame() instead of these 2 lines?

BenMorel · 2018-03-05T11:22:14Z

tests/ReflectionToolsTest.php

+
+    public function testGetFunctionParameterTypesShouldReturnTypesArray()
+    {
+        $functions = (new ReflectionTools)->getFunctionParameterTypes(new \ReflectionFunction('Brick\Reflection\Tests\reflectedParameterFunc'));


Should be called $types as well.

BenMorel · 2018-03-05T11:23:40Z

tests/ReflectionToolsTest.php

+
+    public function testGetParameterTypesShouldReturnTypeArray()
+    {
+        $parameters = (new ReflectionTools)->getParameterTypes(new \ReflectionParameter([


Should be called $types as well.

BenMorel · 2018-03-05T11:24:19Z

tests/ReflectionToolsTest.php

+        ], 'str'));
+
+        $this->assertCount(1, $parameters);
+        $this->assertSame('string', $parameters[0]);


Same here, please replace with assertSame([...], ...

BenMorel · 2018-03-05T11:25:07Z

tests/ReflectionToolsTest.php

+
+    public function testGetPropertyTypesShouldReturnEmptyArray()
+    {
+        $properties = (new ReflectionTools)->getPropertyTypes(new \ReflectionProperty(ReflectionTarget::class, 'foo'));


Should be called $types

BenMorel · 2018-03-05T11:25:30Z

tests/ReflectionToolsTest.php

+    {
+        $properties = (new ReflectionTools)->getPropertyTypes(new \ReflectionProperty(ReflectionTarget::class, 'bar'));
+
+        $this->assertCount(1, $properties);


Should use assertSame()

BenMorel · 2018-03-05T11:25:33Z

tests/ReflectionToolsTest.php

+
+    public function testGetPropertyTypesShouldReturnTypeArray()
+    {
+        $properties = (new ReflectionTools)->getPropertyTypes(new \ReflectionProperty(ReflectionTarget::class, 'bar'));


Should be called $types

BenMorel · 2018-03-05T12:16:57Z

tests/reflectedFunc.php

+/**
+ * The Target Reflection function without parameter.
+ */
+function reflectedFunc()


Can you merge these 2 functions into one file, functions.php?

BenMorel · 2018-03-05T21:21:47Z

tests/ReflectionTarget.php

+    private $bar;
+
+    /**
+     * @var \\Exception $barWithType


Extra \ here

BenMorel · 2018-03-05T21:22:47Z

tests/ReflectionTarget.php

+class ReflectionTarget
+{
+    /**
+     * @param string $foo


Should be @var, and no variable name

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?

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.

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!

BenMorel · 2018-03-05T21:23:18Z

tests/ReflectionTarget.php

+    private $foo;
+
+    /**
+     * @var string $bar


Should be @var string, without variable name

BenMorel · 2018-03-05T21:26:30Z

tests/ReflectionTarget.php

+
+    /**
+     * @param  string $str
+     * @return string $str


Should be @return string, no variable name

BenMorel · 2018-03-05T21:30:30Z

tests/ImportResolverTest.php

+    {
+        $resolver = new ImportResolver(new \ReflectionMethod(ReflectionTarget::class, 'publicStaticMethod'));
+
+        $this->assertSame('Brick\Reflection\Tests\publicStaticMethod', $resolver->resolve('publicStaticMethod'));


This test is actually weird, you're attempting to resolve a class called publicStaticMethod which is very confusing. Let's just call it 'Foo'?

I write the wrong assertions here and it should be ReflectionTarget class.

Ok, FYI it can be any class name, even if it does not exist!

BenMorel · 2018-03-05T21:31:59Z

tests/ImportResolverTest.php

+            ReflectionTarget::class, 'privateFunc',
+        ], 'str'));
+
+        $this->assertSame('Brick\Reflection\Tests\privateFunc', $resolver->resolve('privateFunc'));


Same remark as above, please choose another name like 'Foo' to remove any confusion.

It's same as the publicStaticMethod.

BenMorel · 2018-03-05T21:34:14Z

tests/ReflectionTarget.php

+     */
+    public static function publicStaticMethod()
+    {
+        return 'publicStaticMethod';


You don't need to return anything here. You've declared it @return void, let's remove the body.

BenMorel · 2018-03-05T21:35:20Z

tests/ImportResolverTest.php

+     * @expectedException        \InvalidArgumentException
+     * @expectedExceptionMessage Cannot infer the declaring class from the given ReflectionFunction
+     */
+    public function testConstructorWithReflectedFunctionShouldThrowInvalidArgumentException()


Very long name, please rename to testConstructorWithReflectionFunctionThrowsException

BenMorel · 2018-03-06T12:25:29Z

tests/ReflectionTarget.php

+    }
+
+    /**
+     * @param  string


Now this should be @param string $str!

/** * @param string $str * * @return string */

@param mentions the parameter name
@return does not

peter279k added 4 commits March 5, 2018 14:07

enhance ReflectionTool test

1bde7a1

complete ImportResolver test

506bdf8

set the correct autoload-dev files

c206ca6

add EOF for composer.json

14625aa

BenMorel reviewed Mar 5, 2018

View reviewed changes

approve request changes, merge the two functions

978255d

BenMorel reviewed Mar 5, 2018

View reviewed changes

tests/ReflectionTarget.php Outdated

private $bar;

/**

* @var \\Exception $barWithType

Copy link

Member

BenMorel Mar 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra \ here

BenMorel reviewed Mar 5, 2018

View reviewed changes

tests/ReflectionTarget.php

private $foo;

/**

* @var string $bar

Copy link

Member

BenMorel Mar 5, 2018 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be @var string, without variable name

BenMorel reviewed Mar 5, 2018

View reviewed changes

fix the assertions and test method name

6583e51

BenMorel reviewed Mar 6, 2018

View reviewed changes

peter279k added 3 commits March 18, 2018 00:42

Merge https://github.com/brick/reflection

acbbd21

fix the typo doc block

91cab56

fix the assertions value

5a11f9a

BenMorel force-pushed the master branch 2 times, most recently from 65ad42b to 9d2f509 Compare June 25, 2022 23:03

Conversation

peter279k commented Mar 5, 2018

Changed log

Uh oh!

BenMorel Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenMorel Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peter279k Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenMorel Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenMorel Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

BenMorel Mar 5, 2018 •

edited

Loading

BenMorel Mar 5, 2018 •

edited

Loading

peter279k Mar 6, 2018 •

edited

Loading

BenMorel Mar 5, 2018 •

edited

Loading

BenMorel Mar 5, 2018 •

edited

Loading