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

Yii2 basic codeception #1393

Merged
merged 10 commits into from Dec 17, 2013
Merged

Yii2 basic codeception #1393

merged 10 commits into from Dec 17, 2013

Conversation

@Ragazzo
Copy link
Contributor

Ragazzo commented Dec 2, 2013

Yii2-basic boilerplate codeception integration improved. Fixed some entry-points in web folder. Some things TBD.

TBD:

  • Acceptance tests integration and examples
  • Functional tests integration and examples
  • Unit tests integration and examples
  • Standalone extension, which includes: TestCase, BasePage classes.
  • Qiang comment about urls: #1372 (comment). Since we can now refer to Yii::$app in functional and acceptance tests, you can easily create url with Yii::$app->urlManager->createUrl($route, $params).
@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 2, 2013

also related with Codeception/Codeception#728 and #1372

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 2, 2013

Ok, @qiangxue @cebe @samdark @schmunk42 @DavertMik lets continue in this PR because of it easy to review and suggest some other things. The list of things that must be done:

  1. Currently unit-tests can not be run because of unit/_bootstrap.php error (see this issue Codeception/Codeception#728). That must be fixed before we will merge this one, so it is more for @DavertMik but ofcourse we will help. I've tested unit-tests fixtures by simple moving unit/_bootstrap.php into global codeception ``_bootstrap.php` file, but anyway this bug must be fixed.
  2. Need to make TestCase for unit tests based on good test case for yii itself - https://github.com/yiisoft/yii2/blob/master/tests/unit/TestCase.php
  3. Need to solve @qiangxue described issue #1372 (comment)

maybe some other thoughts/suggestions?

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 2, 2013

Also dont look at changes in *Guy classes because they are generated automatically.

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 2, 2013

@qiangxue i think that maybe we will need a yii2-codeception extension repo, we will put there some basic stuff that can be needed by codeception for Yii, for example BasePage object class. what do you think? or it is not needed? just a suggestion :)

@@ -0,0 +1,3 @@
<?php

return [];

This comment has been minimized.

Copy link
@qiangxue

qiangxue Dec 2, 2013

Member

Why divide test configuration into acceptance.php, functional.php and unit.php? In which case will they differ from each other? Shouldn't DB fixture be needed by all three types of tests?

This comment has been minimized.

Copy link
@Ragazzo

Ragazzo Dec 2, 2013

Author Contributor

well each type of test should be in different db: yii2basic_functional, yii2basic_acceptance, yii2basic_unit (also very good when using CI http://codeception.com/05-24-2013/jenkins-ci-practice.html). Well, in acceptance and functional tests it is not very common to use db fixtures, because of we test like this:

  1. load big dump with demo-data
  2. run all tests.
@@ -1,5 +1,9 @@
<?php

Yii::setAlias('tests', realpath(__DIR__ . '/../tests'));

This comment has been minimized.

Copy link
@qiangxue

qiangxue Dec 2, 2013

Member

I don't think we should define test alias here because web.php is meant for production use.

This comment has been minimized.

Copy link
@Ragazzo

Ragazzo Dec 2, 2013

Author Contributor

right, will put it in web-test.php, ok?

This comment has been minimized.

Copy link
@cebe

cebe Dec 16, 2013

Member

remove alias here :)

@@ -0,0 +1 @@
<?php

This comment has been minimized.

Copy link
@qiangxue

qiangxue Dec 2, 2013

Member

empty?

This comment has been minimized.

Copy link
@Ragazzo

Ragazzo Dec 2, 2013

Author Contributor

yes, this is a global bootstrap file, not needed and empty by default.

{
if (!empty($contactData))
{
$this->guy->fillField($this->name,$contactData['name']);

This comment has been minimized.

Copy link
@qiangxue

qiangxue Dec 2, 2013

Member

shouldn't it test isset($contactData['name'])?

This comment has been minimized.

Copy link
@Ragazzo

Ragazzo Dec 2, 2013

Author Contributor

well, dont know, this method for submiting all data, if user want to submit some part of data he always can do $I->fillField($contactpage->neededField,'neededValue')

config:
PhpBrowser:
url: 'http://localhost/index-test.php'
WebDriver:

This comment has been minimized.

Copy link
@qiangxue

qiangxue Dec 2, 2013

Member

What's the difference between PhpBrowser and WebDriver?

This comment has been minimized.

Copy link
@Ragazzo

Ragazzo Dec 2, 2013

Author Contributor

PhpBrowser only uses curl and dont test javascript and other "in-browser" features, WebDriver does. Also this module is a binding for the new facebook developed api for selenium server.

@schmunk42
Copy link
Contributor

schmunk42 commented Dec 2, 2013

What I'd like to add is that I had some problems from time to time with the *Guy classes which I had to regenerate when I've updated codeception.

Which will be the preferred way of using codeception with Yii?
a) as a dev-dependency in composer
b) global .phar
c) application .phar

Found no info at https://github.com/yiisoft/yii2/blob/master/docs/guide/testing.md

PS: @Ragazzo In general I'd like to see codeception being compatible with the *Guy classes when only updating the version on the patch-level digit. Eg. 1.6.3 is compatible with 1.6.22 but may not necessarily be compatible with 1.7.0.

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 3, 2013

@schmunk42 well i think it will be phar because of it easy use (but phar will not be included in yii2 boilerplates), but ofcourse it all comes up to the end developer and how he want to use it. also what is difference between global and application phar?

In general I'd like to see codeception being compatible with the *Guy classes when only updating the version on the patch-level digit. Eg. 1.6.3 is compatible with 1.6.22 but may not necessarily be compatible with 1.7.0.

well it is more of Codeception architecture question, but i think that there will not be a lot of BC breaks or smth. like that, anyway php codecep.phar build is not a big deal overall ))

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 3, 2013

ok, i solved issues described by @qiangxue in the last review, also added base testcase for unit-tests, and now with this one we no more need to wait while codeception problem (mentioned in pt. 1 in the list) will be solved.

I also think that 2 base classes that will be needed by advanced and basic applications can be moved to yii-codeception extension, those classes are: tests\_helpers\TestCase, tests\_pages\BasePage.

One more suggestion/problem is how to deal when user need to test console application entities? Currently we dont require console.php config as you cans see here Ragazzo@22b9560#diff-dc1419bd66fd2caed1b8827a2e69ebe6R34
So current solution for end user is:

  1. in setUp of hist test he need to make this
protected function setUp()
{
    //will be merged with main config when creating application
    $this->config = require(__DIR__.'/../../../config/console.php');
    parent::setUp();
}

is this a correct way or we need to somehow improve this one? or we can make var $baseConfig in method mockApplication property of the TestCase class, by this we will avoid hardcoding and it will be helpful if we will put those classes in the extensions.

@qiangxue
Copy link
Member

qiangxue commented Dec 5, 2013

Finally got time to review your changes. It looks great to me overall.

Yes, I think we can move TestCase and BasePage to yii/extensions/codeception. If you can think of other classes that are common for testing purpose, you may add them into this folder too.

For console app testing, I think it should be done similarly to testing Web apps.

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 6, 2013

@qiangxue yes, i think i will make a property baseConfig of the TestCase that will be an alias or an array for making yii application with this config, so for console baseConfig will be @app/config/console.php. Also got some ideas for the integration, will do in next few days (currently not have much time).
Thanks for review :)

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 13, 2013

done, created new extension under the extensions\codeception with some basic classes that can be needed by the developers. Also modified composer files, so ones that are very familiar with it can you please recheck that?

One more idea about, not only about codeception integration as more of general tests:

  1. I think we need MigrationCase < TestCase it will be needed for migrations testing: will up and down all migrations in setUpBeforeClass/tearDownAfterClass. This will make it very easy for developers to ensure that migrations works correclty, because first they write test-case (spec) for needed database structure after changes (yii have tableschema/columnschema for that) and then they just run this test. Also very useful to check if migrations successfully applied on the dev server. What do you think about that? Will create separate issue if needed.
@qiangxue
Copy link
Member

qiangxue commented Dec 14, 2013

Cool! Could you merge from master?

I'm not sure if we need migration test. We usually write automated tests for regression purpose to ensure new code won't break existing behavior. How will migration tests be used?

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 14, 2013

Could you merge from master?

how to do this? i thought that yii github rules is to make each PR in different branch.

How will migration tests be used?

main purpose here is to ensure that you have not broke database structure and that migration changes are applied. Because now when you apply migration you need manually to check via some tools (pgAdmin3, phpmyadmin, mysql, psql) that everything is in the place and in this stage you can forget something.
While with migration testcase all you need is just write spec for migration (when you writing spec all you attention is on it and it lows ability to make a mistake) and then just write code for this migration + run test and ensure it is valid, or fix breaks.
For example all migrations will be up in the yii2basic_unit database, its like a sandbox. Main thing here also is that developer does not need to up database to the some migration state and check it, it will be automated. So that is not usual regression test, but very useful :)

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 14, 2013

I also think that we need a better solution for database managing in Yii2, i mean that currently we only have ability to load fixtures in tables, but we dont have tools for creating database structure.
The problem is that in the tests that use fixtures it can take some time to load/unload database dump (especially if there are 20 tables there, not a big number though) and we dont have solutions for this and for creating database structure (and also dont have solution like transaction rollback in tests). Current possible solutions are:

  1. Load all big dump before test suite and then use it, but dont drop tables just reset them (this solution was suggested long time ago by @samdark in chat). Pros and cons: simple implementation, big problems with data collisions especially when have to deal with some triggers and procedures.
  2. Create only needed tables (each table structure represented by separated sql dump file). Pros and cons: not so simple to implement (have one solution though), no troubles with data collisions, triggers, procedures, sequences. So in test case user can select which tables to create by specifyingpublic $tables = array();

I think that we definitely need to improve this to make tdd very native and easy in Yii2. @qiangxue what do you think? should i create a separated issue for that and discuss it there?

@samdark
Copy link
Member

samdark commented Dec 14, 2013

Personally I always had separate dump for structure only w/o data and either data dump or data fixtures so before performing tests structure was created/updated and then for each test there was cleanup/data load.

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 14, 2013

ok, will start tread in new issue to not making this one too big.

@Ragazzo Ragazzo mentioned this pull request Dec 14, 2013
0 of 3 tasks complete
@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 14, 2013

dont merge yet, need to write readme, changelog, license files for the extension repo.

@qiangxue
Copy link
Member

qiangxue commented Dec 17, 2013

I just reviewed the extension code (sorry, I should have done this earlier). Below are my comments:

I don't quite like BasePage::$URL as explained in #1372 (comment). Can we replace it with BasePage::$route and BasePage::getUrl($params = [])? We can also drop BasePage::route().

Then we can use the following code in tests:

class AboutPage extends BasePage 
{
    // can be either a string or an array. Use a string to represent a route (controllerID/actionID).
    // Use array($route, $key1=>$value1, $key2=>$value2, ...) to represent a route with parameters.
    public $route = 'site/about';
}

$aboutPage = AboutPage::of($I);
$I->amOnPage($aboutPage->url);

getUrl() will construct a real URL using UrlManager.

Also, how about introducing AboutPage::open($I) which does the of() and amOnPage() calls as shown above?

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 17, 2013

sure, we can introduce open method. can you also show example about getUrl() method or maybe add implementation for it? Anyway we i think we should keep some BC for codeception, because of it may be that codeception will change how it will use page-objects (dont think that it will be, but anyway) and we will be BC-broken.

@DavertMik
Copy link
Contributor

DavertMik commented Dec 17, 2013

@Ragazzo please don't look at me while implementing page objects ) If you feel you are doing it right and solution is nice and clean - just do it. So I think you should follow @qiangxue ideas and if you come to a nice result, I can take your ideas into Codeception 2.0

Right now PageObjects are absolutely standalone classes. They are only generated by Codeception and not used by framework itself. Thus. there are no BC breaks, as this are plain classes. So feel free to implement them for your good.

@qiangxue
Copy link
Member

qiangxue commented Dec 17, 2013

Yes, I think this change doesn't affect codeception at all since it is only done with the page class.

getUrl() can be implemented as follows,

public function getUrl($params = [])
{
    if (is_string($this->route)) {
        return Yii::$app->getUrlManager()->createUrl($this->route, $params);
    } elseif (is_array($this->route) && isset($this->route[0])) {
        $ps = $this->route;
        $route = $ps[0];
        unset($ps[0]);
        return Yii::$app->getUrlManager()->createUrl($route, array_merge($ps, $params));
    } else {
        throw new Exception("...");
    }
}
@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 17, 2013

