Hacker News Reader

Comment by dreghgh | Hacker News Reader

This is the wrong approach. He should be putting data in the database schema before trying to run migrations on it.

Doing that is simple and can potentially catch all sorts of bugs, including the bugs you didn't think of yet. His solution is complicated but only catches one very specific type of bug.

View on HN

The author mentions that approach at the end of the post:

> Apart from parsing the SQL query, I also considered an alternative testing approach that I might implement in the future: go through each migration one by one, and insert some dummy data into the database before applying it, to make sure that we test each migration being applied on a non-empty database. The data would either have to be generated automatically based on the current database schema, or we could commit some example DB dataset together with each migration, to make sure that we have some representative data sample available.

That would certainly catch nearly all migration issues, but it doesn't provide a helpful error message like a test for a specific mistake like this does.

Ideally both approaches would be used, with the general case being used to detect and inform more targeted tests.

The author correctly notes the challenge of correctly populating some pre-migration data at each step.

Does it need to be done at each step, though? Couldn't the test data be added at just one point (where the schema is known) and just let it run through all the subsequent migrations to see if it goes boom?

Ideally imo you'd have a property based test that does the following:

Perform some arbitrary list of valid actions. (This alone is valuable)

Run the new migrations.

Perform some arbitrary list of valid actions.

Assert no crashes/errors.

I've found this great for testing APIs - just "perform some list of user actions and make sure things don't explode" can catch a lot.

I have also found this kind of approach very valuable. With defensive code, where the code itself does a bit of the kind of correctness checking that one might conventionally put into tests, it works even better. Pre/post- conditions in functions for example.

Not if you could have data that could only have been inserted at some stage to behind with and then a subsequent migration only breaks that case (eg create table in step 10 and step 11 breaks that new table - if you only had data that you inserted in step 1, it would still pass all migrations. In other words, you need to have sample data that exercises the entire DDL that is added or you risk missing something.

Normally you have per-migration tests for non-trivial schema migrations at minimum...

You should solve the problem you need to solve, not the problem you have a cool solution for.

The problem to solve is "given my database in an arbitrary but valid state, applying a migration should succeed and leave the database in a equally valid state". Not "how do I stop people adding NOT NULL columns into tables".

You're just over-generalizing the problem into something you will not be able to solve by proving that a migration works for a _specific_ arbitrary state.

I mean this is a widely adopted practice so it clearly is not impossible to solve.

All tests substitute small collections of representative inputs for the generality of inputs.

Of course, but then let's not knock the OP article for covering a different subset of testable states, in a way that will need no adjusting for future schema changes, while still preventing the actual problem.

The author had an off-the-shelf crate they could trivially add to their project & prevent instances of this specific failure in 5 minutes even though solving the general case is possible but probably significantly harder & probably still contains various corner cases that are hard to completely eliminate.

I'd characterize it less as the cool solution and more as the quick & dirty solution that gets you a lot of value for little effort.

Writing a populate script for every migration sounds like a huge overkill.

Reply on news.ycombinator.com