setIgnoreNotImportedAnnotations(true) doesn't work on already autoloaded classes
#110
Conversation
Because FuelPHP has a Package class, which isn’t an @annotation the following error occurs: > [Semantical Error] The class "package" is not annotated with @annotation. Are you sure this class can be used as annotation? If so, then you need to add @annotation to the _class_ doc comment of "package". If it is indeed no annotation, then you need to add @IgnoreAnnotation("package") to the _class_ doc comment of ... Using @IgnoreAnnotation("package") won’t help because this issue also affects the AnnotationReader->preParser, so this error also occurs when trying to extract the @IgnoreAnnotations
|
@bfanger Thanks for the PR. Someone still needs to verify that the logic of this change is sound. |
| @@ -387,8 +387,11 @@ public function testInvalidAnnotationUsageButIgnoredClass() | |||
| $reader = $this->getReader(); | |||
| $ref = new \ReflectionClass('Doctrine\Tests\Common\Annotations\Fixtures\InvalidAnnotationUsageButIgnoredClass'); | |||
| $annots = $reader->getClassAnnotations($ref); | |||
|
|
|||
| $this->assertEquals(2, count($annots)); | |||
| if ($annots[0] instanceof IgnoreAnnotation) { | |||
There was a problem hiding this comment.
Can you split that test is 2 distinct ones.
The "if" here indicate that you are testing 2 different behaviours in the same test.
There was a problem hiding this comment.
I have removed if statement from the unittest and moved testing the behaviour of SimpleAnnotationReader into the SimpleAnnotationReaderTest
|
|
||
| $this->assertEquals(2, count($annots)); | ||
| if ($annots[0] instanceof IgnoreAnnotation) { | ||
| $this->assertEquals(2, count($annots)); |
There was a problem hiding this comment.
Please use assertCount everywhere you count in the assertion.
There was a problem hiding this comment.
grep -r "assertCount" tests/|wc -l = 13
grep -r "assertEquals" tests/|grep "count("|wc -l = 51
This codebase clearly prefers assertEquals over assertCount.
There was a problem hiding this comment.
You realise that this justification is weak right ?
With that kind of logic why not using assertTrue everywhere ?
Why do you think that we need to conform to use only one assertion in the hundred that exists ?
The reason to use assertCount here is because it will make the error message when the test fails an order of magnitude easier to understand and that why we test.
There was a problem hiding this comment.
I'm in favour of assertCount(), but when creating pull requests it's considered good behaviour to conform to the style of the project.
Please create a pull request changing all the assertEquals into the better assertCount and when that gets merged first i'll gladly change the assertions.
PS. In my personal project I use assertSame and don't use assertEquals at all, as it doesn't catch subtle bugs like $this->assertEquals("1", true);
| if ($annots[0] instanceof IgnoreAnnotation) { | ||
| $this->assertEquals(2, count($annots)); | ||
| } else { // SimpleAnnotationReader doens't include the IgnoreAnnotation in the results. | ||
| $this->assertEquals(1, count($annots)); |
| $parser->setIgnoreNotImportedAnnotations(true); | ||
| $result = $parser->parse('@PHPUnit_Framework_TestCase'); | ||
|
|
||
| $this->assertEquals(0, count($result)); |
…mpleAnnotationReader into the SimpleAnnotationReaderTest
|
Merged and moved to count-oriented assertions. Will apply further fixes to assertions on |
Ocramius
changed the title
setIgnoreNotImportedAnnotations(true) doesn't work on already autoloaded classes
Because FuelPHP has a Package class, which isn’t an @annotation the following error occurs:
Duplicate of #25, requested by @Ocramius