Implement functional migration tests #19353

roji · 2019-12-18T15:01:13Z

This adds functional migration tests which perform DDL operations on the database.

The total running time of the SQL Server suite is roughly the same as before, thanks to test parallelism and async I/O. ❤️

The new tests have been added as MigrationTestBase, MigrationsSqlServerTest, MigrationsSqliteTest; they are ports of almost all existing MigrationsSqlGeneratorTests.
The previous MigrationsTestBase has been renamed to MigrationsInfrastructureTestBase, and the First/Second test has been removed from it. We can probably refactor it a bit (e.g. move SQL-only tests out to MigratorTests or something).
Moved MigrationsSqlGeneratorTests from EFCore.Relational.Specifications.Tests to EFCore.Relational.Tests. For now I've kept all tests, although I think we can remove any already covered in the new MigrationTestBase (which is most of them).
Similar question for MigrationsModelDifferTest: the new functional tests implicitly exercise the model differ, so I'm not sure if many of the differ-specific are still valuable.
Naming-wise, since MigrationTestBase tests have been ported from MigrationSqlGeneratorTests, I've conserved the names. But these are basically migration operations - but that doesn't really seem appropriate, as the operations are internal implementation details. We should consider renaming to something more functional (e.g. instead of AddUniqueConstraintOperation, we could all it AlternateKey_add).
For data/seeding tests, ideally we'd query the database for data, but that's a bit difficult to set up. SQL assertions are probably good enough for now.
Database creation tests have not been ported over and remain in SqlServerMigrationSqlGeneratorTest, as in the real world they seem pretty flaky, at least for SQL Server and Sqlite. We can look at it later if we want to.
Originally I had the test infrastructure apply the reverse migration, and even compare reverse-engineered models before and after for equality. However, I ran into issues on Sqlite where symmetrical operations aren't always supported (e.g. you can add columns but you can't drop them). So for now we can just explicitly test the reverse migrations, which we want to do anyway.

Closes #19039
Replaces #19258

roji

Here are some specific comments

roji · 2019-12-18T15:03:49Z

test/EFCore.SqlServer.FunctionalTests/MigrationsSqlServerTest.cs

-                        b.Property<string>("NormalizedName")
-                            .HasMaxLength(256);
+        [ConditionalFact(Skip = "Column 'FullName' in table 'People' is of a type that is invalid for use as a key column in an index.")]
+        public override async Task AlterColumnOperation_change_computed_with_index()


This fails with Column 'FullName' in table 'People' is of a type that is invalid for use as a key column in an index., is that expected?

roji · 2019-12-18T15:04:56Z

test/EFCore.Relational.Specification.Tests/MigrationsTestBase.cs


        [ConditionalFact]
-        public virtual void Can_get_active_provider()
+        public virtual async Task CreateTableOperation_all_settings()


This test is somewhat useless since a database must support everything in order to run it (e.g. Sqlite disables it). Better to just remove and leave the feature-by-feature tests?

What doesn't SQLite support here?

Schema, check constraints...

Am going to leave as-is (ignored for Sqlite) and we can re-discuss in cleanup if this test is valuable or not...

test/EFCore.SqlServer.FunctionalTests/MigrationsSqlServerTest.cs

roji · 2019-12-18T15:07:02Z

test/EFCore.SqlServer.FunctionalTests/MigrationsSqlServerTest.cs


-                        b.HasKey("LoginProvider", "ProviderKey");
+        [ConditionalFact(Skip = "Doesn't work")]
+        public virtual async Task AlterColumnOperation_remove_identity()


Like AlterColumnOperation_make_identity, this fails with: To change the IDENTITY property of a column, the column needs to be dropped and recreated., is this expected?

Removed, thanks.

roji · 2019-12-18T15:08:17Z

test/EFCore.Sqlite.FunctionalTests/MigrationsSqliteTest.cs

        }

-        public override void Can_generate_one_up_script()
+        // In Sqlite, comments are only generated when creating a table


I can only see comments generated for CreateTableOperation... I'm guessing this is intentional?

Yes. It's not possible to add these with ALTER TABLE..ADD statements.

Thanks, will leave these overrides to assert the actual behavior.

AndriySvyryd · 2019-12-20T04:40:23Z

test/EFCore.Relational.Specification.Tests/MigrationsTestBase.cs

        {
+            // Build the source and target models. Add current/latest product version if one wasn't set.
+            var sourceModelBuilder = Fixture.TestHelp


For higher fidelity the source builder shouldn't have conventions, that's how the snapshot is built and we have had several bugs that were hidden by conventions. However it will require more configuration to actually set up the tests.

bricelam

LGTM

Regarding MigrationsModelDifferTest, I agree we can probably remove it.

bricelam · 2020-01-06T22:19:04Z

test/EFCore.Relational.Specification.Tests/MigrationsTestBase.cs


        [ConditionalFact]
-        public virtual void Can_get_active_provider()
+        public virtual async Task CreateTableOperation_all_settings()


What doesn't SQLite support here?

bricelam · 2020-01-06T22:36:07Z

test/EFCore.SqlServer.FunctionalTests/MigrationsSqlServerTest.cs


-                        b.HasKey("LoginProvider", "ProviderKey");
+        [ConditionalFact(Skip = "Doesn't work")]
+        public virtual async Task AlterColumnOperation_remove_identity()


test/EFCore.SqlServer.FunctionalTests/MigrationsSqlServerTest.cs

bricelam · 2020-01-06T22:37:27Z

test/EFCore.SqlServer.FunctionalTests/MigrationsSqlServerTest.cs


-                        b.ToTable("AspNetUserLogins");
-                    });
+        // AlterColumnOperation_remove_identity_legacy


