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

unloadFixtures() should run after db transaction #4497

Merged
merged 1 commit into from Sep 28, 2017

Conversation

Projects
None yet
4 participants
@devonliu02
Contributor

devonliu02 commented Sep 11, 2017

No description provided.

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Sep 11, 2017

Member

Makes sense... But makes a little sense to do both in transaction :)

Member

DavertMik commented Sep 11, 2017

Makes sense... But makes a little sense to do both in transaction :)

@devonliu02

This comment has been minimized.

Show comment
Hide comment
@devonliu02

devonliu02 Sep 11, 2017

Contributor

Fixtures were loaded before transaction (if transaction is on ), then if we unload fixtures before transaction rollback, the delete SQL in unloadFixture will be rollback too. so it is useless for cleanup configuration.

Contributor

devonliu02 commented Sep 11, 2017

Fixtures were loaded before transaction (if transaction is on ), then if we unload fixtures before transaction rollback, the delete SQL in unloadFixture will be rollback too. so it is useless for cleanup configuration.

@devonliu02

This comment has been minimized.

Show comment
Hide comment
@devonliu02

devonliu02 Sep 11, 2017

Contributor

Do you mean all fixtures operation should be done in transaction? @DavertMik

Contributor

devonliu02 commented Sep 11, 2017

Do you mean all fixtures operation should be done in transaction? @DavertMik

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Sep 11, 2017

Member

Not sure. As for me - yes, inside transaction. But maybe Yii community have another opinion

@samdark what you think?

Member

DavertMik commented Sep 11, 2017

Not sure. As for me - yes, inside transaction. But maybe Yii community have another opinion

@samdark what you think?

$this->transaction->rollBack();
$this->debugSection('Database', 'Transaction cancelled; all changes reverted.');
}
if ($this->config['cleanup']) {
foreach ($this->loadedFixtures as $fixture) {
$fixture->unloadFixtures();

This comment has been minimized.

@DavertMik

DavertMik Sep 13, 2017

Member

ok but fixtures are cleaned up by rolled back transaction...
and this call may throw an exception...

@DavertMik

DavertMik Sep 13, 2017

Member

ok but fixtures are cleaned up by rolled back transaction...
and this call may throw an exception...

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Sep 13, 2017

Collaborator

I don't understand the change. Currently there are two options:

  1. Wrap everything with a transaction so no cleanup actually needed: transaction reverts data from fixtures by itself.
  2. Not using transaction but cleaning up manually with unloadFixtures().

Why should we use both?

Collaborator

samdark commented Sep 13, 2017

I don't understand the change. Currently there are two options:

  1. Wrap everything with a transaction so no cleanup actually needed: transaction reverts data from fixtures by itself.
  2. Not using transaction but cleaning up manually with unloadFixtures().

Why should we use both?

@devonliu02

This comment has been minimized.

Show comment
Hide comment
@devonliu02

devonliu02 Sep 13, 2017

Contributor

the problem here is that fixtures were loaded before the transaction, and transaction will not reverts them.

Contributor

devonliu02 commented Sep 13, 2017

the problem here is that fixtures were loaded before the transaction, and transaction will not reverts them.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Sep 13, 2017

Collaborator

Would this change fix loading fixtures before transaction?

Collaborator

samdark commented Sep 13, 2017

Would this change fix loading fixtures before transaction?

@devonliu02

This comment has been minimized.

Show comment
Hide comment
@devonliu02

devonliu02 Sep 16, 2017

Contributor

Maybe I didn't make that clear, or I misunderstood the configuration.

I want that fixtures will be unloaded after each test, and transaction on too.

Let's see the problem step by step,

  1. before test (_before)
    1. load fixtures
    2. begin transaction
  2. test run
  3. after test (_after)
    1. unload fixtures
    2. rollback transaction

Now, we can see that fixtures are not really be unloaded because the transaction has been rollback.
In this case, the fixtures will be loaded again before next test run.

Contributor

devonliu02 commented Sep 16, 2017

Maybe I didn't make that clear, or I misunderstood the configuration.

I want that fixtures will be unloaded after each test, and transaction on too.

Let's see the problem step by step,

  1. before test (_before)
    1. load fixtures
    2. begin transaction
  2. test run
  3. after test (_after)
    1. unload fixtures
    2. rollback transaction

Now, we can see that fixtures are not really be unloaded because the transaction has been rollback.
In this case, the fixtures will be loaded again before next test run.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Sep 18, 2017

Collaborator

As far as I remember, sequence was meant to be:

  1. before test (_before)
    1. begin transaction
    2. load fixtures
  2. test run
  3. after test (_after)
    3. rollback transaction
    4. unload fixtures in case transactions aren't enabled

Is is different?

Collaborator

samdark commented Sep 18, 2017

As far as I remember, sequence was meant to be:

  1. before test (_before)
    1. begin transaction
    2. load fixtures
  2. test run
  3. after test (_after)
    3. rollback transaction
    4. unload fixtures in case transactions aren't enabled

Is is different?

@devonliu02

This comment has been minimized.

Show comment
Hide comment
@devonliu02
Contributor

devonliu02 commented Sep 19, 2017

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Sep 19, 2017

Collaborator

Indeed. @DavertMik need your input on original intentions...

Collaborator

samdark commented Sep 19, 2017

Indeed. @DavertMik need your input on original intentions...

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Sep 28, 2017

Member

Sorry, you are right. I missed the execution order in _before. Looks like it changed after I looked into it last time. Yes, we need to merge it

Member

DavertMik commented Sep 28, 2017

Sorry, you are right. I missed the execution order in _before. Looks like it changed after I looked into it last time. Yes, we need to merge it

@DavertMik DavertMik merged commit 949f5ba into Codeception:2.3 Sep 28, 2017

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details
wercker/build Wercker pipeline passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment