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

feat: behat support #378

Draft
wants to merge 3 commits into
base: 1.x
Choose a base branch
from
Draft

feat: behat support #378

wants to merge 3 commits into from

Conversation

kbond
Copy link
Member

@kbond kbond commented Dec 7, 2022

Fixes #235.

@kbond kbond mentioned this pull request Dec 7, 2022
@kbond
Copy link
Member Author

kbond commented Dec 7, 2022

I'd like this to work seamlessly with dama if possible!

@nikophil
Copy link
Member

nikophil commented Dec 7, 2022

do you have any problems with dama?

@kbond
Copy link
Member Author

kbond commented Dec 7, 2022

I don't have the behat tests working yet. I just wanted to note this for when I do (dama has a behat extension)

@mpdude
Copy link
Contributor

mpdude commented Dec 9, 2022

I have been thinking a bit about this since we started the discussion in #235, and I am not sure that adding the Behat support in this repo here is the best way to go. I'll try to give reasons by looking at package dependencies.

  1. Let's get straight that dev dependencies only work for the "root" package, i. e. when developing zenstruck/foundry itself. They are not relevant for anyone installing zenstruck/foundry as a dependency themselves, regardless of whether that is as a regular or as a dev dependency in their repo.

  2. To ensure compatibility with the libraries and stuff we build upon (behat/behat for sure, friends-of-behat/symfony-extension or behat/symfony2extension possibly?), sooner or later we need to enforce version constraints for those packages.

  3. suggest dependencies have no significance and do not enforce version constraints. conflicts might prevent people from installing zenstruck/foundry with particular versions of the other packages, even when they are not interested in using the Behat-related features (maybe they have their own context and helper classes already?). require would force everyone using Foundry to install Behat as well, which is a waste of resources.

  4. Especially when we're starting out with the Behat related features, this might be a moving target, i. e. not stable initially and with some unavoidable BC breaks in the early days. If that code is part of the zenstruck/foundry repo, we'd either be forced to have major version bumps that scare away users that are not interested in the Behat-related stuff; or have breaking changes in the Behat-related code that are not conveyed by the version numbers used.

TL;DR IMO Behat support should live in a dedicated repo/package (zenstruck/foundry-behat or zenstruck/foundry-behat-extension, zenstruck/foundry-behat-bridge, ...?) so it can be installed separately only by users who need it, can declare dependencies clearly, have independent releases and start with a 0.x version number.

@mpdude
Copy link
Contributor

mpdude commented Dec 9, 2022

Additional remarks after looking at what can be seen over at https://github.com/dmaicher/doctrine-test-bundle/tree/master/src/DAMA/DoctrineTestBundle/Behat, because this was mentioned above.

Their code needs to be registered as a Behat extension and will setup listeners to do the setup/teardown work in the database for every test.

This may really slow down tests a lot if not all tests (scenarios/examples) need it.

By having a Behat context that takes care of this, users are free to work with different Behat test suites and register the context only for those suites where they care.

If it even were a Behat "step definition", I could start the relevant scenarios with Given the database has been reset (or put this into a feature Background). Shutdown work should/need only happen when the setup has been triggered.

@kbond
Copy link
Member Author

kbond commented Dec 9, 2022

Thanks for the great feedback on this @mpdude!

Am I correct the same dependency argument could be made for mongo support? My concern with another package (at this point) is maintenance. I've been burned by this in the past (creating a php package, symfony bundle for this package, laravel provider for this package, etc). I find it's easier to maintain and keep all compatible when they are in a single "mono-repo". In the future, I have no problem creating sub-tree splits (like Symfony core or Flysystem does) to create separate packages.

For instance, in the future, we could potentially split mongo/behat into different readonly packages but keep maintenance here.


Regarding the behat support itself: Should we have a behat extension + context(s)? If you want to have it available for all scenarios, use the extension. If not, use the contexts on a scenario-by-scenario basis?

- Zenstruck\Foundry\Tests\Behat\TestContext
- Zenstruck\Foundry\Test\Behat\FactoriesContext
extensions:
FriendsOfBehat\SymfonyExtension:
Copy link
Member Author

@kbond kbond Dec 9, 2022

Choose a reason for hiding this comment

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

I need some help figuring out why this doesn't seem to be working. When I run vendor/bin/behat I get the following error:

Can not find a matching value for an argument $container of the method Zenstruck\Foundry\Test\Behat\FactoriesContext::__construct().

I've confirmed the kernel isn't being booted but not sure why.

Choose a reason for hiding this comment

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

I think the problem is to do with this: src/Listener/KernelOrchestrator.php:tearDown in symfony extension...

Choose a reason for hiding this comment

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

Its loaded in the service extension here:
\FriendsOfBehat\SymfonyExtension\ServiceContainer\SymfonyExtension::loadKernelRebooter

Copy link

