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

[Yii] Splitted `cleanup` and `transactional` configuration #4379

Merged
merged 6 commits into from Jul 8, 2017

Conversation

Projects
None yet
6 participants
@leandrogehlen
Contributor

leandrogehlen commented Jul 3, 2017

No description provided.

leandrogehlen referenced this pull request in yiisoft/yii2-app-basic Jul 3, 2017

@Naktibalda

This comment has been minimized.

Show comment
Hide comment
@Naktibalda

Naktibalda Jul 4, 2017

Member

@leandrogehlen Could you make it backwards compatible and use cleanup option of transaction option is not set?

Member

Naktibalda commented Jul 4, 2017

@leandrogehlen Could you make it backwards compatible and use cleanup option of transaction option is not set?

@Naktibalda Naktibalda requested review from samdark and DavertMik Jul 4, 2017

@Naktibalda Naktibalda changed the title from Splitted `cleanup` and `transactional` configuration to [Yii] Splitted `cleanup` and `transactional` configuration Jul 4, 2017

@Naktibalda Naktibalda added the Yii label Jul 4, 2017

@leandrogehlen

This comment has been minimized.

Show comment
Hide comment
@leandrogehlen

leandrogehlen Jul 4, 2017

Contributor

@Naktibalda, i believe that is happening, because the default configs are:

protected $config = [
    'cleanup'     => true,
    'transaction' => true,
    'entryScript' => '',
    'entryUrl'    => 'http://localhost/index-test.php'
];
Contributor

leandrogehlen commented Jul 4, 2017

@Naktibalda, i believe that is happening, because the default configs are:

protected $config = [
    'cleanup'     => true,
    'transaction' => true,
    'entryScript' => '',
    'entryUrl'    => 'http://localhost/index-test.php'
];
@Naktibalda

This comment has been minimized.

Show comment
Hide comment
@Naktibalda

Naktibalda Jul 4, 2017

Member

Situation: we have an existing Yii2 website with cleanup: false in functional.suite.yml.
After update, the tests will start failing because your new setting will use a default value transaction: true.

A backwards compatible way would be to set default value of transaction to null and use cleanup setting if transaction is null, because it isn't set in suite.yml.

Member

Naktibalda commented Jul 4, 2017

Situation: we have an existing Yii2 website with cleanup: false in functional.suite.yml.
After update, the tests will start failing because your new setting will use a default value transaction: true.

A backwards compatible way would be to set default value of transaction to null and use cleanup setting if transaction is null, because it isn't set in suite.yml.

@leandrogehlen

This comment has been minimized.

Show comment
Hide comment
@leandrogehlen

leandrogehlen Jul 4, 2017

Contributor

ok, you are rigth, i will do the change

Contributor

leandrogehlen commented Jul 4, 2017

ok, you are rigth, i will do the change

Show outdated Hide outdated src/Codeception/Module/Yii2.php
@@ -150,6 +153,10 @@ class Yii2 extends Framework implements ActiveRecord, PartedModule
public function _initialize()
{
if ($this->config['transaction'] == null) {
$this->config['transaction'] = $this->config['cleanup'];

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Jul 5, 2017

Whitespace found at end of line

@Nitpick-CI

Nitpick-CI Jul 5, 2017

Whitespace found at end of line

Show outdated Hide outdated src/Codeception/Module/Yii2.php
@@ -337,7 +343,7 @@ public function grabFixtures()
{
return call_user_func_array(
'array_merge',
array_map( // merge all fixtures from all fixture stores
array_map(// merge all fixtures from all fixture stores

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Jul 5, 2017

Opening parenthesis of a multi-line function call must be the last content on the line

@Nitpick-CI

Nitpick-CI Jul 5, 2017

Opening parenthesis of a multi-line function call must be the last content on the line

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Jul 6, 2017

Member

From my side, I'd like to keep the cleanup option for transactions.
This is compatible with other ORM modules, like Doctrine2 and Laravel.
I know this name is not perfect but at least it is consistent and backwards compatible.
Current implementation is consistent but is not clear from the documentation that setting cleanup to false will disable transactions as well.

Maybe we can create a different option to cleanup fixtures?

cleanupFixtures: true

But let's wait for a review @samdark

Member

DavertMik commented Jul 6, 2017

From my side, I'd like to keep the cleanup option for transactions.
This is compatible with other ORM modules, like Doctrine2 and Laravel.
I know this name is not perfect but at least it is consistent and backwards compatible.
Current implementation is consistent but is not clear from the documentation that setting cleanup to false will disable transactions as well.

Maybe we can create a different option to cleanup fixtures?

cleanupFixtures: true

But let's wait for a review @samdark

@leandrogehlen

This comment has been minimized.

Show comment
Hide comment
@leandrogehlen

leandrogehlen Jul 6, 2017

Contributor

The transactional cleanup is not valid for functional tests. Does not make sense to start a transanction in client side.

Contributor

leandrogehlen commented Jul 6, 2017

The transactional cleanup is not valid for functional tests. Does not make sense to start a transanction in client side.

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Jul 7, 2017

Member

The transactional cleanup is not valid for functional tests. Does not make sense to start a transanction in client side.

Ok, makes sense

Member

DavertMik commented Jul 7, 2017

The transactional cleanup is not valid for functional tests. Does not make sense to start a transanction in client side.

Ok, makes sense

@samdark

samdark approved these changes Jul 8, 2017

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Jul 8, 2017

Collaborator

@DavertMik looks good to me.

Collaborator

samdark commented Jul 8, 2017

@DavertMik looks good to me.

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Jul 8, 2017

Member

Thanks

Member

DavertMik commented Jul 8, 2017

Thanks

@DavertMik DavertMik merged commit 62b353a into Codeception:2.3 Jul 8, 2017

0 of 4 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
semaphoreci The build is pending on Semaphore.
Details
wercker/build Wercker pipeline executing
Details
@michaelarnauts

This comment has been minimized.

Show comment
Hide comment
@michaelarnauts

michaelarnauts Aug 18, 2017

This change caused issue #4417

This change caused issue #4417

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