[CMCSMACD-6648] Allow for a setup function in SchemaTest by hundt-corbalt · Pull Request #25 · corbaltcode/go-libraries

hundt-corbalt · 2026-07-14T14:50:23Z

This allows callers to set up prerequisite data or tables that are needed for the migrations without running afoul of the empty- database check at the beginning of SchemaTest.

Testing: pulled this into professor-mac and successfully used it to allow app migrations to depend on teamsapi migrations.

New automated tests have been written to the extent possible.
The code has been checked for structural/syntactic validity.
- AMI/application: a build was performed
- terraform changes: "terraform plan" checked on every affected environment
(If applicable) the code has been manually tested on our infrastructure.
- AMI/application: deployed an a test or dev environment
- terraform changes: applied to test or dev environment
- script: run against test or dev environment
Likely failure points and new functionality have been identified and tested manually.
Examples:
- Application manually run in a way that triggers any new branches
- AMI logged into and changes verified from login shell
Pull request description includes a description of all the manual steps performed to accomplish the above.

This allows callers to set up preerquisite data or tables that are needed for the migrations without running afoul of the empty- database check at the beginning of SchemaTest.

leslie-corbalt

cmd/schema-test/README.md, line 3 is stale.

My comments are related to additional clarification and preciseness.

leslie-corbalt · 2026-07-15T17:07:01Z

A more precise message:
Existing relations found. You must run this on an empty database

Maybe omit specifying SchemaTest, since it can also be called by SchemaTestWithSetup?

leslie-corbalt · 2026-07-15T17:08:04Z

This is not part of this change, but a better function name is verifyNoRelations since \d returns user relations.

leslie-corbalt · 2026-07-15T17:09:29Z

Replace tables with relations

leslie-corbalt · 2026-07-15T19:03:57Z

-func SchemaTest(emptyDBConfig *PostgresConfig, allMigrations []NamedMigration) error {
+// Does a SchemaTest but calls the provided setup function after verifying that the
+// database is empty.
+func SchemaTestWithSetup(emptyDBConfig *PostgresConfig, allMigrations []NamedMigration, setup func() error) error {


If setup is required to be non-nil, then the comment should say that.
If setup can be nil, the code must check for it.

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.

Makes sense.

leslie-corbalt · 2026-07-15T19:42:37Z

 	return nil
 }
+
+// Schema test expects a new *empty* postgres database.


Replace this comment with:
// SchemaTest runs SchemaTestWithSetup with no setup step.

leslie-corbalt · 2026-07-15T19:44:16Z

-func SchemaTest(emptyDBConfig *PostgresConfig, allMigrations []NamedMigration) error {
+// Does a SchemaTest but calls the provided setup function after verifying that the
+// database is empty.
+func SchemaTestWithSetup(emptyDBConfig *PostgresConfig, allMigrations []NamedMigration, setup func() error) error {


This function should get the long comment. This suggestion adds the points that

the caller must remove objects that the setup created before running another schema test on the same database

setup() must be non-nil (based on the current code)

// SchemaTestWithSetup expects a new *empty* postgres database. After confirming // the database is empty, it calls setup once - e.g. to create schemas or tables // the migrations depend on - and then: // 1. Applies all migrations // 2. Reverses all migrations // 3. For each migration: // a. Applies the migration // b. Reverses the migration // c. Applies the migration again // // Before and after each step, it uses pg_dump to dump the schema, and verifies: // A. The schema after reversing matches the schema before applying. // B. (If re-applying) The schema after applying matches the schema after re-applying. // // Whatever setup is created is treated as the baseline schema: it is not rolled // back or dropped, so the caller must remove it before running another schema // test on the same database. setup must not be nil. // // You must have `pg_dump` in your `PATH` to run this.

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.

Makes sense.

hundt-corbalt

Thanks for the review!

hundt-corbalt · 2026-07-16T13:17:37Z

hundt-corbalt · 2026-07-16T13:17:53Z

hundt-corbalt · 2026-07-16T13:18:31Z

-func SchemaTest(emptyDBConfig *PostgresConfig, allMigrations []NamedMigration) error {
+// Does a SchemaTest but calls the provided setup function after verifying that the
+// database is empty.
+func SchemaTestWithSetup(emptyDBConfig *PostgresConfig, allMigrations []NamedMigration, setup func() error) error {


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.

hundt-corbalt · 2026-07-16T13:20:33Z

-func SchemaTest(emptyDBConfig *PostgresConfig, allMigrations []NamedMigration) error {
+// Does a SchemaTest but calls the provided setup function after verifying that the
+// database is empty.
+func SchemaTestWithSetup(emptyDBConfig *PostgresConfig, allMigrations []NamedMigration, setup func() error) error {


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.

hundt-corbalt · 2026-07-16T13:20:51Z

leslie-corbalt · 2026-07-16T16:44:52Z

-//
-// You must have `pg_dump` in your `PATH` to run this.
-func SchemaTest(emptyDBConfig *PostgresConfig, allMigrations []NamedMigration) error {
+// Does a SchemaTest but calls the provided setup function after verifying that the


The capitalization SchemaTest sounds like SchemaTestWithSetup calls SchemaTest but calls the provided setup after verifying that the database is empty.

This comment is confusing.

How about now?

leslie-corbalt

A comment on your short docstring.

leslie-corbalt

LGTM

[CMCSMACD-6648] Allow for a setup function in SchemaTest

db2ea4d

This allows callers to set up preerquisite data or tables that are needed for the migrations without running afoul of the empty- database check at the beginning of SchemaTest.

hundt-corbalt requested a review from leslie-corbalt July 14, 2026 14:50

leslie-corbalt reviewed Jul 15, 2026

View reviewed changes

leslie-corbalt assigned hundt-corbalt Jul 15, 2026

hundt-corbalt commented Jul 16, 2026

View reviewed changes

Address review comments

8856b8c

leslie-corbalt reviewed Jul 16, 2026

View reviewed changes

Adjust comment

e605b31

leslie-corbalt approved these changes Jul 16, 2026

View reviewed changes

hundt-corbalt merged commit c57eab5 into main Jul 16, 2026
1 check passed

Uh oh!

Conversation

hundt-corbalt commented Jul 14, 2026

Uh oh!

leslie-corbalt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hundt-corbalt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leslie-corbalt left a comment

Choose a reason for hiding this comment

Uh oh!

leslie-corbalt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants