Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DateInterval properties test #80

Merged
merged 1 commit into from
Jan 11, 2017
Merged

Add DateInterval properties test #80

merged 1 commit into from
Jan 11, 2017

Conversation

simPod
Copy link
Contributor

@simPod simPod commented Jan 10, 2017

So. I noticed it says that properties of DateInterval are undefined. But did I update the test correctly? Who knows... Also, this PR only includes updated test. Honestly, I have no Idea how to fix it.

@ondrejmirtes
Copy link
Member

Please fix that in PhpDefectClassReflectionExtension (it's easy - it already exists because ZipArchive class has the same problem) and test it in PhpDefectClassReflectionExtensionTest too. Thanks!

@ondrejmirtes
Copy link
Member

Unfortunately the test in AccessPropertiesRuleTest will always fail because the Broker instance created by AbstractRuleTest does not contain this extension. You need to move it to PhpDefectClassReflectionExtensionTest.

@simPod
Copy link
Contributor Author

simPod commented Jan 10, 2017

haha, is it that simple?

@ondrejmirtes
Copy link
Member

Yep, reflection of native PHP stuff is not always up to date so I'm already solving a few cases in PHPStan codebase.

@ondrejmirtes
Copy link
Member

@simPod I see you're implementing it :) One tip: You can use multiple @dataProvider above one test method. I'd like if there was only one test method also with a string $className parameter because right now there's a lot of duplication.

@simPod
Copy link
Contributor Author

simPod commented Jan 10, 2017

Good to know. Let me see

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

I found two small issues. Fix them please and squash the commits into one, I will merge it then :) Thanks!

$this->assertTrue($zipArchiveReflection->hasProperty($propertyName));
$propertyReflection = $zipArchiveReflection->getProperty($propertyName);
$this->assertInstanceOf(PhpDefectPropertyReflection::class, $propertyReflection);
$this->assertSame($zipArchiveReflection, $propertyReflection->getDeclaringClass());
Copy link
Member

Choose a reason for hiding this comment

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

Fix this variable name just to $classReflection

$propertyReflection = $zipArchiveReflection->getProperty($propertyName);
$this->assertInstanceOf(PhpDefectPropertyReflection::class, $propertyReflection);
$this->assertSame($zipArchiveReflection, $propertyReflection->getDeclaringClass());
$this->assertSame($typeDescription, $propertyReflection->getType()->describe());
Copy link
Member

Choose a reason for hiding this comment

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

Problem with this line is that when it fails, I have no idea which property is it. Add a third parameter - message with sprintf('%s::$%s', $className, $propertyName) and test how it looks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's amazing

@ondrejmirtes ondrejmirtes merged commit 703ea10 into phpstan:master Jan 11, 2017
@ondrejmirtes
Copy link
Member

Really nice, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants