Closed mpdude closed 1 year ago
@walva since you opened #149, I'd love to get your :eyes: on this one.
I'll do it this weekend!
I would like to test your PR in my applications to make sure there are no BC breaks. Even I see the repository is already eavily tested and I reviewed the test, and it's just perfect, it solves completely the issues.
I agree with everything so far. Furthermore, I'll have a more in-depth look during the weekend.
I think rebooting this kernel instance after each scenario/example as well is correct in its own right I do agree with this bold statement.
I see the worfklow await approval from 1 of the maintainer. @pamil would agree to run the checks on this PR ?
@mpdude can you please resolve conflicts )
@Yozhef Rebased + squashed commits. Would ❤️ to hear what you think about this.
Two of the tests I added here are failing against the current master
, as can be seen here: https://github.com/mpdude/FriendsOfBehat_SymfonyExtension/actions/runs/3542948400/jobs/5948973471
Those are the symptoms described in #149.
@mpdude I think this is a very good idea. Currently, I am also looking for problems with resetting Doctrine Translate in my project -> sometimes the wrong translation is returned)
Awesome we got this merged this so fast 🎉
I hope it does not break things for anyone, but at least it would break Tests only and that’s easy to see.
@Yozhef do you think you could make a release for this?
If you would like to have some written release notes and/or a changelog entry, let me know and I’ll see what I can do
@mpdude It would be great if you wrote a changelog and a note. I want to understand tomorrow how this will affect the current projects to make the correct release tag.
Thank you very much for the job done!!😍
I have run these changes against two clients' projects where we use the *.driver_container
to modify/inspect state, and all tests still passed.
From my POV, it's a bugfix release. The behavior is now fixed to what one would/should expect, with no container state leaking between scenarios, and no driver_container
reboots happening just because another scenario was exectued before where a Mink request was made. Also, no new switches, methods or classes added that would justify a minor release version.
Regading a text that you could use for the GitHub Release notes, after looking at https://github.com/FriendsOfBehat/SymfonyExtension/compare/v2.3.1...master my suggestion would be:
This bugfix release addresses a bug (#149) related to when the Symfony Kernel and Container used by the Mink driver are being reset.
With this fix, the behat.driver.service_container
is reset:
RawMinkContext::visitPath()
, Given I am on ...
or When I go to ...
) within a scenario is being made.The documentation describes these changes a bit more in detail.
I forgot to publish one comment 😅 Anyway, this fix is remarkable as it is. I would like to address the limitation below at some point.
From:
Feature:
Scenario:
Given the counter service is zeroed
And I increment the counter
When I visit the page "/hello-world"
Then I should see "The counter value is 2" on the page
And the counter service should return 2
# Now a second request is made, which will reset the container, but leaves us
# with no easy way of pre-setting the container once again:
When I increment the counter
# ... you might expect "3"
And I visit the page "/hello-world"
# ... now you might expect "4". But, in fact, the reset happened just before the request.
Then I should see "The counter value is 1" on the page
And the counter service should return 1
To:
Feature:
Scenario:
Given the counter service is zeroed
And I increment the counter
When I visit the page "/hello-world"
Then I should see "The counter value is 2" on the page
And the counter service should return 2
# Now a second request is made, which will reset the container, but leaves us
# with no easy way of pre-setting the container once again:
When I increment the counter
And I visit the page "/hello-world"
Then I should see "The counter value is 4" on the page
And the counter service should return 4
Congratulation @mpdude !
@walva @mpdude thanks for a good release) Already available by tag v2.4.0)
Thank you for your support and @Yozhef for the swift replies!
Thanks a lot, Matthias, for all the work here and in the other PRs and also to Yozhef for reviewing and merging! You're great!
I´m still facing this issue. The first Scenario goes well, but the second seems to not have the correct container. The Container what is mentioned here is the one, i currently use. Anything else to do to have every step mockable?
Thx!
Current Code-Part:
protected ContainerInterface $container;
public function __construct(private readonly KernelInterface $kernel) { }
#[BeforeStep]
public function beforeStep($event): void
{
$this->container = $this->kernel->getContainer()->get('behat.driver.service_container');
}
#[Given('MockMySymfonyService')]
public function mockMySymfonyService(): void
{
$data = 'mockInto';
$this->container->get('mocked.services')->mock($data);
}
This tries to fix #149.
TL;DR:
KernelOrchestrator
reset not only thefob_symfony.kernel
, but alsofob_symfony.driver_kernel
at the end of every scenario/exampleSymfonyDriver
Mink implementation, create a newKernelBrowser
instance whenever the driver isreset()
– that is, at least at the beginning of every new feature.Example of successful build: https://github.com/mpdude/FriendsOfBehat_SymfonyExtension/actions/runs/3543814732 (this PR + fixed build pipeline from #191).
My impression is that the bug described in #149 has two aspects.
First:
Currently, the
KernelOrchestrator
class is used to reset thefob_symfony.kernel
kernel instance. This kernel instance is used at the Behat level to provide and configure contexts. At the end of each scenario or example, that kernel is shut down and booted again, which also clears/resets its container. This gives a clean state and separation of context instances while executing different scenarios.[^1]There was, however, no similar handling of the
fob_symfony.driver_kernel
kernel instance, which is the one that requests are sent to (through Mink and abstractions) and where the application or SUT (system under test) actually runs.I think rebooting this kernel instance after each scenario/example as well is correct in its own right.[^2]
Second:
We should use fresh/new
KernelBrowser
instances every time the Mink driver isreset()
.To my understanding,
reset()
in Mink is the equivalent of bringing a browser to a state as clean as possible, without completely terminating and re-starting it. This may not be an issue for something as lightweight as BrowserKit and theKernelBrowser
, but of course for real headless browsers the difference is important.Until now, the
SymfonyDriver
would use a singleKernelBrowser
instance throughout the entire lifetime, even acrossstop()
/start()
cycles.Since the
KernelBrowser
used insideSymfonyDriver
maintains state in order to reboot the kernel immediately before the second and every subsequent request, and since there is no way to reset this state, we should use a newKernelBrowser
instance at least uponreset()
, which is called by\Behat\MinkExtension\Listener\SessionsListener
at the beginning of every scenario.Re-creating the
KernelBrowser
makes sense for a second reason:The
KernelBrowser
(test.client
service) is taken from thefob_symfony.driver_kernel
kernel instance, which is also rebooted between scenarios. Fetching thetest.client
service repeatedly makes sure we are using the same instance that others might look up in the container, and not an "old" instance created before the kernel was rebooted().[^1]: Note that the kernel is not booted when a scenario/example starts and shut down at the end, since it needs to be in a usable state also somewhere in between. Instead, we have a immediate shutdown()/boot() sequence at the end of scenarios/examples, and an initial boot() when the
fob_symfony.kernel
service is created in Behat's own service container.[^2]: "When/how was the
fob_symfony.driver_kernel
kernel reset/rebooted until now?", you might ask. I think these reboots happened upon another occasion: When theKernelBrowser
used by theSymfonyDriver
detects that it has already processed another request before, it would do the reboot immediately before handling the next request. This previous request may have been in another scenario, or a previous request in the same scenario. Note that with two requests in a single scenario, you might not be able to prepare the kernel for the second request, since theKernelBrowser
will do the reset immediately before the request, so you have no occasion to intervene.