feat: make migration/seed settings flexible on database testing

[kenjis]

Description
Now you can't disable migration on database testing if you want.
And running migration/seed on each test may slow tests.

• You can disable migration: $migrate = false
• You can run migration only once: $migrateOnce = true
• You can run seed only once: $seedOnce = true

Checklist:

• Securely signed commits
• Component(s) with PHPdocs
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[MGatner]

I will have to look at this one on mobile, but I like breaking these out. First and foremost we must be absolutely sure that current tests will still work after this change (I.e. still respects $refresh). Second IMO we should go ahead and deprecate $refresh and rely on these going forward.

These changes will also require updates to the User Guide.

[kenjis]

Updated the User Guide.

[kenjis]

Only PHP 7.3 - Postgre failed. Why?
https://github.com/codeigniter4/CodeIgniter4/pull/3993/checks?check_run_id=1576289817

[MGatner]

@kenjis it's been happening from time to time. Not related to your PR, we need to look into why the test table is persisting.

[kenjis]

I see. Thanks!

[kenjis]

PHP 7.3 - Postgre failed again.
https://github.com/codeigniter4/CodeIgniter4/pull/3993/checks?check_run_id=1587713000

[paulbalandan]

PHP 7.3 - Postgre failed again.
https://github.com/codeigniter4/CodeIgniter4/pull/3993/checks?check_run_id=1587713000

You just need to re-run actions until it goes green.

[kenjis]

@paulbalandan How do I re-run?

[kenjis]

I rebased. Can #4015 help?

[kenjis]

Oh no! PHP 7.3 - Postgre and PHP 7.4 - Postgre failed...

[kenjis]

There was 1 failure:

1) CodeIgniter\Database\Live\ForgeTest::testAddFields
Array
(
    [0] => stdClass Object
        (
            [name] => id
            [type] => integer
            [default] => nextval('db_forge_test_fields_id_seq'::regclass)
            [max_length] => 32
        )

    [1] => stdClass Object
        (
            [name] => active
            [type] => integer
            [default] => 0
            [max_length] => 32
        )

    [2] => stdClass Object
        (
            [name] => username
            [type] => character varying
            [default] => 
            [max_length] => 255
        )

    [3] => stdClass Object
        (
            [name] => name
            [type] => character varying
            [default] => 
            [max_length] => 255
        )

)

Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'integer'
+'character varying'

/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Database/Live/ForgeTest.php:605

--

See #4015 (comment).

[kenjis]

All tests passed.

[kenjis]

@MGatner Is something wrong with this PR?

[MGatner]

Not necessarily! It's more complicated than I am able to review on mobile, which is where I do most of my reviews. I will set aside some desktop time later to do it.

[kenjis]

@MGatner Okay, thanks!

[MGatner]

Generally this looks good. My main concern is around the "once" aspect. As is, with the private scope on the flags, migrations & seeds will only run once for the entire run of PHPUnit, and yet will be dependent on the class-specific settings of whichever test case is run first. Consider these three test cases all in the same project:

class FooTest extends CIDatabaseTestCase
{
	protected $migrateOnce = true;
	protected $namespace   = 'Tests\Support';
}

class BarTest extends CIDatabaseTestCase
{
	protected $migrateOnce = false;
	protected $namespace   = null;
}

class BamTest extends CIDatabaseTestCase
{
	protected $migrateOnce = true;
	protected $namespace   = 'App';
}

Depending on which are run and in which order (and by which thread) they could all leave the database in very different states.

In my opinion, the "once" methods should apply per test case (file), rather than for the whole suite - and if we want to offer the option for a single static database test configuration then it should be handled in a bootstrap, not the test cases themselves.

I hope that makes sense.

[kenjis]

@MGatner Thank you for the review.

See https://github.com/codeigniter4/CodeIgniter4/pull/3993/files#diff-7591a921e2ba957a20a6e334d50bb134de0a53a4c4c4a3f01e27d001b9bae4c6R449-R458
It resets the private properties after each test class.
So I think the "once" methods apply per test case (file).

I added test for $migrateOnce.

$ vendor/bin/phpunit --no-coverage tests/system/Database/DatabaseTestCase --debug
PHPUnit 8.5.4 by Sebastian Bergmann and contributors.

Test 'CodeIgniter\Database\DatabaseTestCase\DatabaseTestCaseMigrationOnce1Test::testMigrationDone' started
Test 'CodeIgniter\Database\DatabaseTestCase\DatabaseTestCaseMigrationOnce1Test::testMigrationDone' ended
Test 'CodeIgniter\Database\DatabaseTestCase\DatabaseTestCaseMigrationOnce2Test::testMigrationDone' started
Test 'CodeIgniter\Database\DatabaseTestCase\DatabaseTestCaseMigrationOnce2Test::testMigrationDone' ended


Time: 173 ms, Memory: 12.00 MB

OK (2 tests, 2 assertions)

[MGatner]

Oh! Sorry then, that was my mistake. I wonder what kind of weird scopiness is happening that private static is applied for every file? Maybe each child class inherits the original static property with false and then sets it to true at the local scope? @samsonasik help?

Given this, I approve the PR, but I will wait for merge.

[samsonasik]

I am ok with this 👍

[MGatner]

@samsonasik do you understand how the scoping is working out such that the "once" variables are toggled for every file?

[samsonasik]

@MGatner that rely on doneSeed ?

[MGatner]

Yes. Since $doneSeed is private static I would expect it to be set once for the entire test suite and thus seeding would only happen one time. But @kenjis confirms that it actually resets between files, which makes me think it must be something with the scoping I don't understand.

[paulbalandan]

This method is called after the test suite class is tested:

	/**
	 * Reset $doneMigration and $doneSeed
	 *
	 * @afterClass
	 */
	public static function resetMigrationSeedCount()
	{
		self::$doneMigration = false;
		self::$doneSeed      = false;
	}

Notice the @afterClass annotation in the doc block. PHPUnit calls this static method after all test methods have been run in the test class. It is like implicitly calling tearDownAfterClass method. See @afterClass

[MGatner]

I totally missed that. Thanks for explaining 🤩