Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Fix build failures in develop #59

Merged
merged 3 commits into from Dec 31, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions phpstan.neon
Expand Up @@ -2,3 +2,5 @@ parameters:
level: 7
paths:
- src/
ignoreErrors:
- '#Cannot call method getName\(\) on ReflectionType\|null.#'
12 changes: 9 additions & 3 deletions test/CodeGenerator/AbstractInjectorTest.php
Expand Up @@ -13,6 +13,7 @@
use Prophecy\Argument;
use Prophecy\Prophecy\ObjectProphecy;
use Psr\Container\ContainerInterface;
use ReflectionProperty;
use stdClass;
use Zend\Di\CodeGenerator\AbstractInjector;
use Zend\Di\CodeGenerator\FactoryInterface;
Expand Down Expand Up @@ -187,17 +188,22 @@ public function testConstructionWithoutContainerUsesDefaultContainer()
public function testFactoryIsCreatedFromClassNameString()
{
$subject = $this->createTestSubject(function () {
return ['SomeClass' => StdClassFactory::class ];
return ['SomeClass' => StdClassFactory::class];
});

$factoryInstancesProperty = new ReflectionProperty(AbstractInjector::class, 'factoryInstances');
$factoriesProperty = new ReflectionProperty(AbstractInjector::class, 'factories');
$factoryInstancesProperty->setAccessible(true);
$factoriesProperty->setAccessible(true);

$this->assertSame(
StdClassFactory::class,
self::readAttribute($subject, 'factories')['SomeClass'] ?? null
$factoriesProperty->getValue($subject)['SomeClass'] ?? null
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This and the other assertion check the internal state of the class, which is considered bad practice and readAttribute() has been removed from phpunit (see sebastianbergmann/phpunit#3338).

Testing the result of create() should be sufficient.

$this->assertInstanceOf(stdClass::class, $subject->create('SomeClass'));
$this->assertInstanceOf(
StdClassFactory::class,
self::readAttribute($subject, 'factoryInstances')['SomeClass'] ?? null
$factoryInstancesProperty->getValue($subject)['SomeClass'] ?? null
);
Copy link
Member

Choose a reason for hiding this comment

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

what is the reason of removing these assertions?

I know that probably these are not the best, as these checks attributes and we really shouldn't do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my Review comment. The test should not check the internal state and it will stop working with PHPUnit 9

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree it should not. The problem is that there was no any other way to check it, that's why it was done that way (and we have it in many components....).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, and it was me who added them. We could use Reflection directly, but it still feels kinda hacky and bad.
The question is: do we really need this kind of assertion, or could we do better with infection and benchmark testing here (two tools I'd like to be added anyways).

What do we want to test here? A factory class name should be accepted or more? If more: does it really add value?

That's some topic where I'd like to hear @Ocramius opinion, if he could spend a minute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michalbundyra I re-added the assertions with reflection, but I still don't think that they add any real value to the test.

}
}