Skip to content
This repository has been archived by the owner on Sep 25, 2022. It is now read-only.

Always use the current services #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

egeloen
Copy link
Contributor

@egeloen egeloen commented Jul 13, 2016

Hey @theofidry!

I hope you're fine since my last PR :) Let"s first try to explain what does this PR and why I need to refactor the library this way...

All the contexts are based on the KernelAwareContext in order to have access to the Symfony2 kernel. When the kernel is set, you have implemented an init method which fetch the right services from the container and propagate/store them in your context.

It could work if behat calls the setKernel for each scenarios but it is not the case, the setKernel is called for each scenarios but not for each scenarios outline. Then, if we are in a scenarios outline which have an high number of examples, your context does not use anymore the doctrine orm/odm from the current container but from the very first container and my memory grows and grows (beause the shutdown of the DoctrineBundle is not executed on the right container and so the orm/odm are not clear/close) as well as having strange behavior when I do assertion on the database...

This PR is much more a discussion than a PR to merge since it breaks BC but since I need it in a project I push it here in order to discuss the current extension implementation.

WDYT?

@theofidry
Copy link
Owner

Hi @egeloen :)

If I understand correctly, contexts are re-created for each scenario except scenario outlines for which it's the same context for each example is that right? (just want to confirm that before continuing the conversation)

@egeloen
Copy link
Contributor Author

egeloen commented Jul 13, 2016

Yes, I confirm it and the kernel is rebooted for each scenario and scenario outline.

@theofidry
Copy link
Owner

Ok, so I think the problem rather comes from alice loader and the doctrine manager to keep references after each loading. A simple fix would be to actually wipe out all the references for both after each loading.

@egeloen
Copy link
Contributor Author

egeloen commented Jul 13, 2016

We notice with @geoffrey-brier that clearing the object managers after flushing it directly in alice also fixes the issue but I was not sure that using a different object manager than the one stored in the current container was a good idea...

@theofidry
Copy link
Owner

@egeloen there's doctrine manager which keeps the persisted references, but there's also alice loader which keeps all references. Ideally, it would be better to find a hook to re-create the contexts for each scenario outline example. Actually I think this should be discussed in Behat as well as it's kind of a false promise: you have the guarantee to have a clean state for scenarios but not for scenario outlines? That looks weird to me. But in any case I think it would be great to add this "cleanup" after loading to the extension.

@egeloen
Copy link
Contributor Author

egeloen commented Jul 13, 2016

I'm a little bit lost between alice, the bundle and the extension... Can you point me where you would like to clear the manager and the loader?

@egeloen
Copy link
Contributor Author

egeloen commented Jul 13, 2016

An other alternative would be to introduce a method with a @BeforeScenario tag which will re-init the context. AFAIR, this tag is also triggered for scenarios outline.

@theofidry
Copy link
Owner

Can you point me where you would like to clear the manager and the loader?

An issue has already been filled in alice (nelmio/alice#418) but I believe this should be fixed in AliceBundle rather (https://github.com/hautelook/AliceBundle/issues/239).

This should probably be done here before returning the objects.

@theofidry
Copy link
Owner

An other alternative would be to introduce a method with a @BeforeScenario tag which will re-init the context. AFAIR, this tag is also triggered for scenarios outline.

I think it's a problem that should be fixed at the Bundle level as although it doesn't seem to have been an issue for now, there is still this big memory leak in regular (non Behat) tests as well or long-lived processed which would load fixtures.

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

Successfully merging this pull request may close these issues.

3 participants