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

[WIP] Codeception yii2 advanced #1372

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@Ragazzo
Contributor

Ragazzo commented Dec 1, 2013

this is first attempt to integrate codeception tests into yii2-advanced boilerplate. I have some suggestions and questions, will write them in comment to this PR. other developers opinions are very welcome.

P.S. will also add more description about tests into the README.md of the yii2-advanced repo.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Dec 1, 2013

Contributor

I have this problems/suggestions on current implementation:

  1. Would be very good if Codeception will have BasePage class that will have of(), route() methods and static::$url field. This will allow to avoid big code duplication. @DavertMik any news/suggestions on this one?
  2. I was thinking to make integration also for common/console folders but i think that current Yii2 testcase that is used in Yii2 testing by itself is very good and can be used by other developers in this boilerplate too (https://github.com/yiisoft/yii2/blob/master/tests/unit/TestCase.php). What do you think @qiangxue ? does it needed to be also integrated or let the developers to decide this one?
  3. I think that based on this integration, yii2-basic boilerplate can be improved too, @qiangxue what your thoughts about this?
  4. solving this issue Codeception/Codeception#726 will also help to avoid code duplication, for example look in current LoginPage under the main _pages folder and functional/_pages, same to others.
Contributor

Ragazzo commented Dec 1, 2013

I have this problems/suggestions on current implementation:

  1. Would be very good if Codeception will have BasePage class that will have of(), route() methods and static::$url field. This will allow to avoid big code duplication. @DavertMik any news/suggestions on this one?
  2. I was thinking to make integration also for common/console folders but i think that current Yii2 testcase that is used in Yii2 testing by itself is very good and can be used by other developers in this boilerplate too (https://github.com/yiisoft/yii2/blob/master/tests/unit/TestCase.php). What do you think @qiangxue ? does it needed to be also integrated or let the developers to decide this one?
  3. I think that based on this integration, yii2-basic boilerplate can be improved too, @qiangxue what your thoughts about this?
  4. solving this issue Codeception/Codeception#726 will also help to avoid code duplication, for example look in current LoginPage under the main _pages folder and functional/_pages, same to others.
@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Dec 1, 2013

Contributor

other developers opinions are also good to be heard, @samdark, @cebe, @schmunk42?

Contributor

Ragazzo commented Dec 1, 2013

other developers opinions are also good to be heard, @samdark, @cebe, @schmunk42?

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Dec 1, 2013

Member

@Ragazzo Great job! The tests for the basic app should certainly be improved like you did here. In fact, I would rather suggest you to start with the basic app first because it is smaller and easier for us to review and discuss about the overall design.

Besides the improvement you suggested, may I ask for another one: right now in each Cept we use the following code to specify the URL. It would be great if we could also specify it using $route, $params, or creating a URL via urlManager with $route and $params.

$I->amOnPage('?r=site/contact');
Member

qiangxue commented Dec 1, 2013

@Ragazzo Great job! The tests for the basic app should certainly be improved like you did here. In fact, I would rather suggest you to start with the basic app first because it is smaller and easier for us to review and discuss about the overall design.

Besides the improvement you suggested, may I ask for another one: right now in each Cept we use the following code to specify the URL. It would be great if we could also specify it using $route, $params, or creating a URL via urlManager with $route and $params.

$I->amOnPage('?r=site/contact');
@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Dec 1, 2013

Contributor

@qiangxue sure, will start work on yii2-basic tomorrow.

Besides the improvement you suggested, may I ask for another one: right now in each Cept we use the following code to specify the URL. It would be great if we could also specify it using $route, $params, or creating a URL via urlManager with $route and $params.

hm, dont understand to be true, maybe you talking about some page-objects feature like:

$I->amOnPage(LoginPage::$url)

or is it something else and where we need urlmanager?

Contributor

Ragazzo commented Dec 1, 2013

@qiangxue sure, will start work on yii2-basic tomorrow.

Besides the improvement you suggested, may I ask for another one: right now in each Cept we use the following code to specify the URL. It would be great if we could also specify it using $route, $params, or creating a URL via urlManager with $route and $params.

hm, dont understand to be true, maybe you talking about some page-objects feature like:

$I->amOnPage(LoginPage::$url)

or is it something else and where we need urlmanager?

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Dec 1, 2013

Member

How to specify LoginPage::$url?

Currently URL like ?r=site/contact is hardcoded. If we change the URL rules, it could become site/contact or contact. So my suggest was to allow specifying URL using $route and $params.

Member

qiangxue commented Dec 1, 2013

How to specify LoginPage::$url?

Currently URL like ?r=site/contact is hardcoded. If we change the URL rules, it could become site/contact or contact. So my suggest was to allow specifying URL using $route and $params.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Dec 1, 2013

Contributor

How to specify LoginPage::$url?

this can be done in page-object, by default codeception page objects are generated with some methods/properties, see link below to the BasePage and it $URL, route() and other.
https://github.com/Ragazzo/yii2/blob/e8fe433a622139e4645dac52625b52465dd517a5/apps/advanced/frontend/tests/codeception/tests/_pages/BasePage.php

Or we can create some helper but dont know if it worth to do so, also if we switch url-format to path Yii will handle it automatically even with routeVar, no? Yii1 would do that afaik.

Also i what do you think about pt 2 in my first comment? Have some ideas will implement them in yii2-basic app for better review ofcourse.

Contributor

Ragazzo commented Dec 1, 2013

How to specify LoginPage::$url?

this can be done in page-object, by default codeception page objects are generated with some methods/properties, see link below to the BasePage and it $URL, route() and other.
https://github.com/Ragazzo/yii2/blob/e8fe433a622139e4645dac52625b52465dd517a5/apps/advanced/frontend/tests/codeception/tests/_pages/BasePage.php

Or we can create some helper but dont know if it worth to do so, also if we switch url-format to path Yii will handle it automatically even with routeVar, no? Yii1 would do that afaik.

Also i what do you think about pt 2 in my first comment? Have some ideas will implement them in yii2-basic app for better review ofcourse.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Dec 1, 2013

Member

Perhaps we can just use Yii::$app->urlManager->createUrl($route, $params) in the test?

Yes, I think your point 2 is meaningful. The existing TestCase is a good start.

Member

qiangxue commented Dec 1, 2013

Perhaps we can just use Yii::$app->urlManager->createUrl($route, $params) in the test?

Yes, I think your point 2 is meaningful. The existing TestCase is a good start.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Dec 1, 2013

Contributor

Yii::$app->urlManager->createUrl($route, $params)

still dont understand the use-case, sorry( Also if we will do so, then we will need some basic Yii bootstrap integration (like in Yii1 bootstrap file for phpunit, not complicated and easy to do), because of acceptance tests are not using Yii2 module as unit tests too.

Yes, I think your point 2 is meaningful. The existing TestCase is a good start.

ok, will do some workarounds in yii2-basic boilerplate about that.

Contributor

Ragazzo commented Dec 1, 2013

Yii::$app->urlManager->createUrl($route, $params)

still dont understand the use-case, sorry( Also if we will do so, then we will need some basic Yii bootstrap integration (like in Yii1 bootstrap file for phpunit, not complicated and easy to do), because of acceptance tests are not using Yii2 module as unit tests too.

Yes, I think your point 2 is meaningful. The existing TestCase is a good start.

ok, will do some workarounds in yii2-basic boilerplate about that.

@schmunk42

This comment has been minimized.

Show comment
Hide comment
@schmunk42

schmunk42 Dec 1, 2013

Contributor

Perhaps we can just use Yii::$app->urlManager->createUrl($route, $params) in the test?

Yes, that would be really helpful. Currently (Yii 1) I am using format get for all my tests, but we should be able to switch that.

Contributor

schmunk42 commented Dec 1, 2013

Perhaps we can just use Yii::$app->urlManager->createUrl($route, $params) in the test?

Yes, that would be really helpful. Currently (Yii 1) I am using format get for all my tests, but we should be able to switch that.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Dec 1, 2013

Member

Sorry for the confusion. What I meant is that: currently in each page test, we are specifying the URL explicitly. But the URL actually may change if the application changes its URL rules. For example, ?r=site/contact, site/contact, contact could all refer to the same page, depending on the URL rule setting. So I'm proposing to use the pair $route and $params to specify a URL because they don't change.

Member

qiangxue commented Dec 1, 2013

Sorry for the confusion. What I meant is that: currently in each page test, we are specifying the URL explicitly. But the URL actually may change if the application changes its URL rules. For example, ?r=site/contact, site/contact, contact could all refer to the same page, depending on the URL rule setting. So I'm proposing to use the pair $route and $params to specify a URL because they don't change.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Dec 1, 2013

Contributor

yes, no confusion, i got the point, but to be true dont know how to do it in quick and easy way, need to think. maybe you or someone else will also come up with some implementation idea or smth. else.

Contributor

Ragazzo commented Dec 1, 2013

yes, no confusion, i got the point, but to be true dont know how to do it in quick and easy way, need to think. maybe you or someone else will also come up with some implementation idea or smth. else.

*
* @param array $contactData
*/
public function submit(array $contactData)

This comment has been minimized.

@DavertMik

DavertMik Dec 2, 2013

Contributor

Why not just $contactData = null ?
So you won't pass it like submit([])
I think it's a bit ugly

@DavertMik

DavertMik Dec 2, 2013

Contributor

Why not just $contactData = null ?
So you won't pass it like submit([])
I think it's a bit ugly

This comment has been minimized.

@Ragazzo

Ragazzo Dec 2, 2013

Contributor

no, submit([]) is needed to show user that we submit empty form with no data :)

@Ragazzo

Ragazzo Dec 2, 2013

Contributor

no, submit([]) is needed to show user that we submit empty form with no data :)

@Ragazzo Ragazzo referenced this pull request Dec 2, 2013

Merged

Yii2 basic codeception #1393

5 of 5 tasks complete
@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Dec 14, 2013

Contributor

closing this one because of out of date and it will be reworked after yii2 basic codeception integration merged.

Contributor

Ragazzo commented Dec 14, 2013

closing this one because of out of date and it will be reworked after yii2 basic codeception integration merged.

@Ragazzo Ragazzo closed this Dec 14, 2013

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