-
Notifications
You must be signed in to change notification settings - Fork 35
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
[Feature] Per-unit and per-class fixture configuration - PR welcome? #92
Comments
I'm not excited about fixtures in general. 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:
ExamplesDisable fixtures on class-level, but use fixtures from configuration for one test#[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
}
} Define fixtures on class-level, but disable them on one test#[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
}
} Define a default set of fixtures and use that in the test#[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.
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!
Yep, could be an option. The reason why I did it this way, was that most of the logic is encapsulated within
Sounds good to me.
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
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.
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. |
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.
If I get the picture right, the approach with an extension would be to
Assuming 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 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 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. |
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 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? |
Ok.
In the project I am contributing to, the Looking at it now, with your concerns, I see advantages and disadvantages.
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. $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. 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. |
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 ...
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!
I guess the only thing here is - as mentioned above - that it helps us if the fixtures are ready before
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 ... :-)
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 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 Setting up everything in 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. |
Yes,
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 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.
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! |
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.
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 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
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
I was referring to the |
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.
The text was updated successfully, but these errors were encountered: