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

[5.5] Reset RefreshDatabaseState after DatabaseMigrations rolls back #21325

Merged
merged 2 commits into from Sep 25, 2017

Conversation

Projects
None yet
3 participants
@inxilpro
Contributor

inxilpro commented Sep 21, 2017

If you mix DatabaseMigrations and RefreshDatabase (for example if you're running some Dusk tests and some regular tests), you may run into a state where RefreshDatabaseState::$migrated is set to true but the database has been rolled back (eg. if you run a refresh test, followed by a migration test, followed by another refresh test). This addresses that edge case.

@inxilpro

This comment has been minimized.

Show comment
Hide comment
@inxilpro

inxilpro Sep 21, 2017

Contributor

In the interim, if anyone else runs into this, you can add the following to your TestCase class:

<?php

namespace Tests;

use Illuminate\Foundation\Testing\DatabaseMigrations;
use Illuminate\Foundation\Testing\RefreshDatabaseState;
use Illuminate\Foundation\Testing\TestCase as BaseTestCase;
use Tests\Traits\CreatesApplication;

abstract class TestCase extends BaseTestCase
{
    use CreatesApplication;

    protected function setUpTraits()
    {
        $uses = parent::setUpTraits();

        if (isset($uses[DatabaseMigrations::class])) {
            $this->beforeApplicationDestroyed(function () {
                RefreshDatabaseState::$migrated = false;
            });
        }

        return $uses;
    }
}
Contributor

inxilpro commented Sep 21, 2017

In the interim, if anyone else runs into this, you can add the following to your TestCase class:

<?php

namespace Tests;

use Illuminate\Foundation\Testing\DatabaseMigrations;
use Illuminate\Foundation\Testing\RefreshDatabaseState;
use Illuminate\Foundation\Testing\TestCase as BaseTestCase;
use Tests\Traits\CreatesApplication;

abstract class TestCase extends BaseTestCase
{
    use CreatesApplication;

    protected function setUpTraits()
    {
        $uses = parent::setUpTraits();

        if (isset($uses[DatabaseMigrations::class])) {
            $this->beforeApplicationDestroyed(function () {
                RefreshDatabaseState::$migrated = false;
            });
        }

        return $uses;
    }
}

@GrahamCampbell GrahamCampbell changed the title from Reset RefreshDatabaseState after DatabaseMigrations rolls back to [5.5] Reset RefreshDatabaseState after DatabaseMigrations rolls back Sep 21, 2017

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Sep 21, 2017

Member

Hmm, can you provide more info as to why you would ever combine the two on one test class?

Member

taylorotwell commented Sep 21, 2017

Hmm, can you provide more info as to why you would ever combine the two on one test class?

@inxilpro

This comment has been minimized.

Show comment
Hide comment
@inxilpro

inxilpro Sep 21, 2017

Contributor

That static migrated flag persists across test cases. So if you have three files, two that use the refresh trait and one that uses the migration trait, you can run into this issue.

Contributor

inxilpro commented Sep 21, 2017

That static migrated flag persists across test cases. So if you have three files, two that use the refresh trait and one that uses the migration trait, you can run into this issue.

@inxilpro

This comment has been minimized.

Show comment
Hide comment
@inxilpro

inxilpro Sep 22, 2017

Contributor

The issue turns up when you have at least three separate TestCases: two that use the RefreshDatabase trait and one that uses DatabaseMigrations. For example:

ATest.php

class ATest extends TestCase
{
  use Illuminate\Foundation\Testing\RefreshDatabase;
  // ...
}

BTest.php

class BTest extends DuskTestCase
{
  // Since Dusk test cases run in a separate process from the application
  // under test, you usually can't use RefreshDatabase (since none of the data
  // in the DB transaction will be visible to the app process).
  use Illuminate\Foundation\Testing\DatabaseMigrations;
  // ...
}

CTest.php

class CTest extends TestCase
{
  use Illuminate\Foundation\Testing\RefreshDatabase;
  // ...
}

When I run my tests:

  1. ATest will call migrate:fresh and set RefreshDatabaseState::$migrated to true.
  2. BTest will call migrate:fresh and then migrate:rollback.
  3. CTest will skip migrate:fresh because RefreshDatabaseState::$migrated will still be set to true from ATest, and it will fail because the database will have been rolled back by BTest.
Contributor

inxilpro commented Sep 22, 2017

The issue turns up when you have at least three separate TestCases: two that use the RefreshDatabase trait and one that uses DatabaseMigrations. For example:

ATest.php

class ATest extends TestCase
{
  use Illuminate\Foundation\Testing\RefreshDatabase;
  // ...
}

BTest.php

class BTest extends DuskTestCase
{
  // Since Dusk test cases run in a separate process from the application
  // under test, you usually can't use RefreshDatabase (since none of the data
  // in the DB transaction will be visible to the app process).
  use Illuminate\Foundation\Testing\DatabaseMigrations;
  // ...
}

CTest.php

class CTest extends TestCase
{
  use Illuminate\Foundation\Testing\RefreshDatabase;
  // ...
}

When I run my tests:

  1. ATest will call migrate:fresh and set RefreshDatabaseState::$migrated to true.
  2. BTest will call migrate:fresh and then migrate:rollback.
  3. CTest will skip migrate:fresh because RefreshDatabaseState::$migrated will still be set to true from ATest, and it will fail because the database will have been rolled back by BTest.
@deleugpn

This comment has been minimized.

Show comment
Hide comment
@deleugpn

deleugpn Sep 22, 2017

Contributor

Why are you runing BTest in the same process as ATest and CTest? php artisan dusk is suppose to be separated from ./vendor/bin/phpunit

Contributor

deleugpn commented Sep 22, 2017

Why are you runing BTest in the same process as ATest and CTest? php artisan dusk is suppose to be separated from ./vendor/bin/phpunit

@inxilpro

This comment has been minimized.

Show comment
Hide comment
@inxilpro

inxilpro Sep 22, 2017

Contributor

OK, maybe that’s not the best example. In our case we have legacy systems that use MyISAM tables (which don’t support transactions). A few tests still run DatabaseMigrations because of this, but most work/we’ve made work with RefreshDatabase. It’s definitely an edge case, but the fix is a single line and breaks nothing.

Contributor

inxilpro commented Sep 22, 2017

OK, maybe that’s not the best example. In our case we have legacy systems that use MyISAM tables (which don’t support transactions). A few tests still run DatabaseMigrations because of this, but most work/we’ve made work with RefreshDatabase. It’s definitely an edge case, but the fix is a single line and breaks nothing.

@taylorotwell taylorotwell merged commit 4f81a2f into laravel:5.5 Sep 25, 2017

2 checks passed

continuous-integration/styleci/pr The StyleCI analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment