Cleanup: move to correct phpunit assertions #111
Conversation
|
Should we also change |
|
@lcobucci good point - will apply that change too :-) |
| @@ -9,5 +9,5 @@ | |||
| /** | |||
| * @Secure | |||
| */ | |||
| function foo(); | |||
| public function foo(); | |||
| } | |||
There was a problem hiding this comment.
Should we have the newline at the end?
| @@ -653,15 +653,15 @@ public function testAnnotationWithRequiredAttributes() | |||
|
|
|||
| $docblock = '@Doctrine\Tests\Common\Annotations\Fixtures\AnnotationWithRequiredAttributes("Some Value")'; | |||
| try { | |||
| $result = $parser->parse($docblock,$context); | |||
| $parser->parse($docblock,$context); | |||
There was a problem hiding this comment.
Shouldn't we have a space after the comma?
| $this->fail(); | ||
| } catch (AnnotationException $exc) { | ||
| $this->assertContains('Attribute "annot" of @Doctrine\Tests\Common\Annotations\Fixtures\AnnotationWithRequiredAttributes declared on property SomeClassName::invalidProperty. expects a(n) Doctrine\Tests\Common\Annotations\Fixtures\AnnotationTargetAnnotation. This value should not be null.', $exc->getMessage()); | ||
| } | ||
|
|
||
| $docblock = '@Doctrine\Tests\Common\Annotations\Fixtures\AnnotationWithRequiredAttributes(annot = @Doctrine\Tests\Common\Annotations\Fixtures\AnnotationTargetAnnotation)'; | ||
| try { | ||
| $result = $parser->parse($docblock,$context); | ||
| $parser->parse($docblock,$context); |
There was a problem hiding this comment.
Shouldn't we have a space after the comma?
There was a problem hiding this comment.
I didn't apply any CS fixes
| @@ -687,15 +687,15 @@ public function testAnnotationWithRequiredAttributesWithoutConstructor() | |||
|
|
|||
| $docblock = '@Doctrine\Tests\Common\Annotations\Fixtures\AnnotationWithRequiredAttributesWithoutConstructor("Some Value")'; | |||
| try { | |||
| $result = $parser->parse($docblock,$context); | |||
| $parser->parse($docblock,$context); | |||
There was a problem hiding this comment.
Shouldn't we have a space after the comma?
| $this->fail(); | ||
| } catch (AnnotationException $exc) { | ||
| $this->assertContains('Attribute "annot" of @Doctrine\Tests\Common\Annotations\Fixtures\AnnotationWithRequiredAttributesWithoutConstructor declared on property SomeClassName::invalidProperty. expects a(n) \Doctrine\Tests\Common\Annotations\Fixtures\AnnotationTargetAnnotation. This value should not be null.', $exc->getMessage()); | ||
| } | ||
|
|
||
| $docblock = '@Doctrine\Tests\Common\Annotations\Fixtures\AnnotationWithRequiredAttributesWithoutConstructor(annot = @Doctrine\Tests\Common\Annotations\Fixtures\AnnotationTargetAnnotation)'; | ||
| try { | ||
| $result = $parser->parse($docblock,$context); | ||
| $parser->parse($docblock,$context); |
There was a problem hiding this comment.
Shouldn't we have a space after the comma?
As per @lcobucci's review
|
@lcobucci if you think it's good to go, go on and 🚢 |
| $this->assertCount(1, $reader->getClassAnnotations($class)); | ||
| $this->assertInstanceOf($annotName = DummyAnnotation::class, $annot = $reader->getClassAnnotation($class, $annotName)); | ||
| $this->assertEquals('hello', $annot->dummyValue); | ||
| self::assertCount(1, $reader->getClassAnnotations($class)); |
There was a problem hiding this comment.
Can you point to the reasoning for this change or explain it ?
There was a problem hiding this comment.
Yes it is (as far as I know) that why I want to know why you @lcobucci preferer this version.
There was a problem hiding this comment.
Better semantics and error messages, usually
There was a problem hiding this comment.
Wait I was talking about the change from $this->assertCount to $self::assertCount not the change from assertTrue( , count()) to assertCount(). Or it's the same answer ?
| @@ -192,7 +192,7 @@ private function updateFieldAceProperty($name, array $changes) | |||
|
|
|||
| $aceIdProperty = new \ReflectionProperty('Symfony\Component\Security\Acl\Domain\Entry', 'id'); | |||
| $aceIdProperty->setAccessible(true); | |||
| $aceIdProperty->setValue($ace, intval($aceId)); | |||
| $aceIdProperty->setValue($ace, (int) $aceId); | |||
There was a problem hiding this comment.
What is the reason for the preference ?
There was a problem hiding this comment.
Intval is generally to be replaced with simpler to look up casts
|
Also how am I suppose to validate mark the review as being positive (without requiring change)? I can't find any button 😢 . |
|
Only while the PR is still open |
|
Thanks 👍 anyway |
|
Oh, that's just because static methods should be invoked statically,
nothing else
…On 1 Jan 2017 22:29, "mikeSimonson" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tests/Doctrine/Tests/Common/Annotations/AbstractReaderTest.php
<#111>:
> @@ -20,90 +20,90 @@ public function testAnnotations()
$class = $this->getReflectionClass();
$reader = $this->getReader();
- $this->assertCount(1, $reader->getClassAnnotations($class));
- $this->assertInstanceOf($annotName = DummyAnnotation::class, $annot = $reader->getClassAnnotation($class, $annotName));
- $this->assertEquals('hello', $annot->dummyValue);
+ self::assertCount(1, $reader->getClassAnnotations($class));
Wait I was talking about the change from $this->assertCount to
$self::assertCount not the change from assertTrue( , count()) to
assertCount(). Or it's the same answer ?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#111>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJakMrkJeiOTvVGs63RpX0-C3l7YKizks5rOBrPgaJpZM4LYOIl>
.
|
As discussed in #110