well i think i will implement open, but can you implement getUrl() on your own (because maybe you will find some other things that not looks so good, etc)? Also maybe we need then to extend BasePage from Component to use getters/setters, no? And maybe create separate issue with 2 tasks (1. open method, 2. getUrl() method) to discuss/implement things there?

@qiangxue
Copy link
Member

qiangxue commented Dec 17, 2013

I can take care of the whole implementation. Just want to discuss with you about the idea to see if I misunderstand anything or the idea is bad.

@qiangxue
Copy link
Member

qiangxue commented Dec 17, 2013

I will finish this today (also create the yii2-codeception subrepo/package).

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 17, 2013

nope, as for me idea is good, since we have all freedom in extending and implementing - anything can be done :) pity that no so much developers are involved (( but anyway we need to provide for Yii2 good testing support (not only codeception integration, maybe some other commands/tools/etc).

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 17, 2013

I will finish this today (also create the yii2-codeception subrepo/package).

great, also if you will have time, maybe take a look at this PR #1527.

@qiangxue
Copy link
Member

qiangxue commented Dec 17, 2013

Will do.

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 17, 2013

ah, one small addition, since you are working on this one, can you take into account this comment #1393 (comment) And move codeception in separated folder (some path-related things should be fixed then of course)?.

@qiangxue
Copy link
Member

qiangxue commented Dec 18, 2013

I made some changes to BasePage and TestCase. Could you help review it?

Should we add some IP address check in index-test-functional.php and index-test-acceptance.php so that inexperienced users won't expose sensitive information after default installation?

@cebe
Copy link
Member

cebe commented Dec 18, 2013

I made some changes to BasePage and TestCase. Could you help review it?

looks good to me. need to adjust the readme file to reflect the changes.

Should we add some IP address check in index-test-functional.php and index-test-acceptance.php so that inexperienced users won't expose sensitive information after default installation?

Yes that should be done imo. But only for the index-test-acceptance.php the functional one only returns the config.

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 18, 2013

Should we add some IP address check in index-test-functional.php and index-test-acceptance.php so that inexperienced users won't expose sensitive information after default installation?

i dont think so, if user will not delete this files on deploy this means that he made a great mistake. This files only for test purposes.

Also @qiangxue after TestCase refactoring we no more can set appConifg to be an alias, i dont like that. Old implementation allowed us to just specify one alias and all other things will be done automatically, now user have to remember that he need to call mockApplication(), also he need to make Yii::getAlias() or require to load needed config, thats too much things to do, i think we should bring old implementation back. BasePage looks good.

And we still need to put codeception tests under tests/codeception folder as i described above.

@cebe
Copy link
Member

cebe commented Dec 18, 2013

  • mockApplication() is not always needed so it is optional to call it.
  • appConfig is set by default in bootstrap file and can be adjusted by manipulating the array. Thats fine imo
@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 18, 2013

mockApplication() is not always needed so it is optional to call it.

i think that creating application instance in setUp() by default is important, so user will not be worried about smth. that he can override, also he dont need to store components before mock. Bring it back please :) Laravel also do this.

appConfig is set by default in bootstrap file and can be adjusted by manipulating the array. Thats fine imo

give me an example for console test. i want to test migration/console command, what should i need to do? where do i need to do that? I just want to point alias to config and expect that everything else will work out of the box, i dont want to to smth. else for the fw, thats not user-friendly.

I vote for everything to be out of the box, while you standing on point that developer need to think/introduce/create something new, not a good point for user.

@qiangxue
Copy link
Member

qiangxue commented Dec 18, 2013

i dont think so, if user will not delete this files on deploy this means that he made a great mistake. This files only for test purposes.

The user should still remove this file on production. But in case he forgets so or doesn't know this, the IP checking will at least provide some security.

after TestCase refactoring we no more can set appConifg to be an alias, i dont like that.

This depends on how TestCase will be used. My thought was that TestCase should NOT care about how the configuration array is generated - it should NOT attempt to load or merge configuration arrays.

i think that creating application instance in setUp() by default is important...

I think it's fine we call mockApplication() by default in setUp().

give me an example for console test.

This depends on how unit tests are organized. My thought was that we can create different base TestCase classes for different application configurations. Application configurations can be set in _bootstrap.php and/or customized in TestCase::init().

This TestCase design is not final yet. Your feedback is very welcome.

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 18, 2013

This depends on how TestCase will be used. My thought was that TestCase should NOT care about how the configuration array is generated - it should NOT attempt to load or merge configuration arrays.

well that was the main idea, to carry about aliases/arrays so developer dont need to think how he need to configure/bootstrap application. he only need to point to one file and thats it. :)