@roboticflamingo roboticflamingo Feb 7, 2023

Choose a reason for hiding this comment

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

I would probably try to restructure the new foundry behat contexts in this PR as a listener (like KernelOrchestrator) and perhaps try to use the priority (int) on the subscribed callbacks (e.g. 16 or something) to trigger database/foundry registry reset. Ive found it possible to induce memory leaks quite easily or race conditions in a few integrations ive attempted previously.

@mpdude
Copy link
Contributor

mpdude commented Dec 9, 2022

Am I correct the same dependency argument could be made for mongo support?

Yes – just as an example, nothing prevents users from installing Foundry with doctrine/mongodb-odm-bundle 4.1.*. It's not an acceptable dev-dependency for Foundry itself for whatever reason (assuming that's intentional and correct), but Composer won't complain when users choose it. If it works, the constraint is unnecessarily strict. If it does not work, people will raise issues here.

You could even argue about the Doctrine dependency. In fact I am even using Foundry in parts with a legacy ORM system with lots of withoutPersisting() and manual MyBackend::save() calls, but hey!

Since Foundry describes itself as making "creating fixtures data fun again [...] with Symfony and Doctrine", that's fine.

"Everything is a trade-off", they say?

My concern with another package (at this point) is maintenance. I've been burned by this in the past

I feel you – have been there as well. Especially when issues start to pop up all over the place and especially in the wrong place. It's not always easy to decide for the uninitiated where an issue or PR should go.

I have been maintaining projects/repos where you need to install three or four packages to gather everything you need, and some of those consist of only one or two classes. Basically a system where you have a "core" and then some "import" and "export" adapters around it, with different packages for the doctrine-import, mongo-import, solr-export, lucene-export etc.

In the future, I have no problem creating sub-tree splits (like Symfony core or Flysystem does) to create separate packages.

Yes – and note every single one of them comes with their own composer.json.

But how do you keep those subtree splits updated, and how/where will you tag releases when the different parts evolve at a different pace?

Regarding the behat support itself: Should we have a behat extension + context(s)? If you want to have it available for all scenarios, use the extension. If not, use the contexts on a scenario-by-scenario basis?

Honestly I don't know. I have been using Behat for quite some time now, but never had to write an extension for it. My guess would be:

  • You need an extension when you want to make it configurable from within behat.yml. Probably extensions can interfere/hook into the way the container used by Behat itself can be built, but they can also register pre/post scenario/example listeners.
  • Contexts can be made available per suite, and if they have @BeforeScenario/@AfterScenario hooks, those will run for all tests within the suite.
  • A plain step definition is a method that must be in one of the registered contexts for the suite, but it does nothing unless you mention it in the scenario (in Gherkin).

Note that contexts cannot be used on a per-scenario basis (you probably mean the step definition?).

@kbond
Copy link
Member Author

kbond commented Dec 9, 2022

You could even argue about the Doctrine dependency

Yes, I regret tightly coupling to the ORM (even though it's not required). Maybe we can make some improvements here in 2.0.

I'd really like ModelFactory::create() to just work w/o needing the Factories trait for unit tests. The problem is it's common for users to forget to add the Factories trait in their integration/functional test so wanted to give a helpful error. As you say: "everything's a trade-off"

But how do you keep those subtree splits updated, and how/where will you tag releases when the different parts evolve at a different pace?

I think the system Symfony/Flysystem uses is open source so I can investigate this.


I'm sort of thinking the following but let me know what you think:

  • FactoriesContext: have it provide step definitions ("Given I have factories enabled") instead of hooks
  • ResetDatabaseContext: same here ("Given I reset the database")
  • FoundryExtension: configurable in the following ways:
    • enable resetting db before the suite
    • enable resetting the db schema before every scenario suite-wide
    • enable enabling factories before every scenario suite-wide
    • option for adding @factories annotation for scenarios you want factories (assuming you don't enable suite-wide)
    • option for adding @reset-db-schema annotation for scenarios you want to reset the db schema for (assuming you don't enable suite-wide)

(I don't know if all of the above is possible)

@mpdude
Copy link
Contributor

mpdude commented Dec 9, 2022

@kbond Poking around your code... think I got things working, except one part: When/how does the database schema get set up for tests?

@kbond
Copy link
Member Author

kbond commented Dec 9, 2022

When/how does the database schema get set up for tests?

It should be created when the database is reset (initially), then reset (dropped/created) before each scenario (with the current hook-based setup).

For the how, I created the ResetDatabaseContext to do this (it isn't yet added to behat.yml.dist).

@kbond
Copy link
Member Author

kbond commented Dec 15, 2022

Missed your PR @mpdude (didn't get notified for some reason).

kbond#2 - will check it out later today

@nikophil
Copy link
Member

But how do you keep those subtree splits updated, and how/where will you tag releases when the different parts evolve at a different pace?

what Symfony does is to simply tag all components each time a new version is realeased, even if the component did not had any changes. See for example https://github.com/symfony/yaml/releases, notice all releases with no significant changes. this is a pragmatic approach which is quite simple IMO

@kbond
Copy link
Member Author

kbond commented Dec 16, 2022

even if the component did not had any changes.

I believe we no longer make bug-fix tags if there are no changes.

* Get POC running

* Fix after schema @annotation
@kbond
Copy link
Member Author

kbond commented Dec 16, 2022

Thanks for the PR @mpdude, the root of my issue was not passing the env vars...

USE_ORM=1 USE_FOUNDRY_BUNDLE=1 DATABASE_URL=mysql://... vendor/bin/behat

So how do you think we should proceed with this PR? Do my ideas above make sense?

@roboticflamingo
Copy link

Is there are movement on this? I got it wokring with behat, but recent changes have broken things so im looking for something stable going forward for my behat test suite...

@kbond
Copy link
Member Author

kbond commented Feb 7, 2023

@roboticflamingo, does the plan I laid out in my above comment make sense to you? I have no experience with behat so I'm in need of some direction here.

@roboticflamingo
Copy link

roboticflamingo commented Feb 7, 2023

@kbond lol I am also in the same boat re behat 3 - there doesnt seem to be much about the internals re documentation. Based on what I know about behat, I would say architecturally yes (with the potential addition of a subscriber - see my comment in code on KernelOrchestrator), however, I think having tried creating something similar the biggest problem is synchronising the kernel reboot, clearing the foundry registry and the database reset to happen together (My first guess is I think your error is a symptom of callback order).

BTW ive found foundry to be excellent. Its the best thing out there by far for fixture management. The traits dont work in all situations, but I tore the bits out to integrate it - so was no problem tbh.

use Foundry for creating test data. Foundry needs to be configured,
started and stopped accordingly. The database has to be reset before
scenarios are run.

Choose a reason for hiding this comment

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

PS might be worth adding a test for "example tested" syntax.
https://behat.org/en/latest/user_guide/writing_scenarios.html#scenario-outlines

Associated events for subscriber: ExampleTested::BEFORE, ExampleTested::AFTER

@@ -0,0 +1,15 @@
Feature: Behat support for Foundry

Choose a reason for hiding this comment

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

Also probably add an example of using backgrounds to implement the fixtures. I use the back ground to set up core fixtures (that are used in all tests).
https://behat.org/en/latest/user_guide/writing_scenarios.html#backgrounds

}

/**
* @BeforeScenario

Choose a reason for hiding this comment

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

These annotations cause race conditions because you cannot guarantee when they are run... hence why I think subscriber is the way to go.

@kbond
Copy link
Member Author

kbond commented Feb 7, 2023

with the potential addition of a subscriber

In Behat, is a subscriber a different concept than an extension?

@roboticflamingo
Copy link

Yes they are different - I think everzet intended them to be a replacement for the annotations - they are Symfony event subscribers - you can load them in the extension using:

public function load(ContainerBuilder $container, array $config): void
{
    $container->register('roboticflamingo.listener', DoctrineListener::class)
        ->addTag(EventDispatcherExtension::SUBSCRIBER_TAG)
    ; 

@roboticflamingo
Copy link

roboticflamingo commented Feb 7, 2023

I am just rewriting mine with the updated code from this repo, that will work with background, examples and scenarios. If I can get it working I will report back here, probably be thursday night though before I get it done or report any issues.

Update: It is actually working, but I have to test it with DAMA support!

@roboticflamingo
Copy link

Yes they are different - I think everzet intended them to be a replacement for the annotations - they are Symfony event subscribers - you can load them in the extension using:

public function load(ContainerBuilder $container, array $config): void
{
    $container->register('roboticflamingo.listener', DoctrineListener::class)
        ->addTag(EventDispatcherExtension::SUBSCRIBER_TAG)
    ; 

https://symfony.com/doc/current/event_dispatcher.html#creating-an-event-subscriber

Here are some example events:
public static function getSubscribedEvents(): array {
return [
BackgroundTested::BEFORE => ['before', 16],
ExampleTested::BEFORE => ['before', 16],
ScenarioTested::BEFORE => ['before', 16],
ExampleTested::AFTER => ['after', -16],
ScenarioTested::AFTER => ['after', -16]
];
}

Note you must register it in the extension (so you might have FoundryExtension and FoundryListener) - KernelOrchestrator in fos symfony extension for behat is a good example actually :):

https://github.com/FriendsOfBehat/SymfonyExtension/blob/master/src/Listener/KernelOrchestrator.php

@roboticflamingo
Copy link

OK I think I have a working subscriber recipe for this repo... :)

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

Successfully merging this pull request may close these issues.

Behat Integration
4 participants