yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.24k stars 6.9k forks source link

Fixtures initializer #1956

Closed Ragazzo closed 10 years ago

Ragazzo commented 10 years ago

Currently Yii2 support only one init plain file that will be included before fixtures load. This is good but not enough. For some different cases we may need special initializers per each fixture that will take care of creating triggers, views, maybe adding/deleting fk and other, so to avoid copy-paste between setUp methods we need some class-initializer. For example:


use yii\test\fixture\Initializer;

class UsersInitializer extends Initializer
{
     public function run()
     {
          $this->createTable(); 
          $this->addForeignKey();
          .....
     }
}

After initializer call fixture file will be loaded if it is exists. So ower goals after all:

  1. ability to reuse Initializer in multiple setUp() methods of tests; Avoid copy-paste of init. code in special testing methods (env. creation like triggers, tables, fk and constraints should not be in setUp of test-case, and we should avoid this copy-paste between different test-cases).
  2. add some additional abilitites that will be needed in most cases (user can use initializer to create needed env. fo fixture: tables, fk, triggers, other constraints). Provide migration-like API for working with DB schema;
  3. speed-up database tests, by creating only needed things and not all schema with all tables, etc;
Ragazzo commented 10 years ago

Will work on this one and make PR :)

Ragazzo commented 10 years ago

And as a reminder for myself: add two events before/after fixtures load. p.s. add ability to use initializers via db-fixture-manager (like it is done for models/rows) with method getInitializer($fixtureName). Because it could be that user may need there some code that should be executed in tearDown() like the opposite of the run() method actions.

Ragazzo commented 10 years ago

well after some thinking, i guess i need core developers opinions here. @cebe @samdark @qiangxue whats your thoughts on this one? Provided class will be Initializer with two methods setUp/tearDown but seems it like more standalone class and only have some integration with DbFixtureManager::getInitializer method i doubt should we add this class or not? Yes all benefits that i wrote above are counted, but is it really worth? developer can create such file buy himself, should we provide this base class to add ability to extend it, or not?

samdark commented 10 years ago

I'm for implementing it.

qiangxue commented 10 years ago

Do we really need this? Note that each fixture is a PHP file. You can put code there to do initialization work. For global initialization work, do it in init.php.

Ragazzo commented 10 years ago

Not that each fixture is a PHP file

currently it is so, for table fixtures, they are simple plain php-files, and end-user may need (i am sure about this) to customize some database env. setup.

For global initialization work, do it in init.php.

yes, but we are not talking about global initialization about partial, see point 3 in the list.

qiangxue commented 10 years ago

What is the difference between using plain php-file and Initializer to customize some database env. setup? Basically my question remains: do we really need to introduce a whole dimension called initializers?

Ragazzo commented 10 years ago

What is the difference between using plain php-file and Initializer to customize some database env. setup?

how will you do this with plain php-fixtures files? Example can be like in yii2-advanced, need to tests Users model, it hits database. But you dont need to load all dump, just one small piece. how will you do this? also take into account that fixtures files will be generated automatically by faker so you cant write in them something like Yii::$app->db->createCommand().... And if you will need again this database env. in other test that uses user-fixtures how will you handle it? if copy-paste then see point 1 in list.

Basically my question remains: do we really need to introduce a whole dimension called initializers?

not a big one to be true. There also will be db_cleaner port of ruby gem. Also small but useful.

qiangxue commented 10 years ago

Alright, I'm convinced. Thanks for explaining.

Now I have some questions about the initializer design.

  1. How many initializers can a fixture have? one or multiple?
  2. How will getInitializer() be implemented?

And also a wild thought: How about we introducing Fixture classes like AssetBundle classes?

Ragazzo commented 10 years ago

How many initializers can a fixture have? one or multiple?

only one.

How will getInitializer() be implemented?

very similar to getModels/getRows. We only need to check if the file with fixtureName+initializerSuffix exists and simply create it. Then run it method start() in DbTestCase in setUp and run method clean in DbTestCase in tearDown. We will know what initializer to create by analyzing property public $fixtures of the DbTestCase. I guess when i submit PR it will be easy to review/understand/suggest.

And also a wild thought: How about we introducing Fixture classes like AssetBundle classes?

i think thats a great idea, some workaround and features can be added in this area.

Fixture classes may have dependencies (like asset bundles).

yes, cool, but we already have public $fixtures also need to note that having dependecies between fixtures should be handled carefully. There should not be only 10 fixtures for all tests in project. Each more or less independent feature should use its own fixtures if it is possible, for example: modules or some components.

Fixture classes can load and provide fixture data

need more talks and suggestions here. I think after basic implementation of initializer we will return to this.

Fixture classes also serve as the initializers as you described above

yep, after implementing initializers we will have more or less understanding where to move on.

Tests load fixtures by something like: $this->requireFixtures(...fixture class names...)

well, we have public $fixtures in DbTestCase + DbFixtureManager. But ofcourse can be discussed after some basic implementation.

Also keep in mind that we have this ticket - https://github.com/yiisoft/yii2/issues/1521 that also will give us good abilities for database clean-up and clean-up strategies. You can see same-named ruby gem for more info.

qiangxue commented 10 years ago

we have public $fixtures in DbTestCase + DbFixtureManager

where is it?

My idea was actually redesigning the whole fixture stuff.

Ragazzo commented 10 years ago

we have public $fixtures in DbTestCase + DbFixtureManager

currently in my mind)) because code is not written yet.

well i like all your suggestions but i dont think that currently we will have time on them. for now we need to introduce to the developers some useful tools. After we have something working we can make some redesigning and other to see if it will work or not, but we already will have some fallback implemented.

Ragazzo commented 10 years ago

And we also should not end up with something big and over-abstracted-layered like DbUnit or other things.

Ragazzo commented 10 years ago

@qiangxue also would be interesting to know your opinion on https://github.com/cakephp/cakephp/tree/master/lib/Cake/Test/Fixture as i see you are thinking of something looks a like.

qiangxue commented 10 years ago

Thanks for the pointer. Will look into it. FYI, I'm already starting working on this. I think we don't need to waste time in developing a temporary solution. We still have time and this new design isn't very complicated.

Ragazzo commented 10 years ago

hm, good to know that you are working on it, but as an end-user i would like to point that it should be very easy to setup and use. Because end-user dont want to manage all this deps and configs he only want it work asap, it was one of the main reasons why i like Yii1 fixtures - very simple to write, very simple to use and understand.

P.S. can you also assign this issue to yourself then?

qiangxue commented 10 years ago

Sure. I also want to keep simplicity. Below is a use example (not finalized yet). Your suggestions are welcome.

class UserFixture extends DbFixture
{
    public $modelClass = 'app\models\User';

    public function loadData()
    {
        return [...]; // or require external data file
    }
}

class MyTestCase
{
    protected function setUp()
    {
        $this->getFixtureManager()->load([
            UserFixture::className(),
        ]);
    }

    protected function tearDown()
    {
        $this->getFixtureManager()->unload();
    }

    public function testSave()
    {
        $user1 = UserFixture::getModel('user1');
        $rows = UserFixture::getRows();
    }
}
Ragazzo commented 10 years ago

hm, i like it, but i think that in fixture class loadData sounds not so good. i also would like to have public $fixtures in the MyTestCase extends DbTestCase, rather then each time writing load-unload.

Also is there some code about how it will load plain-php files? I think that after you will end up your work we can add/adjust fixture/generate command to generate also this classes, because they will be very similar. Maybe adjust not yii2-faker (improving faker command) and just adding this command to yii\console\FixtureController.

And i think that Fixture should implemnt get() and set() as in Yii1 DbTestCase where () was for model and [] was for row. or maybe make some shor-cut to not use always $neededRow = UserFixture::getRows('myrow');

qiangxue commented 10 years ago

Yes, adding $fixtures is fine. It would require us to provide DbTestCase base class though. Right now I'm struggling to determine how to pass some critical information to the fixture objects (such as db connection). Note that here we are talking about traditional databases, but we should also consider nosql databases and other test fixtures as well.

Ragazzo commented 10 years ago

(such as db connection)

i guess they can be taken in DbFixtureManager::load(), just pass them to fixture new $fixtureClass(['db' => $this->db]). Like in createCommand for db-connection.

Note that here we are talking about traditional databases, but we should also consider nosql databases and other test fixtures as well.

true, thats why i was considering to invite here @cebe and @klimov-paul , but looks like they are busy enough now. I will also need their help in this issue.

qiangxue commented 10 years ago

If an application needs two different DBs (one mysql, the other sqlite or redis), what kind of fixture manager should we use? one manager or two?

Ragazzo commented 10 years ago

i guess that db could be an array also. Or if needed user can explicitly set db property of the DbFixtureManager in setUp and then bring it back in tearDown, or third approach:

