Skip to content
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

[8.x] MySQL Builds #39415

Merged
merged 27 commits into from
Nov 2, 2021
Merged

[8.x] MySQL Builds #39415

merged 27 commits into from
Nov 2, 2021

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented Oct 29, 2021

This PR adds a separate build for MySQL that runs our database integration tests against MySQL 5.7 & MySQL 8.0. What I did was add separate database builds that run our database integration tests to validate that all functionality works in every engine. I've already discovered several places in our tests which were set up incorrectly or validating data incorrectly. I've added comments to each part that got changed to explain why that happened. Feel free to "Resolve conversation" those that are clear and add comments to the ones which you have questions about.

A few follow up PR's that I'd like to make after this is to consolidate how the environment gets set up and data gets refreshed as well as to look in the unit test suite which tests might be able to move to the integration test suite.

Builds for PostgreSQL, MSSQL & MariaDB will be added after this gets merged to keep the scope of this PR small.

$this->artisan('db:wipe', ['--drop-views' => true]);
}

parent::tearDown();
Copy link
Member Author

@driesvints driesvints Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we're not running an in-memory database anymore we'll need to wipe the database each time after every test. I want to refactor this to a RefreshDatabase trait later on but for now this is okay.

@@ -132,7 +132,7 @@ public function testCustomPivotClass()
$post->tagsWithCustomPivot()->attach($tag->id);

$this->assertInstanceOf(PostTagPivot::class, $post->tagsWithCustomPivot[0]->pivot);
$this->assertEquals('1507630210', $post->tagsWithCustomPivot[0]->pivot->getAttributes()['created_at']);
$this->assertEquals('1507630210', $post->tagsWithCustomPivot[0]->pivot->created_at);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch to an accessor instead of relying on SQLite's U date format. We only need to verify that the custom pivot class is properly hydrated so no need to use $dateFormat specifically.

'created_at' => '1507630210',
'updated_at' => '1507630210',
'created_at' => '2017-10-10 10:10:10',
'updated_at' => '2017-10-10 10:10:10',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use universal date formats instead of SQLite specific timestamps.


$this->assertSame('select "one".*, (select count(*) from "two" where "one"."id" = "two"."one_id") as "twos_count" from "one"', $result);
$this->assertNull($query->orders);
$this->assertSame([], $query->getRawBindings()['order']);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to verify a specific query, just that the sorting got removed from the subquery.

$blueprint->build($this->getConnection(), new SQLiteGrammar);

$this->assertArrayHasKey(TinyInteger::NAME, Type::getTypesMap());
$this->assertSame('tinyinteger', Schema::getColumnType('test', 'test_column'));
$this->assertEquals($expected, $statements);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to check the expected queries as that should be a concern of the Doctrine test suite. Just checking if the test column got renamed is enough.

@@ -101,7 +101,7 @@ public function testAddSelectWithSubQuery()

public function testFromWithAlias()
{
$this->assertSame('select * from "posts" as "alias"', DB::table('posts', 'alias')->toSql());
$this->assertCount(2, DB::table('posts', 'alias')->select('alias.*')->get());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually verify if the aliasing works instead of checking the query.


Schema::dropAllViews();

DB::statement('create view "view"("id") as select 1');
DB::statement('create view foo (id) as select 1');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the universal syntax of creating database views.

@driesvints driesvints changed the title [8.x] MySQL Build [8.x] MySQL 5.7 Build Oct 29, 2021
@driesvints
Copy link
Member Author

@cbl I started working on this ^

@@ -48,7 +48,7 @@ protected function setUp(): void

Schema::create('posts_tags', function (Blueprint $table) {
$table->integer('post_id');
$table->integer('tag_id');
$table->string('tag_id');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In one test we're storing the name as a value so this needs to be a string. SQLite accepted this but it's in fact an incorrect column type for these tests.

@@ -180,7 +180,7 @@ public function testPaginationWithDistinctColumnsAndSelect()
TestPost::create(['title' => 'Goodbye world']);
}

$query = TestPost::query()->distinct('title')->select('title');
$query = TestPost::query()->orderBy('title')->distinct('title')->select('title');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor based pagination relies on an orderBy clause when selecting specific columns.

$user = TestModel1::create([
'date_field' => '2019-10',
TestModel1::create([
'date_field' => '2019-10-01',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Date fields in most engines need a full date to be inserted.

@@ -26,7 +26,7 @@ protected function setUp(): void
Schema::create('texts', function (Blueprint $table) {
$table->increments('id');
$table->unsignedInteger('post_id');
$table->boolean('content');
$table->text('content');
Copy link
Member Author

@driesvints driesvints Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

content for these tests relies on string values so this column type was incorrect.

(array) DB::table('posts')->select(['id', 'title', 'foo' => function ($query) {
$query->select('bar');
$query->select('content');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bar isn't a column.

@@ -127,7 +127,7 @@ public function testWhereValueSubQuery()

public function testWhereValueSubQueryBuilder()
{
$subQuery = DB::table('posts')->selectRaw("'Sub query value'");
$subQuery = DB::table('posts')->selectRaw("'Sub query value'")->limit(1);
Copy link
Member Author

@driesvints driesvints Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to limit this raw select because MySQL will produce more than one row and will throw an error when used as a subquery.

@driesvints driesvints changed the title [8.x] MySQL 5.7 Build [8.x] MySQL Builds Nov 2, 2021
@@ -53,7 +53,7 @@ public function testWhereHasMorph()
{
$comments = Comment::whereHasMorph('commentable', [Post::class, Video::class], function (Builder $query) {
$query->where('title', 'foo');
})->get();
})->orderBy('id')->get();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SQLite, MySQL 5.7 & MySQL 8.0 have different behavior for ordering the result set in these tests. Since the focus of the tests isn't on the order of the results, I've added orderBy clauses to all of these queries.


foreach (['id', 'name', 'age', 'color'] as $column) {
$this->assertContains($column, $columns);
}
Copy link
Member Author

@driesvints driesvints Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really unhappy with this solution but there doesn't seems like a better way to do this 😅

Needed because the ordering of these columns isn't the same between MySQL 8 and MySQL 5.7

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could sort the arrays and check they are same?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, there's a function for that assertEqualsCanonicalizing

@driesvints driesvints requested review from nunomaduro and taylorotwell and removed request for nunomaduro November 2, 2021 13:18
@driesvints driesvints marked this pull request as ready for review November 2, 2021 13:18
@taylorotwell taylorotwell merged commit 50f5079 into 8.x Nov 2, 2021
@taylorotwell taylorotwell deleted the db-driver-builds branch November 2, 2021 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants