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

Use SymfonyFixturesLoader to load fixtures #432

Merged
merged 2 commits into from
Jul 16, 2018

Conversation

magnetik
Copy link
Contributor

It's another try to fix #411.

The downside of the SymfonyFixturesLoader is that it always load all fixtures and cannot load a subset of it.

So I wrapped around some code to load only needed dependencies.

The test are not passing, but I'm not sure why as I can use it in my project.

@alexislefebvre alexislefebvre added this to the 2.0 milestone Jun 14, 2018
@alexislefebvre
Copy link
Collaborator

@magnetik I fixed tests in #433, you can rebase your PR on the 2.x branch and tests should work again.

@rvanlaak
Copy link
Contributor

As "symfony/framework-bundle": "^3.4 || ^4.0" is the requirement on 2.x, having auto-wiring on fixtures as the new default would be great for the new release 👍

@magnetik magnetik closed this Jul 11, 2018
@magnetik magnetik reopened this Jul 11, 2018
@magnetik
Copy link
Contributor Author

I think some test are failing because of my limited knowledge of functional tests. I'll take a look tomorrow but if you feel like taking a look I can use some help !

I'm quite confident that the feature is working as I use it in a dev project I have.

_defaults:
autowire: true
autoconfigure: true
Liip\FunctionalTestBundle\Tests\App\DataFixtures\ORM\:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think tests are failing because the service on this resource import should be tagged as fixture.

Liip\FunctionalTestBundle\Tests\App\DataFixtures\ORM\:
    resource: 'DataFixtures/ORM/*'
    tags: ['doctrine.fixture.orm']

see http://symfony.com/doc/current/bundles/DoctrineFixturesBundle/index.html#loading-fixtures

Copy link
Contributor

Choose a reason for hiding this comment

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

created magnetik#1 for you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

humm, I was thinking that the "autoconfigure" should have taken care of this automagically (as test are implementing the right interface)

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if DoctrineFixturesBundle did add __instanceof, but you might be right. Checking now if they did..

https://symfony.com/blog/new-in-symfony-3-3-service-autoconfiguration

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right, implementing ORMFixtureInterface should be enough to autoconfigure the doctrine.fixture.orm tag, whereafter FixturesCompilerPass will add the fixture to the definition of doctrine.fixtures.loader.

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've added the tag definition in the config.yml and the test are passing. Is that something that can be related to order in which the services are loaded ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I guess the DoctrineFixturesBundle is not loaded / included with the test environment's AppKernel?

That would also explain why it's working in your dev environment, as you probably did include the bundle in your kernel over there.

@magnetik
Copy link
Contributor Author

magnetik commented Jul 12, 2018

Okay so now it's green except for lowest dependencies.

And I've no idea why :D

@magnetik magnetik force-pushed the di-bug-fix branch 5 times, most recently from 97dddd2 to cbd2ba6 Compare July 12, 2018 09:48
@magnetik
Copy link
Contributor Author

Okay so it requires doctrine-fixtures-bundle 3.0.2 (3.0.0 is bugged).

I've adapted the composer.json :)

@rvanlaak
Copy link
Contributor

🎉

@alexislefebvre would be great if 2.0.0-alpha5 could get released after this PR is merged

@rvanlaak
Copy link
Contributor

Or maybe @maximgubar could help us in getting this merged and releasing 2.0.0-alpha5?


public function __construct(TokenStorageInterface $tokenStorage)
{
$this->tokenStorage = $tokenStorage;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way to use $this->tokenStorage in this test? I would like to ensure that this service is callable.

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've added a call in the load method.

@alexislefebvre
Copy link
Collaborator

LGTM, I'm surprised that it doesn't break anything even though the fixture loader has been changed.

Thanks!

@magnetik
Copy link
Contributor Author

magnetik commented Jul 16, 2018

I'm expecting quite a few bugs to pop in, regarding DX with the declaration of the service and so on.

Let's see how it goes when an alpha is released :)

@rvanlaak
Copy link
Contributor

Where the previous fixture loader did simply rely on plain old class instantiation or a service definition, the loader now basically is enhanced by one that supports autowiring and autoconfiguration. In other words, as long as userland's service definitions are valid everything should be fine. Before vs After 🎉 Really looking forward to this!

@alexislefebvre
Copy link
Collaborator

Could you please add a small documentation about these changes in https://github.com/liip/LiipFunctionalTestBundle/blob/2.x/CHANGELOG.md, and https://github.com/liip/LiipFunctionalTestBundle/blob/2.x/UPGRADE-2.0.md if this is a breaking change?

@magnetik
Copy link
Contributor Author

Done

@alexislefebvre alexislefebvre merged commit 15e82b4 into liip:2.x Jul 16, 2018
@alexislefebvre
Copy link
Collaborator

Thanks @magnetik and @rvanlaak for your work on this PR, I released the 2.0.0-alpha5 version: https://github.com/liip/LiipFunctionalTestBundle/releases/tag/2.0.0-alpha5

@@ -25,9 +25,9 @@
"symfony/phpunit-bridge": "^3.4 || ^4.0",
"doctrine/orm": "^2.6",
"symfony/monolog-bundle": "^3.1",
"doctrine/doctrine-fixtures-bundle": "^3.0",
"doctrine/doctrine-fixtures-bundle": "^3.0 >=3.0.2",

Choose a reason for hiding this comment

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

This line shouldn't be in require section instead of require-dev ?
Since we use it in Services\FixturesLoaderFactory, it's became a dependency, WDYT ?

Got this error when updating to 2.0.0-alpha5

The service "liip_functional_test.services.fixtures_loader_factory" has a dependency on a non-existent service "doctrine.fixtures.loader". 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hum so we should load the service only if the DoctrineFixturesBundle is available, because you should be able to use the bundle without using the fixtures

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 #437

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.

4 participants