initial snapshot does not define collation info

[rrd108]

This PR is intended to fix cakephp/phinx#2225

The added test case passes, so it seems to be fine for me.

However, thi is not a final version as I was not sure about 2 things.

Question 1

The current solution actually execute the migration_snapshot command and creates and writes a new migration file. My guess is that I could do it without creating the file, but I did not find out how.

Question 2

The current test alters the articles table's title field's collation. So I guess I have to alter it back to avoid affecting other tests. Do I have to do it, or the test system will drop the table after each tests?

[rrd108]

I see, I have to handle the case when the collation is the default. I will get back to it in a few days.

[ndm2]

Please compare against the full snapshot like the other snapshot tests do, ideally use runSnapshotTest(), see for example the testSnapshotWithAutoIdCompatibleSignedPrimaryKeys() test.

The comparison file would be tests/comparisons/Migration/test_snapshot_with_non_default_collation.php. And then also please register that comparison file with MigrationsTest::migrationsProvider(), that way it will be tested whether the generated snapshot can be applied without errors.

As can be seen from the failing tests, this would affect diffs too, so encoding defaults and changes would need to be properly tested there too.

Might sound a bit pedantic, but when my recent work on Migrations taught me anything, then that its sloppy testing is one of its worst enemies, and a major source of pain when making changes.

[rrd108]

I still have to fix what @ndm2 suggested.

[rrd108]

Do I have to do anything more on this?

[LordSimal]

LGTM

[ndm2]

Please also add the new snapshot to \Migrations\Test\TestCase\MigrationsTest::migrationsProvider().

[ndm2]

So I just realized that \Migrations\Test\TestCase\MigrationsTest::testMigrateSnapshots() is actually still broken and not testing properly. That would also affect this snapshot right here, which as a foreign key for a non-existent table.

I'll try to get that fixed somewhat quickly.

[rrd108]

Please also add the new snapshot to \Migrations\Test\TestCase\MigrationsTest::migrationsProvider().

added in 9052c29

[rrd108]

@ndm2 As I see tha failing tests coming from other test, not related to this one.

[ndm2]

I'm note sure what exactly you did, but it looks like rebasing somehow went wrong, all the changes here from existing commits should not be seen as new. The test failures might be a result of that.

[rrd108]

I'm note sure what exactly you did, but it looks like rebasing somehow went wrong, all the changes here from existing commits should not be seen as new. The test failures might be a result of that.

I do not know what did I do wrong.

Anyway I would need some suggestion how to move forward.

I cloned cakephp/migrations to my dev machine and executed vendor/bin/phpunit tests without any changes and I get Tests: 122, Assertions: 396, Errors: 8, Warnings: 1, Skipped: 10

So it seems that is the starting point for now.

Re-adding my changes to this newly cloned repo does not change the number of errors, so I guess my additions are ok, but the checks will certainly fail.

[rrd108]

anyway it seems no error on the pipeline.

[dereuromark]

I wonder if this could be squash merged
But it would be safer to squash and rebase Prior to merging