public $fixtures = [
   'tbl_user' => [
       'db' => 'some_my_connection'
   ],
];

since fixtures are objects now. But need to deal with aliasing i guess, like in current DbFixtureManager::load() when $fixtures can be in different ways: simple array, array of arrays, etc (https://github.com/yiisoft/yii2/blob/master/framework/test/DbTestTrait.php#L69).

qiangxue commented 10 years ago

Well, my plan is that $fixtures only takes configuration of fixture classes. All fixture data must be loaded through the corresponding fixture classes.

Ragazzo commented 10 years ago

Well, my plan is that $fixtures only takes configuration of fixture classes

hm, but the third approach is just about, no?

qiangxue commented 10 years ago

What's the third approach?

Ragazzo commented 10 years ago
public $fixtures = [
   'tbl_user' => [
       'db' => 'some_my_connection'
   ],
];

Well, my plan is that $fixtures only takes configuration of fixture classes.

or i misunderstand something?)

qiangxue commented 10 years ago

tbl_user should be app\test\fixtures\UserFixture. Yes, configuration like this is supported.

Ragazzo commented 10 years ago

so, whats the problem then?) if we can explicitly set db to the fixture if needed, like:

public $fixtures = [
   'tbl_user' => [
       'db' => 'mongo_node_2'
   ],
];

tbl_user should be app\test\fixtures\UserFixture

well, i think that if class is not set then it can be done by simple usage of StringHelper methods to get needed name for class-name.

qiangxue commented 10 years ago

What does fixture manager do? What about disabling/enabling integrity checks? what about loading global init scripts?

Ragazzo commented 10 years ago

What about disabling/enabling integrity checks? what about loading global init scripts?

yes, that what he should do i guess, disable integrity checks before loading all data. Same work, but just with correction that fixtures are now objects.

qiangxue commented 10 years ago

What if we need multiple DBs? How many fixture managers do we need? Or do we need fixture manager at all? The situation here seems to be similar to ActiveRecord which doesn't need things like ActiveRecordManager.

Ragazzo commented 10 years ago

What if we need multiple DBs? How many fixture managers do we need? Or do we need fixture manager at all?

but we can define db connection in DbTestCase in $fixtures.

Or do we need fixture manager at all?

i guess yes, it will call all fixtures methods and disable/enable integrity checks + init. script.

qiangxue commented 10 years ago

Disabling/enabling integrity checks requires DB connection. How to configure that? And if we have different kinds of DBs (mysql and redis, for example), should we have a single fixture manager or multiple?

Ragazzo commented 10 years ago

Disabling/enabling integrity checks requires DB connection

afaik, all redis/mongo/elastic have Connection class.

And if we have different kinds of DBs (mysql and redis, for example), should we have a single fixture manager or multiple?

hm, i guess one, but for disabling/enabling integrity checks it should first group fixtures according to what category they belongs and then disable/enable integrity for current groups, example

$differentFixtures = ...

foreach ($differentFixtures as $n=>$f) {
   // $this->_fixtures[:group] = $this->determineFixtureConnection($f);
}

....
foreach($this->fixtures as $group=>$fixtures) {
//disable integrity
//load fixtures
//enable integrity
}

or we may just put enable/disable integrity checks in Fixture class, and just leave fixture-manager as requiring init.php file.

Ragazzo commented 10 years ago

@qiangxue i also think that Fixture classes should have following two properties:

  1. createSchema - if need to create schema-data (tables, views, triggers,etc)
  2. populateData - if need to populate tables with data.

Why is this needed? Well because of situations when developer dont want to populate data and just want to make some test with tables without data.

Why having createSchema - same case, user may have his own unique created already db-schema, and just want to load data without re-creating. This also will help us to manage changes in FixtureController because its more about loading data to already created tables rather then recreating tables from bottom. So the only dirty hack in FixtureController::apply will be like

//set here for each fixture property `createSchema` = false;
...
$this->dbFixtureManager->load($fixtures);

So overall Fixture class method setUp (i think it should be named so by other tools special methods conventions) will be like:

public function setUp()
{
     //disable integrity check
     if ($this->createSchema) {
          $this->createSchema();
     }
     if ($this->populateData) {
          $this->populateData();
     }
    //enable integrity check
}

and yes, fixtures dependencies will be good feature :) but we should provide good docs and examples how to use it to avoid overusing and creating complex dependencies.

qiangxue commented 10 years ago

A rough design at https://gist.github.com/qiangxue/89ab1d196570dcebd1ef

Explanations:

Define a DB fixture

The actual API is TBD.

class UserFixture extends DbFixture
{
    public $modelClass = 'app\models\User';

    protected function loadData($table)
    {
        return [...data...];
        // or return from external data file
        return require(__DIR__ . '/' . $table->name . '.php');
    }
}

Using fixtures

class MyTestCase extends TestCase
{
    public function fixtures()
    {
        return [
            // anonymous fixture
            PostFixture::className(),
            // "users" fixture
            'users' => UserFixture::className(),
            // "cache" fixture with configuration
            'cache' => [
                'class' => CacheFixture::className(),
                'host' => 'xxx',
            ],
        ];
    }

    public function testSave()
    {
        $user = $this->getFixture('users')->getModel('user1');
        $rows = $this->getFixture('users')->getRows();
        // or support magic property like the following:
        $user = $this->users->getModel('user1');
    }
}
Ragazzo commented 10 years ago

A fixture can have dependencies. A dependent fixture will be loaded BEFORE a depending fixture, and unloaded AFTER a depending fixture.

hm, but that a very critical question also, because of some dependent fixtures could be loaded also after the fixture, example: UsersFixture loaded first, and then UsersProfilesFixture. So i think that depends property should be split into two: before and after - according to load (reverse it would be for unload). If there is no before or after then fixtures are counting as before (same as you suggested above).

There is no more init script. It is replaced by a global fixture class which is depended by other fixtures. How to specify this more easily?

hm, not sure but i guess that property initScript or something like this will work fine. It will be in base fixture class and will work like template-method for other fixtures, because of they are extended. Smth like this:

class Fixture extends Component
{
    public $initScript;

    public function setUp()
    {
        require(Yii::getAlias($this->initScript));
    }
}

class UsersFixture extends Fixture
{
    public function setUp()
    {
        parent::setUp();
        //do your code here
    }
}

DB Integrity check is toggled in the global fixture class.

k, but would be great to see it in PR later)

Define a DB fixture

currently dont like methods:

       $user = $this->getFixture('users')->getModel('user1');
        $rows = $this->getFixture('users')->getRows();
        // or support magic property like the following:
        $user = $this->users->getModel('user1');

can we add method same-named as fixtures on the fly to the test-case class?

$user = $this->user('user1'); //returns AR model
$user = $this->user['user1']; //returns fixture array element

that would be more readable and easy for end user as i think.

cebe commented 10 years ago

And also a wild thought: How about we introducing Fixture classes like AssetBundle classes?

I really like this idea. Your design of simple load and unload looks useable for any kind of db which is nice for nosql. Had no time to read the whole discussion here but it looks like it is going into the right direction :-)

qiangxue commented 10 years ago

Finished fixture classes: https://github.com/yiisoft/yii2/compare/master...feature-fixture Word to be done next: either TestTrait or a base test class that supports loading and using fixtures. Your comments are welcome!

Ragazzo commented 10 years ago

you merged it on master? O_O

qiangxue commented 10 years ago

No, it's on a feature branch.

Ragazzo commented 10 years ago
  1. Ok, i dont like having two different fixtures: AR and simple DB. I think we just should have one DB fixture that will load AR and provide simple array if needed, like in Yii1 fixture-manager is done via call() and get/__set. I also think that we need in TestCase to provide magic methods to search for data in fixtures like i described above:
$user = $this->user('user1'); //returns AR model
$user = $this->user['user1']; //returns fixture array element
  1. Also see my comment https://github.com/yiisoft/yii2/issues/1956#issuecomment-32444997 about before/after.
  2. I think that in Fixture class since its only about providing schema and loading data, we should introduce property dataPath - path to fixtures plain-files. Because of in all cases users will have only one initializers (fixture-classes) per table and only will be switching content - plain files.
  3. https://github.com/yiisoft/yii2/issues/1956#issuecomment-32433311 This also should be taken into account.
qiangxue commented 10 years ago

Thank you for your comments.

i dont like having two different fixtures: AR and simple DB.

In most cases, you only deal with ActiveFixture. DbFixture is provided so that it can perform some global fixture setup work, such as toggling integrity check. Because ActiveFixture depends on DbFixture, it will be loaded automatically.

I also think that we need in TestCase to provide magic methods to search for data in fixtures like i described above:

Yes, this is the TBD task. Do you think a base test case class is better or a trait is better? If the former, we need to do this in codeception extension. If the latter, it can be kept in framework and potentially can be used by other testing frameworks.

Also see my comment #1956 (comment) about before/after.

I don't think this is needed, at least your example doesn't explain the necessity for this: UserProfileFixture should depend on UserFixture, not the other way around.

I think that in Fixture class since its only about providing schema and loading data, we should introduce property dataPath - path to fixtures plain-files. Because of in all cases users will have only one initializers (fixture-classes) per table and only will be switching content - plain files.

Make sense. Will add it.

1956 (comment) This also should be taken into account.

Will consider adding schema and data methods. I used name load()/unload() instead of setUp()/tearDown() because the former makes more sense and simpler. It also won't confuse users into thinking that they are writing a test case.

Ragazzo commented 10 years ago

In most cases, you only deal with ActiveFixture. DbFixture is provided so that it can perform some global fixture setup work, such as toggling integrity check. Because ActiveFixture depends on DbFixture, it will be loaded automatically.

yes, but to be true in most cases i dont need AR set-up, it would be like user will do AR::find($conditions) and then compare one needed ar to the needed fields (array of needed fixture), rather then dealing with all AR.

Also what about that methods that i described (magic methods for getting fixture data)?

I don't think this is needed, at least your example doesn't explain the necessity for this: UserProfileFixture should depend on UserFixture, not the other way around.

hm, but UserProfileFixture should be loaded after UserFixture. Example: i testing simple creating user with profile and fetching already created, in this case first should be loaded UserFixture and then UserProfileFixture, because in UserProfileFixture i can make some changes to db-schema on UserFixture data.

Will consider adding schema and data methods. I used name load()/unload() instead of setUp()/tearDown() because the former makes more sense and simpler. It also won't confuse users into thinking that they are writing a test case.

yes, should be added. I am fine with load/unload :)

qiangxue commented 10 years ago

yes, but to be true in most cases i dont need AR set-up,

Do you mean you don't need ActiveFixture? It provides you rows and getModel(). Aren't they what you need?

Also what about that methods that i described?

See my comment in https://github.com/yiisoft/yii2/issues/1956#issuecomment-32578535

but UserProfileFixture should be loaded after UserFixture.

Yes, that's exactly how the dependency makes this happen. UserProfileFixture depends on UserFixture, so UserFixture needs to be loaded first.

Ragazzo commented 10 years ago

.Yes, that's exactly how the dependency makes this happen. UserProfileFixture depends on UserFixture, so UserFixture needs to be loaded first.

yes, but how to do this?

See my comment in #1956 (comment)

hm, i gave an example that this is a good short-cut methods for end developers:

$user = $this->getFixture('user')->getModel('user1');
vs
$user = $this->user('user1');

i guess the second one is much better.

Do you mean you don't need ActiveFixture? It provides you rows and getModel(). Aren't they what you need?

no, i am about that methods, see above.

Ragazzo commented 10 years ago

I also dont know if we need to switch integrity-check each time, maybe just do it in DbTestCase before loading fixtures and after? And we should wrap everything in one transaction i think.

qiangxue commented 10 years ago

but how to do this?

Just set $depends = ['UserFixture']; in UserProfileFixture.

i gave an example that this is a good short-cut methods for end developers:

:) I said this is to be done next.

I also dont know if we need to switch integrity-check each time, maybe just do it in DbTestCase before loading fixtures and after?

This is exactly what the fixture solution supports. If your test case doesn't use any ActiveFixtures, DbFixture will not be loaded and thus no integrity check will be toggled.

Ragazzo commented 10 years ago

:) I said this is to be done next.

sorry, looks like misread, just woke up :)

This is exactly what the fixture solution supports. If your test case doesn't use any ActiveFixtures, DbFixture will not be loaded and thus no integrity check will be toggled.

hm, k, will wait for all other changes implemented))

Just set $depends = ['UserFixture']; in UserProfileFixture.

and if i use in fixtures() method

return [
'users' => UsersFixture::className(),
];

then users-profiles fixture will be loaded after UsersFixture ? And if i will use UsersProfilesFixture only like in example above, then UsersFixture will be loaded first?

qiangxue commented 10 years ago

If you only specify users, only UserFixture will be loaded. If you only specify userProfiles, both UserFixture and UserProfileFixture will be loaded, and the former will be loaded first. If you specify both, then UserFixture will always be loaded before UserProfileFixture because of the dependency.