-
Notifications
You must be signed in to change notification settings - Fork 0
[CMCSMACD-6648] Allow for a setup function in SchemaTest #25
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,7 +45,7 @@ func dump(c *PostgresConfig) ([]byte, error) { | |
| return out, err | ||
| } | ||
|
|
||
| func verifyNoTables(db *sqlx.DB) error { | ||
| func verifyNoRelations(db *sqlx.DB) error { | ||
| // Based on the query run for the "\d" command in psql | ||
| // (as revealed when started with -E flag). | ||
| q := `SELECT 1 FROM pg_catalog.pg_class c | ||
|
|
@@ -60,9 +60,9 @@ func verifyNoTables(db *sqlx.DB) error { | |
| // Expected | ||
| return nil | ||
| } else if err != nil { | ||
| return fmt.Errorf("Error checking for existing tables: %w", err) | ||
| return fmt.Errorf("Error checking for existing relations: %w", err) | ||
| } | ||
| return errors.New("Existing tables found. You must run SchemaTest on an empty database.") | ||
| return errors.New("Existing relations found. You must run this on an empty database.") | ||
| } | ||
|
|
||
| func migrateAndRollback(emptyDBConfig *PostgresConfig, db *sqlx.DB, allMigrations []NamedMigration, migrateToIndex, rollbackThroughIndex int, repeatForward bool) error { | ||
|
|
@@ -107,22 +107,9 @@ func migrateAndRollback(emptyDBConfig *PostgresConfig, db *sqlx.DB, allMigration | |
| return err | ||
| } | ||
|
|
||
| // Schema test expects a new *empty* postgres database. | ||
| // It will: | ||
| // 1. Apply all migrations | ||
| // 2. Reverse all migrations | ||
| // 3. For each migration: | ||
| // a. Apply the migration | ||
| // b. Reverse the migration | ||
| // c. Apply the migration again | ||
| // | ||
| // Before and after each step it will use pg_dump to dump the database schema. | ||
| // It will verify that: | ||
| // A. The schema is the same after reversing as before applying. | ||
| // B. (If re-applying) The schema is the same after applying as after re-applying. | ||
| // | ||
| // You must have `pg_dump` in your `PATH` to run this. | ||
| func SchemaTest(emptyDBConfig *PostgresConfig, allMigrations []NamedMigration) error { | ||
| // Does the same thing as SchemaTest but calls the provided setup function after verifying that the | ||
| // database is empty. | ||
| func SchemaTestWithSetup(emptyDBConfig *PostgresConfig, allMigrations []NamedMigration, setup func() error) error { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function should get the long comment. This suggestion adds the points that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually think it's better to have the long docstring on the standard function and not this variant. The caller is not required to remove objects that setup created. They are if they are going to call the function twice, but that is implied by the fact that SchemaTest requires an empty database.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. |
||
| for _, v := range []string{ | ||
| emptyDBConfig.Host, | ||
| emptyDBConfig.Port, | ||
|
|
@@ -141,10 +128,14 @@ func SchemaTest(emptyDBConfig *PostgresConfig, allMigrations []NamedMigration) e | |
| if err != nil { | ||
| return fmt.Errorf("Error connecting: %s", err) | ||
| } | ||
| err = verifyNoTables(db) | ||
| err = verifyNoRelations(db) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| err = setup() | ||
| if err != nil { | ||
| return fmt.Errorf("setup: %s", err) | ||
| } | ||
| err = Migrate(db, []NamedMigration{}) | ||
| if err != nil { | ||
| return fmt.Errorf("Setting up migrations table failed: %s", err) | ||
|
|
@@ -170,3 +161,22 @@ func SchemaTest(emptyDBConfig *PostgresConfig, allMigrations []NamedMigration) e | |
| } | ||
| return nil | ||
| } | ||
|
|
||
| // Schema test expects a new *empty* postgres database. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replace this comment with: |
||
| // It will: | ||
| // 1. Apply all migrations | ||
| // 2. Reverse all migrations | ||
| // 3. For each migration: | ||
| // a. Apply the migration | ||
| // b. Reverse the migration | ||
| // c. Apply the migration again | ||
| // | ||
| // Before and after each step it will use pg_dump to dump the database schema. | ||
| // It will verify that: | ||
| // A. The schema is the same after reversing as before applying. | ||
| // B. (If re-applying) The schema is the same after applying as after re-applying. | ||
| // | ||
| // You must have `pg_dump` in your `PATH` to run this. | ||
| func SchemaTest(emptyDBConfig *PostgresConfig, allMigrations []NamedMigration) error { | ||
| return SchemaTestWithSetup(emptyDBConfig, allMigrations, func() error { return nil }) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
setupis required to be non-nil, then the comment should say that.If
setupcan be nil, the code must check for it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the name makes clear that this function only exists to add a setup step to the standard SchemaTest. It would be silly to call it with a nil setup function. I think it's better to keep documentation concise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.