This test is important to make sure that old model snapshots are handled correctly. Do we still have coverage for this?

I'll leave this test (and others like it) in MigrationSqlGeneratorTestBase and we can rediscuss later (will note in cleanup issue).

bricelam · 2020-01-06T22:46:09Z

test/EFCore.Sqlite.FunctionalTests/MigrationsSqliteTest.cs

        }

-        public override void Can_generate_one_up_script()
+        // In Sqlite, comments are only generated when creating a table


Yes. It's not possible to add these with ALTER TABLE..ADD statements.

roji

@bricelam @AndriySvyryd did a final big cleanup pass and addressed all comments. Another quick sign-off would be great.

The tests now use a conventionless model builder.
I've removed everything in the MigrationSqlGeneratorTest (unit) test suites that is covered by the newer MigrationsTest (functional) test suite. Some things still remain, we should ideally take another look at some point (Remaining clean up for migration tests #19668). I haven't touched the ModelDifferTests because this PR is already big enough (again, Remaining clean up for migration tests #19668).
I've renamed the migration tests and grouped them by type (table, column, index...) and then by operation (create, alter, drop...).

roji · 2020-01-17T15:15:24Z

test/EFCore.Relational.Specification.Tests/MigrationsTestBase.cs


        [ConditionalFact]
-        public virtual void Can_get_active_provider()
+        public virtual async Task CreateTableOperation_all_settings()


Schema, check constraints...

Am going to leave as-is (ignored for Sqlite) and we can re-discuss in cleanup if this test is valuable or not...

roji · 2020-01-17T15:17:40Z

test/EFCore.SqlServer.FunctionalTests/MigrationsSqlServerTest.cs


-                        b.HasKey("LoginProvider", "ProviderKey");
+        [ConditionalFact(Skip = "Doesn't work")]
+        public virtual async Task AlterColumnOperation_remove_identity()


Removed, thanks.

roji · 2020-01-17T15:19:14Z

test/EFCore.SqlServer.FunctionalTests/MigrationsSqlServerTest.cs


-                        b.ToTable("AspNetUserLogins");
-                    });
+        // AlterColumnOperation_remove_identity_legacy


I'll leave this test (and others like it) in MigrationSqlGeneratorTestBase and we can rediscuss later (will note in cleanup issue).

test/EFCore.SqlServer.FunctionalTests/MigrationsSqlServerTest.cs

roji · 2020-01-17T15:23:12Z

test/EFCore.Sqlite.FunctionalTests/MigrationsSqliteTest.cs

        }

-        public override void Can_generate_one_up_script()
+        // In Sqlite, comments are only generated when creating a table


Thanks, will leave these overrides to assert the actual behavior.

test/EFCore.SqlServer.FunctionalTests/MigrationsSqlServerTest.cs

AndriySvyryd · 2020-01-23T00:58:48Z

test/EFCore.SqlServer.FunctionalTests/MigrationsSqlServerTest.cs


-                        b.HasKey("Id");
+        [C


You'll need to create a test condition for online indexes.

Thanks. Added an enum value to SqlServerCondition and configured config.json to false for LocalDB, but am not sure how to set up true in CI. Will merge now and we can fix as part of #19668.

Closes #19039

roji requested a review from bricelam December 18, 2019 15:01

roji commented Dec 18, 2019

View reviewed changes

roji mentioned this pull request Dec 18, 2019

WIP on new migration tests #19258

Closed

AndriySvyryd reviewed Dec 20, 2019

View reviewed changes

bricelam approved these changes Jan 6, 2020

View reviewed changes

roji force-pushed the MigrationTests branch 2 times, most recently from 552280c to 9b8292d Compare January 22, 2020 16:12

roji commented Jan 22, 2020

View reviewed changes

AndriySvyryd approved these changes Jan 23, 2020

View reviewed changes

AndriySvyryd reviewed Jan 23, 2020

View reviewed changes

test/EFCore.SqlServer.FunctionalTests/MigrationsSqlServerTest.cs Show resolved Hide resolved

AndriySvyryd reviewed Jan 23, 2020

View reviewed changes

roji force-pushed the MigrationTests branch 2 times, most recently from 28eeadb to 1a03e66 Compare January 23, 2020 11:36

Implement functional migration tests

5aa0783

Closes #19039

roji force-pushed the MigrationTests branch from 1a03e66 to 5aa0783 Compare January 23, 2020 13:50

roji merged commit 09103e0 into master Jan 23, 2020

roji deleted the MigrationTests branch January 23, 2020 14:59

ajcvickers mentioned this pull request Jan 30, 2020

Entity Framework Weekly Status Updates (2020) #19549

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement functional migration tests #19353

Implement functional migration tests #19353

roji commented Dec 18, 2019 •

edited

Loading

roji left a comment

roji Dec 18, 2019

roji Dec 18, 2019

bricelam Jan 6, 2020

roji Jan 17, 2020

roji Dec 18, 2019

bricelam Jan 6, 2020

roji Jan 17, 2020

roji Dec 18, 2019

bricelam Jan 6, 2020

roji Jan 17, 2020

AndriySvyryd Dec 20, 2019

bricelam left a comment

bricelam Jan 6, 2020

bricelam Jan 6, 2020

bricelam Jan 6, 2020

roji Jan 17, 2020

bricelam Jan 6, 2020

roji left a comment

roji Jan 17, 2020

roji Jan 17, 2020

roji Jan 17, 2020

roji Jan 17, 2020

AndriySvyryd Jan 23, 2020

roji Jan 23, 2020

Implement functional migration tests #19353

Implement functional migration tests #19353

Conversation

roji commented Dec 18, 2019 • edited Loading

roji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bricelam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roji commented Dec 18, 2019 •

edited

Loading