Open martin-rueegg opened 9 months ago
I'm not excited about fixtures in general. I've found code to be more readable by just creating helpers that take care of creating records that I need.
I do things like this in my test suite:
public function testSomething(FunctionalTester $I): void
{
// This helper returns the same object for every call inside the same test
$project = $I->haveProject();
$testSubject = new TestSubject();
$testSubject->project_id = $project->id;
}
I've found this to lead to more readable tests. That said, if I had to choose function level attributes would be a big readability improvement over class level fixtures via method or attributes.
Could you provide some code samples of what you envision?
Thanks, @SamMousa, for looking into this.
I've meanwhile updated the introduction to the PR in humhub which I quote here for easier access:
The following functionality is supported:
#[FixtureEmpty]
to disable fixtures for the current class/methodFixtureConfig
$useConfig
a flag to fall back to the configuration-level setting$useDefault
a flag to fall back to the default configuration of the attribute that extends from FixtureConfig
and defines such a default set (see FixtureDefault
)$fixtures
an array to cusomize the configuration
['user' => ['class' => UserFixture::class], ...]
default
(as array value) which will mix in the default setuser
)#[FixtureEmpty]
class SomeTest extends Unit
{
// #[FixtureEmpty] is synonym with #[FixtureConfig([], useConfig: false, useDefault: false)]
public function testFoo(): void
{
// this test uses no fixtures (inherited from the class)
}
#[FixtureConfig(useConfig: true)]
public function testBar(): void
{
// this test uses the fixtures from the configuration
}
}
#[FixtureConfig([
'user' => ['class' => UserFixture::class],
'group' => ['class' => GroupFixture::class],
])]
class SomeTest extends Unit
{
public function testFoo(): void
{
// this test uses the "user" and "group" fixtures (inherited from the class)
}
#[FixtureEmpty]
public function testBar1(): void
{
// this test uses no fixtures
}
#[FixtureConfig(['user'])]
public function testBar2(): void
{
// this test uses only the "user" fixture from the class
}
}
#[Attribute]
class FixtureDefault extends FixtureConfig
{
public function getDefaultFixtures(): array
{
return [
'user' => ['class' => UserFixture::class],
'group' => ['class' => GroupFixture::class],
];
}
}
#[Attribute]
class FixtureSpecialSet extends FixtureDefault
{
public function getDefaultFixtures(): array
{
return parent::getDefaultFixtures() + [
'articles' => ['class' => ArticleFixture::class],
];
}
}
#[FixtureDefault]
class SomeTest extends Unit
{
public function testFoo(): void
{
// this test uses the "user" and "group" fixtures (inherited from the class, defined in FixtureDefault)
}
#[FixtureEmpty]
public function testBar1(): void
{
// this test uses no fixtures
}
#[FixtureDefault(['user'])]
public function testBar2(): void
{
// this test uses only the "users" fixture from the class
}
#[FixtureDefault(['user', ['articles' => ['class' => ArticleFixture::class]])]
public function testBar3(): void
{
// this test uses only the "user" fixture from FixtureDefault, and adds an additional fixture "articles"
}
#[FixtureDefault(['default', ['articles' => ['class' => ArticleFixture::class]])]
public function testBar4(): void
{
// this test uses all fixtures from FixtureDefault, and adds an additional fixture "articles"
}
#[FixtureSpecialSet]
public function testBar5(): void
{
// this test uses all fixtures from FixtureSpecialSet (which includes those from FixtureDefault)
}
}
The advantage of the attribute solution is that
Some generic remarks, that are personal to me, but might not be a dealbreaker.
FixtureConfig
attribute multiple times instead of supporting a dictionary config?#[FixtureEmpty]
attribute at the class level. If you don't want global fixtures, don't use them. From a code readability perspective it is terrible to (possibly) have globally configured fixtures and which may or may not be disabled at a class level.
Imagine me reading one of your tests and then having to traverse all the configuration to figure out what fixtures I should expect to be available. Within a single test class I'd say this is acceptable, but in my opinion this new feature should not interact with global fixtures at all.Also, we should consider whether we need this feature in the core or whether it could be supported via an extension instead. Since if we merge a feature like this, the burden is on us to maintain it and we already struggle maintaining the code that we have.
I'm happy to review a PR from a code quality perspective, but I'm not going to merge it or decide that we want this living inside this library, for that I hope one of the other maintainers will have an opinion.
Thank you again for your valued time and feedback!
- I don't like complicated array syntax, they are bad for discoverability. (even though Yii2, uses them everywhere)
- Why not uses the
FixtureConfig
attribute multiple times instead of supporting a dictionary config?
Yep, could be an option. The reason why I did it this way, was that most of the logic is encapsulated within FixtureConfig
. If we support the attribute to be repeated, the logic to merge requires support from the code that resolves the fixtures.
- These attributes should be final, we definitely should not tell or even support users extending from them, that's a maintainability nightmare. Use arguments to the attribute instead, and if people want to create their own attributes that do something special, they should implement an interface that we define.
Sounds good to me.
- In my opinion we should not support the
#[FixtureEmpty]
attribute at the class level. If you don't want global fixtures, don't use them. From a code readability perspective it is terrible to (possibly) have globally configured fixtures and which may or may not be disabled at a class level. Imagine me reading one of your tests and then having to traverse all the configuration to figure out what fixtures I should expect to be available. Within a single test class I'd say this is acceptable, but in my opinion this new feature should not interact with global fixtures at all.
My view on this is that whenever a an attribute is used, be it on the class or method level, the levels "above" are ignored, unless specifically included. As such, the #[FixtureEmpty]
attribute at the class level gives me the peace of mind that no fixtures are involved whatsoever.
E.g. I'm an external contributor to the humhub project. I'm not familiar with the entire fixture configuration which is different on a module-per-module basis. If I want to contribute a test for a code that I contribute, then it would make my life much simpler to just add this attribute on the class level and do not have to first figure out the global configuration or put that attribute on every single test. - It's the same problem as you mentioned (traversing all the configuration), just a different perspective on it. :-)
Also, we should consider whether we need this feature in the core or whether it could be supported via an extension instead. Since if we merge a feature like this, the burden is on us to maintain it and we already struggle maintaining the code that we have.
I appreciate this. I'm not too familiar with all of this and would not know how to create an extension that would interact in the right way. But maybe with your help that could work.
I'm happy to review a PR from a code quality perspective, but I'm not going to merge it or decide that we want this living inside this library, for that I hope one of the other maintainers will have an opinion.
That is very kind of you. I appreciate your offer.
Given that it is quite some work to adapt the code to your project, would it be possible to get a preliminary feedback on this from you guys as a "project"? Would you be able to tag the right person(s), as I don't know who would be involved here.
I appreciate this. I'm not too familiar with all of this and would not know how to create an extension that would interact in the right way. But maybe with your help that could work.
Extensions are documented in the codeception documentation, the reasoning is simple:
So all you need to find out is if there is an event that happens just before the test and just after the test.
@samdark what do you think?
Thank you, @SamMousa.
Extensions are documented in the codeception documentation, the reasoning is simple:
- Extensions can react to events
- Events get passed the test (and can thus inspect its attributes)
So all you need to find out is if there is an event that happens just before the test and just after the test.
If I get the picture right, the approach with an extension would be to
test.start
or test.before
and there install the fixtures as configured in the attribute(s), basically replicating the code in Yii2::loadFixtures()
and haveFixtures()
methods.test.end
event and there pull down the fixtures that had been installed before, replicating the code in your _after()
method:
https://github.com/Codeception/module-yii2/blob/d819def01313d1b4f8b3e2127f9ea4171a718346/src/Codeception/Module/Yii2.php#L378-L383Assuming that the extension instance remains the same between the two events, I could store the fixtures in an instance field once pulled up so that I know what to pull down later - as you do in $loadedFixtures
Given the the similarity of the task and code, it would of course be nice to have it integrated. If not, it might be simpler to just create a composer module that provides the attributes - but not the extension - together with an instruction how to enable it with your fixturesMethod
...
Ok, let's see what the panel says. :-)
@samdark @Naktibalda just a gentle poke and the question, if you'd be happy for such an extension of functionality, as described in the examples above, is welcomed to live in this repo?
If so, I'd prepare a PR and @SamMousa has generously offered to do a code review.
Thanks!
Hm... I'd love fixtures that are specific for a test or use-case.
Hm... I'd love fixtures that are specific for a test or use-case.
Thanks @samdark. Shall I take that as a Yes, that you'd be open to receive a PR that implements the feature with attributes?
In order to be backwards-compatible, I'd foresee that if there is a fixturesMethod
defined (i.e. _fixtures()
), attributes will be ignored. If the author wants those attributes to be respected, there will be a method that can be called to get the results from the attribute evaluation and it can be returned from _fixtures()
. (We might want to issue a warning, if that is not done [i.e. if fixtures are defined but not "used" by the fixturesMethod
.])
For better readability and understanding of the code, I'd suggest that as soon as the class has a fixture attribute, the fixtures from the configuration are ignored entirely. But there will be an attribute that allows the author to draw back on the fixtures from the configuration on method level.
The same applies to methods that have a fixture attribute, of course: unless specifically included through an attribute setting, class-level and configuration-level fixtures will be ignored for the given test.
If that sounds good I'll do my best to provide an initial PR to be reviewed and discussed.
I think we should be able to have this in a purely additive manner, without complex interactions.
Just have 1 attribute to ignore "parent" fixtures. Another attribute to add fixtures. The attribute can be used in conjunction with configuration and the standard fixturesmethod imo.
ie the fixtures method has the same priority as the class level fixture attribute, and you may use both at the same time.
On a more abstract level i'm not 100% convinced of using attributes. If the goal is to have fixtures located close to the function that use them, why not just have helper functions inside the test? If you want them at the class level, why is it cleaner to have them as an attribute? In this case attributes just have lower expressive power than a method on the class; what are your thoughts on this?
I think we should be able to have this in a purely additive manner, without complex interactions.
Just have 1 attribute to ignore "parent" fixtures. Another attribute to add fixtures. The attribute can be used in conjunction with configuration and the standard fixturesmethod imo.
ie the fixtures method has the same priority as the class level fixture attribute, and you may use both at the same time.
Ok.
On a more abstract level i'm not 100% convinced of using attributes. If the goal is to have fixtures located close to the function that use them, why not just have helper functions inside the test? If you want them at the class level, why is it cleaner to have them as an attribute? In this case attributes just have lower expressive power than a method on the class; what are your thoughts on this?
In the project I am contributing to, the fixtureMethod
was used with an additional "configuration" field which controlled its result, so that different test cases could have different fixtures. I found that confusing and hard to read as inheritance played a role too. Then came the "need" to have different fixture sets in different tests of the same test case. My first approach was to add an optional parameter $fixtures
to the test method with the fixture definition as the default value. I read this default value with reflection, similar to the attribute approach. But the head of the project didn't like the idea as it might cause a future incompatibility if codeception or PhpUnit one day will use that parameter. Additionally, it became a complex configuration if you wanted to overwrite the class level fixtures. So the idea came to use attributes instead. There is no potential for conflict due to the namespace, it's close to the method. So far as how I came to this.
Looking at it now, with your concerns, I see advantages and disadvantages.
fixtureMethod
is run before the setUp()
method and hence the latter can already draw on this data (e.g. initializing system wide object, that depend on the loaded data set). This would also apply to fixtures defined with attributes.$I
in units)setUp()
method for data initialization, and not the class' attributes. (IMO on method level I'd rather look in annotations and attributes for test setup configuration, rather than in the test itself. E.g. for me, your example would not suggest to me, that data is set up with $project = $I->haveProject();
Of course, if we would have something like $i->prepareFixtures()
that would become obvious.)Sorry for the long text.
Sorry for the long text.
Don't apologize for that!
So assuming we agree that on class level it might not make a big difference.
Regarding my use of $I->haveProject()
, this is actually syntax I borrowed from existing helpers:
$this->tester->haveRecord('app/model/User', ['username' => 'davert']);
// load fixtures
$this->tester->haveFixtures([
'user' => [
'class' => UserFixture::className(),
// fixture data located in tests/_data/user.php
'dataFile' => codecept_data_dir() . 'user.php'
]
]);
// get first user from fixtures
$this->tester->grabFixture('user', 0);
Turns out that there is already a helper for initializing fixtures.. Anyway, this also has to do with the fact attributes are new and I don't have a very developed intuition of what is good and what isn't. Can you explain why the above method(s), documented here: https://codeception.com/for/yii do not satisfy your requirements? The fixtures are defined in arrays and you can easily make sets of fixtures and compose them.
The attributes could of course be simple shortcuts for calling these functions before the test and therefore already have well defined and documented behavior.
Your thoughts? If you still think it's good to have them I won't oppose that further, I don't think they add something that cannot already be done, but no one is forcing me to use them so that's acceptable for me.
Sorry for the long text.
Don't apologize for that!
Thank you. I guess I just appreciate everyone's time and sometimes its a balance to find a good middle-way to give enough but not too much context ...
So assuming we agree that on class level it might not make a big difference. Regarding my use of
$I->haveProject()
, this is actually syntax I borrowed from existing helpers:$this->tester->haveRecord('app/model/User', ['username' => 'davert']); // load fixtures $this->tester->haveFixtures([ 'user' => [ 'class' => UserFixture::className(), // fixture data located in tests/_data/user.php 'dataFile' => codecept_data_dir() . 'user.php' ] ]); // get first user from fixtures $this->tester->grabFixture('user', 0);
Turns out that there is already a helper for initializing fixtures.
I see. I was totally unaware of this feature and so far it has not been used in our project ...! Thank you for the hint!
Anyway, this also has to do with the fact attributes are new and I don't have a very developed intuition of what is good and what isn't. Can you explain why the above method(s), documented here: https://codeception.com/for/yii do not satisfy your requirements? The fixtures are defined in arrays and you can easily make sets of fixtures and compose them.
I guess the only thing here is - as mentioned above - that it helps us if the fixtures are ready before setUp()
(as with _fixtures()
) but paired with the ability to configure them per-test-method. The rest is, as you correctly say just convenience:
The attributes could of course be simple shortcuts for calling these functions before the test and therefore already have well defined and documented behavior.
Your thoughts? If you still think it's good to have them I won't oppose that further, I don't think they add something that cannot already be done, but no one is forcing me to use them so that's acceptable for me.
I do appreciate your critical questions as they trigger this conversation which gives me (or us both) more insights. I will check internally how an implementation with the current functionality could look like for our project. Maybe also with the creation of helpers. And maybe this whole thing was just the long way around the barn ... :-)
@samdark Hm... I'd love fixtures that are specific for a test or use-case.
What makes me wonder: if there is already a way to have per-test fixtures, what lead @samdark to his statement?
Did we miss something?
Well, the codeception project has few maintainers and the yii2 module even fewer. I've mostly rewritten it from scratch over the past few years, but I don't use fixtures at all. So it could just be that he isn't acutely aware of all features...
In general I've found little need to use fixtures, they are almost always either a minor optimization or just an indication that your code is too closely coupled to the AR layer. At least for unit tests this holds. For functional tests where you imitate a request response cycle the hassle of fixtures for me is not worth the few ms I save on running the tests with real queries, also it costs me something in the sense that I'm actually no longer testing the queries..
I see. Well done for this amazing work. Thank you!
Not sure what you mean by "AR layer" (Active Record?). Hence, not sure I fully understand your argumentation.
We have a permission manager (PM) component. As far as I understand unit testing, we test if the PM's methods return the correct result. But this depends on
So the fixtures help us to set up the database in an initial state (users, groups, memberships, configuration). And now I unit test the PM.
I can see that the few amendments I do before the test (e.g. changing the system-wide settings) could be done either with the envisioned attribute or within the test as it is your approach. However, I would not want to have the setup of the fixtures in the test. That's not what I want to test here. What I want to test is: is result is correct, cache and db updated and so on. So yes, I'm testing the queries involved with the current task at hand. But the queries to actually create and manage user, groups and settings I unit test in their respective modules, but not while testing the PM.
But then the PM has some methods, that don't do db access. So I don't need the fixtures to be loaded. Disabling them saves significant amount of time during the test. And when testing an entire application, it does impact productivity if it's taking 10 minutes or 30 minutes, particularly, if those 20 minutes are loading and unloading fixtures that I don't need. :-) But this currently requires a complex setup with fixtureMethod
, as it's not easy to customize the fixtures for a specific text right now.
For me it's not about saving some ms, but about the manageability of the fixtures (and through them the default/actual initial state of the database) and that the data is ready during setUp()
which may then instantiate certain components - or not - depending also on the specific test to run.
Setting up everything in setUp()
would require, that $this->tester
is already available, or that the haveFixtures()
method is available in another easy way. But eventually it is the same issue as with _fixtures()
: there is currently no easy way to configure fixtures for a specific test.
So yes, I might be missing something. I know the lack of knowledge can have detrimental effect here. :-) But right now the attribute approach seems to be an elegant solution. Also, as attributes are already used. In fact, this might be an alternative approach:
#[\Codeception\Attribute\Prepare('someTestFixtures')]
public function someTest()
{
// ...
}
protected function someTestFixtures()
{
// set up fixtures
}
I need to try that. But I need to upgrade to a more recent version of Codeception first.
Not sure what you mean by "AR layer" (Active Record?). Hence, not sure I fully understand your argumentation.
Yes, ActiveRecord
.
But this depends on...
This is what makes it hard to test such things in unit tests. Dependencies for an object should be specified in its constructor, which makes it easy to set it up for your unit test.
In your case, as you mention it depends on a lot of things, like logged in user. In the Yii2 way, you'd probably get that via the global service locator: \Yii::$app->user->identity
. So now this object depends on some global state that you:
In your PM component, you should inject via constructor injection the things it needs to do what it needs to do:
class PM {
public function __construct(private readonly CacheInterface $cache, private PermissionStoreInterface $store) {
}
public function grant(Someone $user, SomePermission $permission): void {
}
}
This could be unit tested by injecting mock implementations and verify they get called correctly. If you internal to this component use AR models, and you load them based on your knowledge of the implementation your tests become brittle. The component has a public API and as long as it doens't change your tests shouldn't be breaking.
haveFixtures()
method is available in another easy way.
Isn't it though?
class SomeCest {
public function testSomething(FunctionalTester $I)
{
// Inline definitions, most readable if short, but not reusable.
$I->haveFixtures([]);
// File local definitions
$I->haveFixtures($this->fixturesForSomething());
// or alternative syntax
$this->haveFixturesForSomething($I);
// Static reusable definitions
$I->haveFixtures(SomethingFixture::all());
$subject = new PermissionManager(...$deps);
$subject->grant($a, $b);
$I->assertSomething(...);
}
}
I don't mind at all this discussion by the way, at the end of it maybe I'll reconsider my stance on fixtures, my lack of knowledge on them is probably also affecting the discussion!
In your PM component, you should inject via constructor injection the things it needs to do what it needs to do:
class PM { public function __construct(private readonly CacheInterface $cache, private PermissionStoreInterface $store) { } public function grant(Someone $user, SomePermission $permission): void { } }
Thank you for this outstanding example. I guess that would be an ideal implementation. As such, testing would inform design patterns.
Unfortunately, real-world and ideal-world diverge quite significantly at times - or most of the times if we not only look at coding ;-)
So the question here would be: are we willing to add some functionality to support non-ideal testing and design patterns, just because in real world they exist. From a psychological stand point, we should not do it, as it does not face devs with the way the code and reconsider. From a practical standpoint, it is simply unrealistic to migrate an non-ideal code base into an ideal code base just for testing. Particularly, as you would want to know that such a transition does not change the actual functionality/result.
So while I start understanding your design pattern for tests (particularly with the example below) I do still think it would be great to have the possibility to test non-ideal code with the use of fixtures.
class SomeCest { public function testSomething(FunctionalTester $I) { // Inline definitions, most readable if short, but not reusable. $I->haveFixtures([]); // File local definitions $I->haveFixtures($this->fixturesForSomething()); // or alternative syntax $this->haveFixturesForSomething($I); // Static reusable definitions $I->haveFixtures(SomethingFixture::all()); $subject = new PermissionManager(...$deps); $subject->grant($a, $b); $I->assertSomething(...); } }
Very nice, thank you. This is a really good illustration of your approach. I like it.
Now, given that code is less-than-optimal in many real-world projects, I'd like to come back to the initial challenge of having
Let's assume I set up some fixtures in fixtureMethod
, I can then extend/update the fixtures in the test method. However, I can't reduce the fixtures-to-be-loaded anymore. (Yes, I can unload them, if I'd really wanted to, but there's no time gain in this - in terms of test execution time):
public class someTest extends \Codeception\Test\Unit {
public function _fixtures() {
return SomethingFixture::all();
}
public function test(){
// adding additional fixtures is easily possible
$this->tester->haveFixtures($this->fixturesForSomething());
}
}
In that sense, the attribute approach would allow to complete and consolidate the different fixtures from different places (configuration > class > method), and then, once the set is clear (after evaluating all attributes) the final set is loaded.
So, I see two main advantages of fixtures configured in attributes over fixtures configured in fixtureMethod:
$this->tester->haveFixtures($this->fixturesDefault());
line in all tests, apart from the one where you'd want $this->tester->haveFixtures($this->fixturesSpecialSubset());
Yii::$app
) is instantiated and before the setUp()
method. So common setup tasks could still draw on the existence of the fixtures. Here I don't see an alternative, as loading them in setUp()
itself would require some other way of getting a configuration from the test method. And loading them in the test is too late. Well, the alternative would be, not to use the setUp()
but mySetup()
and call that too from every test after the fixtures are loaded.However, this approach would lead to different solutions for every project implementing the codeception yii module. Having a standard approach would be nicer.
If we drop the attribute approach, something would still be great: a mechanism to prevent the fixtures configured by configuration to prevent from loading. Maybe a simple #[NoConfigurationFixtures]
on class level or otherwise a static field that can be set, or a method that can be called in the setUpBeforeClass()
?
haveFixtures()
method is available in another easy way.Isn't it though?
I was referring to the setUp()
method. There I can't use the FunctionalTester $I
injection.
So it could just be that he isn't acutely aware of all features...
Yep. I'm still using legacy Codeception version in a few legacy projects. Had no chance to upgrade yet.
Note: I may not be aware of all configuration options or available functionality, so please let me know if I'm missing something.
AFIK, there is the option to configure fixtures in the test configuration, as well as in the configured
fixturesMethod
(_fixtures
).In our project, humhub/humhub, we use the
fixturesMethod
to adjust the configured fixtures from the configuration based on the test case's need. Mainly to speed up tests that do not require db data or just a subset of it.In some situations, however, it seems to make sense to collect several tests in one case, but only some of them do actually need the fixtures, or some need a different set.
Now we had the idea to use attributes to allow that fixtures to be configured: see examples and current suggested implementation (open for improvement).
The question of this issue is if there is interest that we would incorporate this feature into this project here, potentially in the
loadFixtures()
method. The logic could befixturesMethod
is implemented, just use its resultIf the new functionality is encapsulated in a public method (e.g.
getTestFixtures()
) and we would pass the yii2 module instance as an argument to thefixturesMethod
for easy access, thefixturesMethod
could still use that configuration and customize as required. By this, there would be full backwards-compatibility while supporting fixture configuration without the need to implement thefixturesMethod
.The annotations generally allow the following:
FixturesNone
would simply disable any fixtures for the current test.Thank you for considering this. Any feedback welcome.
Please note, the current code in the aforementioned PR is not designed to meet this project's guidelines. Happy to adapt accordingly if you'd be interested in considering an implementation in your code base.