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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sync test DB from schema using its SHA1 #36870

Merged
merged 1 commit into from Aug 6, 2019
Merged

Conversation

jhawthorn
Copy link
Member

@jhawthorn jhawthorn commented Aug 6, 2019

Previously, we used the migration status to determine whether the test database(s) needed to be reloaded from the schema. This worked in most cases, but if a schema.rb was modified outside of migrations or if a migration was rolled back, it would require a manual db:test:prepare.

This commit updates load_schema to record the SHA1 of the loaded schema file inside the ar_internal_metadata table under a new schema_sha key. We can then use this SHA to determine whether we should reload the schema.

This ensures that the test DB stays exactly in sync with the schema file, including rollbacks which fixes a test marked TODO.

This came from #36826 where the motivation was to speed up parallel test DB creation, but I think we should make this change first and there's enough motivation to want it for non-parallel DB creation 馃槃

Copy link
Member

@eileencodes eileencodes left a comment

Choose a reason for hiding this comment

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

馃帀 Nice work @jhawthorn

@eileencodes eileencodes modified the milestones: 6.0.0, 6.1.0 Aug 6, 2019
@eileencodes
Copy link
Member

Moving this to 6.1 since what we'll backport will only be on parallel testing.

@jhawthorn jhawthorn modified the milestones: 6.1.0, 6.0.0 Aug 6, 2019
@jhawthorn
Copy link
Member Author

Moving this to 6.1 since what we'll backport will only be on parallel testing.

We flip-flopped on this for a bit 馃槄. It's a bit of a change the close to release but we figured we'd rather only have one implementation to worry about in 6.0 rather than having a different methods of syncing for single test DB vs parallel.


ActiveRecord::Base.establish_connection(configuration)
return false unless ActiveRecord::InternalMetadata.table_exists?
ActiveRecord::InternalMetadata[:schema_sha1] == schema_sha1(file)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to stop this information in the database? Can't it be a file on tmp?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question 鉂わ笍

I think the database is the best place for it, we want to store this exactly per-database: my_app_test separate from my_app_development, my_app_test-1 separate from my_app_test-2 when using parallelism. So storing it in the database itself I think accomplishes this the best. It also handles databases being dropped/restored from backup/etc.

I also think it's similar enough to the other information we store in the DB: very similar to schema migrations (metadata about the "structure" of the DB) and somewhat similar to environment (it records the state at creation/load) that it "feels" right to me.

Previously, we used the migration status to determine whether the test
database(s) needed to be reloaded from the schema. This worked in most
cases, but if a schema.rb was modified outside of migrations or if a
migration was rolled back, it would require a manual db:test:prepare.

This commit updates load_schema to record the SHA1 of the loaded schema
file inside of the ar_internal_metadata table. We can then use this SHA
to determine whether we should reload the schema.

This ensures that the test DB stays exactly in sync with the schema
file, including rollbacks which fixes a test marked TODO.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants