SonataPageBundle icon indicating copy to clipboard operation
SonataPageBundle copied to clipboard

Added TransactionalManagerInterface

Open eerison opened this issue 3 years ago • 6 comments

Subject

I am targeting this branch, because {reason}.

Closes #{put_issue_number_here}.

Changelog

### Added
- Added some `Class::newMethod()` to do great stuff.

### Changed

### Deprecated

### Removed

### Fixed

### Security

eerison avatar Aug 11 '22 09:08 eerison

Could you please rebase your PR and fix merge conflicts?

SonataCI avatar Aug 17 '22 07:08 SonataCI

Ok I'm struggling a bit to mock the final class like SnapshotManager

What do you think to add it? https://tomasvotruba.com/blog/2019/03/28/how-to-mock-final-classes-in-phpunit/

eerison avatar Aug 17 '22 10:08 eerison

Ok I'm struggling a bit to mock the final class like SnapshotManager

What do you think to add it? https://tomasvotruba.com/blog/2019/03/28/how-to-mock-final-classes-in-phpunit/

You can use the real class new SnapshotManaer() (and mock the dependencies if needed) ; that's generally how we do it.

VincentLanglet avatar Aug 17 '22 10:08 VincentLanglet

Ok I'm struggling a bit to mock the final class like SnapshotManager What do you think to add it? https://tomasvotruba.com/blog/2019/03/28/how-to-mock-final-classes-in-phpunit/

You can use the real class new SnapshotManaer() (and mock the dependencies if needed) ; that's generally how we do it.

My test is: check if the commit and beginTransaction are called!

I saw that I need to check this by registry, I can do this, But maybe this dependency could let's ours tests simplest.

eerison avatar Aug 17 '22 10:08 eerison

Ok I'm struggling a bit to mock the final class like SnapshotManager What do you think to add it? https://tomasvotruba.com/blog/2019/03/28/how-to-mock-final-classes-in-phpunit/

You can use the real class new SnapshotManaer() (and mock the dependencies if needed) ; that's generally how we do it.

My test is: check if the commit and beginTransaction are called!

I saw that I need to check this by registry, I can do this, But maybe this dependency could let's ours tests simplest.

But we have a problem here, to mock it, I will need to mock methods created in others dependencies, and it's a problem, because if the dependency change their implementation, my test gonna broken!

and I remember that @jordisala1991 mention that it's a bad practice mock things that are not in our code/context, and it makes sense!

Note: Well I wouldn't not to mock the container to make this work :'(, IMO it should be just a simple unit test.

eerison avatar Aug 17 '22 10:08 eerison

// Mocks
        $objectManagerMock = $this->createMock(EntityManagerInterface::class);
        $objectManagerMock
            ->expects(static::once())
            ->method('beginTransaction');

        $objectManagerMock
            ->expects(static::once())
            ->method('commit');

        $registryMock = $this->createMock(ManagerRegistry::class);
        $registryMock->method('getManagerForClass')->willReturn($objectManagerMock);
        $snapshotPageProxyFactoryMock = $this->createMock(SnapshotPageProxyFactoryInterface::class);

        $snapshotManagerMock = new SnapshotManager(Snapshot::class, $registryMock, $snapshotPageProxyFactoryMock);

error: TypeError : Return value of Sonata\Doctrine\Entity\BaseEntityManager::getRepository() must be an instance of Doctrine\ORM\EntityRepository, null returned

I'm getting errors related with implementation of others stuffs 😞

eerison avatar Aug 17 '22 10:08 eerison

Could you please rebase your PR and fix merge conflicts?

SonataCI avatar Sep 04 '22 08:09 SonataCI

I'm closing this PR in favor of #1603

eerison avatar Sep 08 '22 09:09 eerison