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

Fixtures initializer #1956

Closed
Ragazzo opened this issue Jan 13, 2014 · 112 comments
Closed

Fixtures initializer #1956

Ragazzo opened this issue Jan 13, 2014 · 112 comments

Comments

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Jan 13, 2014

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
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 13, 2014

Will work on this one and make PR :)

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 13, 2014

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
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 13, 2014

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
Copy link
Member

@samdark samdark commented Jan 14, 2014

I'm for implementing it.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 14, 2014

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
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 14, 2014

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
Copy link
Member

@qiangxue qiangxue commented Jan 14, 2014

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
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 14, 2014

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
Copy link
Member

@qiangxue qiangxue commented Jan 14, 2014

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?

  • Fixture classes may have dependencies (like asset bundles).
  • Fixture classes can load and provide fixture data
  • Fixture classes also serve as the initializers as you described above
  • Tests load fixtures by something like: $this->requireFixtures(...fixture class names...)
@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 14, 2014

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 - #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
Copy link
Member

@qiangxue qiangxue commented Jan 14, 2014

we have public $fixtures in DbTestCase + DbFixtureManager

where is it?

My idea was actually redesigning the whole fixture stuff.

  • The fixture classes will be the central pieces which are managed by FixtureManager (in analogy to asset bundles managed by AssetManager).
  • A fixture is referenced via its class name, thanks to class autoloading mechanism which makes it very flexible regarding to how to organize fixtures. Using a fixture is like registering an asset bundle.
  • The concept of fixture dependency will allow designing some "macro" fixtures for testing some big features. For example, to test the admin feature, you may design an AdminFixture which depends on UserFixture, PostFixture, etc.
  • There will be no initializers, since fixtures already cover them.
  • Fixtures can take external data files, like the current fixture files, which can be generated by tools like faker.
  • The concept of fixture can be extended, not limiting to DB fixtures.
@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 14, 2014

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
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 14, 2014

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

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 15, 2014

@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
Copy link
Member

@qiangxue qiangxue commented Jan 15, 2014

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
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 15, 2014

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?

@ghost ghost assigned qiangxue Jan 15, 2014
@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 15, 2014

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
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 15, 2014

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
Copy link
Member

@qiangxue qiangxue commented Jan 15, 2014

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
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 15, 2014

(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
Copy link
Member

@qiangxue qiangxue commented Jan 15, 2014

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
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 15, 2014

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
Copy link
Member

@qiangxue qiangxue commented Jan 15, 2014

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

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 15, 2014

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

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

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 15, 2014

What's the third approach?

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 15, 2014

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
Copy link
Member

@qiangxue qiangxue commented Jan 15, 2014

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

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 15, 2014

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
Copy link
Member

@qiangxue qiangxue commented Jan 15, 2014

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

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 18, 2014

Yes, I'm questioning the need of initSchema (and thus createSchema) because I don't understand the use case (see #1956 (comment))

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 18, 2014

Nope, initSchema is needed definitly see p1,2 in issue description. It will simplify testing in other way problems of Yii1 will not be solved here and all this fixture-objects dont make sense because they are complex if comparing to fixture-manager and dont introduce anything new.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 18, 2014

p1. This is just a schema fixture. If your test needs it, declare it in fixtures() and it will load. No copy-paste needed.
p2. You use fixture command to apply the schema fixtures.

The new fixture design makes a lot of sense because: it generalizes the fixture concept, not just dealing with DB fixtures; it unifies fixture references (how to refer to a fixture); it allows you to organize fixtures better with the dependency concept. Yii1 FixtureManager doesn't handle any of these.

Is it more complex than Yii1 FixtureManager? Absolute not. For example, you can put the fixture data directly within a fixture class rather than in a different file. The result will be that you have the same number of fixture files as in Yii1 but with greater control and flexibility.

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 18, 2014

The new fixture design makes a lot of sense because: it generalizes the fixture concept, not just dealing with DB fixtures; it unifies fixture references (how to refer to a fixture); it allows you to organize fixtures better with the dependency concept. Yii1 FixtureManager doesn't handle any of these.

only talks, no real profit :)

Is it more complex than Yii1 FixtureManager?

yes :) its not complex for you because you developed it.

p1. This is just a schema fixture. If your test needs it, declare it in fixtures() and it will load. No copy-paste needed.

i dont want to create another fixture just for schema. It should be in fixture class of the UserFixture. Thats it.

p2. You use fixture command to apply the schema fixtures.

currently i cant, and i dont want to use it in test, because see above.

@qiangxue i was using fixtures/db-test-case in Yii1 (such a pain) and this features is really needed, i dont get why you still not convinced, and it looks like it will not get anywhere becuase you are core-developer and only see it from your side. Even @samdark said that he is for implementing this, we discussed with him this feature and main thing was about p1,2 and not simple wrapper to load fixtures. So i think a lot of developers (i spoke to some of good russian developers about it) would be dissapointed, and as always Yii will loose opportunity to get good testing support.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 18, 2014

only talks, no real profit :)

If this is your attitude, I will not argue anymore since I have already given docs and examples.

i dont want to create another fixture just for schema.

You want to put it in an initializer class, but not a fixture class. What an argument?

currently i cant, and i dont want to use it in test, because see above.

True, but that's something to be fixed.

i dont get why you still not convinced,

Because you never answered me. See: #1956 (comment)

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 18, 2014

Because you never answered me. See: #1956 (comment)

i answerd you that schema is part of fixture, in this way its more easy to handle testing.

You want to put it in an initializer class, but not a fixture class. What an argument?

argument is p1,2 in issue description.

