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

Add example test for River's Go migration API #99

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Dec 3, 2023

Follow up #67 by adding an example test for the new Go migration API.
Helps provide a more copy/pastable example for River's godoc, and
something we can link to from our other docs.

@brandur brandur requested a review from bgentry December 3, 2023 23:28
Copy link
Contributor

@bgentry bgentry left a comment

Choose a reason for hiding this comment

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

I'm a bit concerned this isn't a particularly useful or realistic example, but that may be unavoidable working with the constraints of executable tests alongside our broader test suite.

For example, is there a way we could use a new database for this test so that it's not already migrated when it starts? And then the tx.Rollback() at the end is obviously not what people would want in code, but is necessary for a repeatable example. Finally, the non-recommended usage of migrating down to -1 doesn't feel great in code that might get copy/pasted.

The only idea I have to resolve this is to create a brand new DB at the beginning of the test and then nuke it at the end via a defer. I'm not sure if that's better, but it might help keep all the unrealistic code grouped together, particularly if the create/drop logic is all confined to separate helper functions that don't clog up the example body.

On the -1 issue, if we made the above change then we could show a single down migration and let the defer take care of the remaining cleanup. That would still require updating the output whenever a new migration is added, but we already have that problem so 🤷‍♂️

Follow up #67 by adding an example test for the new Go migration API.
Helps provide a more copy/pastable example for River's godoc, and
something we can link to from our other docs.
@brandur brandur force-pushed the brandur-migrate-example-test branch from f06bf67 to 02e3d70 Compare December 7, 2023 02:12
@brandur
Copy link
Contributor Author

brandur commented Dec 7, 2023

Thanks!

Alright I ended up modifying this one so that we run an initial step that removes an existing River schema (in a helper so it's more hidden and not visible in the example), the migrate up, then migrate down. Both of the up/down migrations have been changed to target a specific version or number of steps, so that the example won't need to be updated in case new migrations are added in the future.

We still use a transaction though. I'm a little against using a special-cased database or technique like that because it introduces potential trouble around (1) setup/teardown, in that if say the test fails, we might not be guaranteed to be in the right state afterwards, and (2) concurrency issues — i.e. ability to run package tests in parallel. Already an issue, and it'd be nice not to make it any worse.

@brandur brandur merged commit f736d1d into master Dec 7, 2023
7 checks passed
@brandur brandur deleted the brandur-migrate-example-test branch December 7, 2023 04:57
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.

2 participants