The user should still remove this file on production. But in case he forgets so or doesn't know this, the IP checking will at least provide some security.

but if we will check for 127.0.0.1 some servers like nginx can rewrite headers like x_forwared_for and we will get localhost where it is not so.

This depends on how unit tests are organized. My thought was that we can create different base TestCase classes for different application configurations. Application configurations can be set in _bootstrap.php and/or customized in TestCase::init()

yes, but old one with aliases was pretty good, and for console, user just needed to create console_bootstrap.php where he will return needed array and thats all, very simple. Also we will have some other test-cases DbTestCase (truncation, create/drop, transaction strategies for db-cleanup #1521), MigrateTestCase for testing migrations. But basic TestCase with supporting aliases was good to handle different users configs and customizations, so i think that at list this should be added back.

I think it's fine we call mockApplication() by default in setUp().

sure, so will you revert it back as it was before?

Also need a separate folder)) i think i was annoying with this issue :)

@qiangxue
Copy link
Member

qiangxue commented Dec 18, 2013

Yes, we can add back alias support.

but if we will check for 127.0.0.1 some servers like nginx can rewrite headers like x_forwared_for and we will get localhost where it is not so.

In this case, we should show some error message through which users will know how to fix the problem (he can remove the IP check or fix it. At this stage, we can assume the user is knowledgeable enough.)

so will you revert it back as it was before?

Sure. BTW, your version doesn't have it in setUp() either. ;)

@cebe
Copy link
Member

cebe commented Dec 18, 2013

Sure. BTW, your version doesn't have it in setUp() either. ;)

It was there but I removed it ;)

@qiangxue
Copy link
Member

qiangxue commented Dec 18, 2013

Updated: 3181964

Since mockApplication() is now automatically invoked in setUp(), how will a single child class customize appConfig and/or appClass? Or do we really have such use case?

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 18, 2013

In this case, we should show some error message through which users will know how to fix the problem (he can remove the IP check or fix it. At this stage, we can assume the user is knowledgeable enough.)

not sure if how we can determine that file is on production server and not in dev? or i missed smth?

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 18, 2013

Since mockApplication() is now automatically invoked in setUp(), how will a single child class customize appConfig and/or appClass? Or do we really have such use case?

thats why $config should be a property and not mockApplication param. See early first my commit.

how will a single child class customize appConfig and/or appClass

MyOwnTestCase extends TestCase
{
    public static $appConfig = '@app/path/to/my config';
    public $config = array(
         ///components customize for example
    );
}
@cebe
Copy link
Member

cebe commented Dec 18, 2013

Since mockApplication() is now automatically invoked in setUp(), how will a single child class customize appConfig and/or appClass? Or do we really have such use case?

You can customize application config in setUp():

protected function setUp()
{
      $this->appConfig['components']['whatever'] = [/* ... */]
      parent::setUp() // mocks application
}

config param from mockApplication should be removed.

@qiangxue
Copy link
Member

qiangxue commented Dec 18, 2013

We should drop alias support too then.

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 18, 2013

@cebe but why i need to do it this way if i can do it simply with config property? see first integration commit https://github.com/Ragazzo/yii2/blob/f0d6b560331812db0924e4e31308a599a37d01f9/extensions/codeception/TestCase.php I think it should be back.

We should drop alias support too then.

why? it is useful :)

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 19, 2013

@cebe @qiangxue should i revert (i think yes, because of reasons discussed above) testcase changes (bring back everything like in first commit, see comment above) when now creating MigrateTestCase for migrations, will provide PR shortly?

@qiangxue
Copy link
Member

qiangxue commented Dec 19, 2013

Hold on for a moment. I'm still trying to find a better solution.

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 19, 2013

sure.

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

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.