So overall what is your suggestion? how to handle schema-creating in custom ways? creating +1 schema-fixture for each fixture? bad idea.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 18, 2014

Why do you need a schema fixture for each fixture? Why do you want to repeat what you have done in DB migrations? For example, I want to add a column to the user table. Does this mean I need to add a new DB migration as well as modify the initializer with nearly identical code?

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 18, 2014

Why do you need a schema fixture for each fixture?

because i dont want to make copy-paste of init-schema things (creating tables, fk, etc) each time in different tests where fixture is used.

Why do you want to repeat what you have done in DB migrations?

why are we still talking about migrations? what is the use case of migrations here? no use-case.

For example, I want to add a column to the user table. Does this mean I need to add a new DB migration as well as modify the initializer?

yes, thats why you write your tests not once and forget about it, but also crafting them and working on them during the development time. But once again migrations have nothing to deal with testing (only if we provide MigrateTestCase for testing migrations).

So overall whats your solution, how i should handle custom creating? Example:

class UsersTest extends TestCase
{
    protected function setUp()
    {
            Yii::$app->createCommand()->createTable('users', [...]);
    }
}
class UsersProfilesTest extends TestCase
{
    protected function setUp()
    {
            Yii::$app->createCommand()->createTable('users', [...]);
            Yii::$app->createCommand()->createTable('users_profiles', [...]);
    }
}

So as you see above in example its huge problem of copy-pasting, and it just growing with a big number of tests, so if changed one field in schema, i need to go through all this tests and just fix it rather then simply fixing 1 fixture file. I dont even talk here about some base principles of development as DRY, KISS as you see its all about it.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 18, 2014

yes, thats why you write your tests not once and forget about it, but also crafting them and working on them during the development time. But once again migrations have nothing to deal with testing (only if we provide MigrateTestCase for testing migrations).

Migration is used to adjust your DB schemes along the course of your development. Migrations can be applied to both production/development and test databases. Yes, when you make schema changes, you need to adjust fixture data as well - we're not writing test once and forget about it. But it seems to me pointless and error-prone if I have to modify in two places about the schema with the same change. This is even more problematic in a team development environment if because migration will coordinate simultaneous schema changes while in initializers, you have to manually deal with conflicts.

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 18, 2014

@qiangxue lets not talk about migrations to init. db, ok?)) its so wrong that i even cant describe it. and you also was against it in some issue few days ago.

So what is the solution you are suggesting? Migrations does not count.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 18, 2014

I think I finally understand what you really want to do - you essentially want to represent a database schema in terms of multiple fixtures (plus an init schema) so that you save time when running tests, because you don't need to re-create the whole database each time. Correct?

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 18, 2014

@qiangxue yep, exactly. I was playing with 12 tables (empty tables, just creating structure and dropping them + 2 tables fixtures data) and it took to run 2-3 seconds just 1 test in 1 test-case (simple assertTrue(false)). As you see its a huge time, according to that tests should be run very often.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 18, 2014

What should be in the init schema? Or how to prepare the initial database?

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 18, 2014

What should be in the init schema? Or how to prepare the initial database?

it is the developer choice, we just provide template method for this and thats it. Because he dont need to create all tables just to test 1 AR.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 18, 2014

I'm no longer questioning the necessity of your proposal. I'm convinced.
I'm here asking for suggestions for the missing part - the initial database preparation.

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 18, 2014

I'm here asking for suggestions for the missing part - the initial database preparation.

you already have it: ActiveFixture::initSchema method for particular fixture and DbFixture::initFile for base initial prepartion.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 18, 2014

Should we also add a resetTable flag? Or we should only call resetTable() if schema is not created.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 18, 2014

Ok, updated code/doc based on the above discussions:

Code: master...feature-fixture
Documentation: https://github.com/yiisoft/yii2/blob/feature-fixture/docs/guide/test-fixture.md

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 19, 2014

Should we also add a resetTable flag? Or we should only call resetTable() if schema is not created.

no, it is user choice to decide what he want to reset what he want, in particular we cant guess execatly what is he thinking about in this way. I think resetTable should be removed, because we already have loadSchema.

Also good that dep. of DbFixture by default was removed from ActiveFixture.

So overall it looks fine to me, maybe after some playaround will get some issues/enh. but now its ok.

Also could you then submit fix for FixtureController as we now have loadSchema flag?

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 19, 2014

Should this docs be fixed, or i misread something?

Note that in the above, besides app\tests\fixtures\UserFixture, the dependency of UserProfileFixture also includes yii\test\DbFixture. This is required by all ActiveFixture classes which set yii\test\DbFixture as the default value of the depends property. The DbFixture class is responsible for toggling database integrity check and executing an initialization script. Without DbFixture, you may not be able to freely inserting or deleting rows in a table due to various DB constraints.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 19, 2014

I still kept resetTable() but disable it by default for the reason that some people may still prefer the old way that loads test database dump all at once to save the trouble of writing loadSchema().

Perhaps you can help with the change needed for FixtureController? Note that DbFixtureManager will be removed.

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 19, 2014

I still kept resetTable() but disable it by default for the reason that some people may still prefer the old way that loads test database dump all at once to save the trouble of writing loadSchema().

Ok.

Perhaps you can help with the change needed for FixtureController? Note that DbFixtureManager will be removed.

Right, but i should start work after your PR merged. The only thing will be that i need to search fixture-initializer (fixture-class) by fixture name, and just set loadSchema to false and set fixture-data correct path. So i think your PR can be merged? Great work overall, thank you very much.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 19, 2014

Already merged. Thank you for your critical reviews. It's very helpful.

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 19, 2014

Hope, i was not rude)) Will work on FixtureController.

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 19, 2014

Closing this one as implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.