Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Just write a test for it

kobzol.github.io

Post reply on HN
Print view View original

41–50 of 62 posts

Re: Just write a test for it

#41

Post by vlovich123 »

dreghgh wrote: post #30
vlovich123 wrote: post #17

Earlier quoted context omitted.

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

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".

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.

Re: Just write a test for it

#42

Post by lbreakjai »

In my previous job, we implemented something similar to Neon branching. Each MR would start by cloning the "public" schema into a schema scoped to the merge request, and only then run the migrations and the integration tests.

There's a range of errors you just won't catch until you run your code against the real thing. Especially if you write SQL directly, which feels like a lost art even amongst experienced developers.

Re: Just write a test for it

#43

Post by lelanthran »

wesselbindt wrote: post #19
nikolayasdf123 wrote: post #16

just look at this test for this one-liner DDL. the test is way more complex than thing it tests in first place. such confusing and hard to write and read tests is the reason people avoid writing tests in the first place. make tests great again! (great = simple, short, easy to write, read, maintain. at very least no more complex than the thing it testing!)

Totally with you on the merits of simplicity, writability, readability. But I'm getting strong "rest of the owl" vibes. How would you have prevented this defect instead, in a way that you find simple?

> How would you have prevented this defect instead, in a way that you find simple?

I currently prevent this sort of thing using a populated database. This database has actual real data (user info overwritten with random data).

Using a pre-populated database catches many more errors than this article's approach does.

Using a pre-populated database that has actual data generated by the users over a year or so catches even more errors.

This approach is fragile and misses the actual error, fixing only the symptom. The actual error is "there's a hole in my tests big enough to fly a passenger jet through".

The correct approach is to take a dump of the production database, scrub PII from it, and then perform your migration.

Re: Just write a test for it

#44

Post by wesselbindt »

karparov wrote: post #23
wesselbindt wrote: post #19

Earlier quoted context omitted.

Totally with you on the merits of simplicity, writability, readability. But I'm getting strong "rest of the owl" vibes. How would you have prevented this defect instead, in a way that you find simple?

Pre-populate the db.

This section:

> 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.

Suggests that this may also be a fairly complicated direction. Although it's not entirely clear to me why he can't just put one record in the db before any migrations, and then pull it all through. Plus it has the added drawback of removing your coverage for the (not unimportant) zero case.

Re: Just write a test for it

#45

Post by shepmaster »

null_deref wrote: post #37

Enjoyed the article. Am I out of touch or have the article linked to a PR of the rust-lang eco system that didn’t go through a CR, is this really the standard for such a large language standard library?

Agree with simonw’s sibling comment. To add to it, I’m the primary maintainer of the Rust playground and basically self-review every single commit.

The rust-lang/rust repository has higher scrutiny (in part driven by tools like bors, the subject of the article).

Re: Just write a test for it

#46

Post by kobzol »

bobnamob wrote: post #29

And now you've pulled in a full sql parser as a dependency (admittedly a dev/build time dependency, but a dependency nonetheless) in a project that has no business parsing sql. In this day and age of increasingly rampant supply chain attacks & dependency vulnerabilities, I'd definitely be second guessing the approach of "just write a test for it" if that test involved blowing up your attack/vuln surface

I don't really see an attack surface for a dev dependency.

Re: Just write a test for it

#47

Post by kobzol »

(author of the post)

Just to clarify a bit, the test ofc isn't a fully general solution to solving issues with database migrations (I hinted what that might be in the blog post), although it's still useful to provide a nice error message even if a more general solution was implemented.

That was not at all the goal of the post. I just wanted to appreciate how easy it was to achieve this specific task in Rust. In any other systems programming language that I used (even most other languages, except maybe for Python), I would never even imagine something like this being feasible, and so easy to do. That's it :)

Re: Just write a test for it

#48

Post by kobzol »

bsaul wrote: post #3

Sidenote : i found this code quite readable. I'm usually reading Rust code here on HN that's full of weird lifetime annotations or Dyn or super long generic types. As a non-rust dev this freaked me out. How common is this kind of code in practice ?

If you're writing applications or tests, it's mostly the simpler kind of code. If you're writing reusable (or perf. critical code), you will start seeing generics and lifetimes much more often.

Re: Just write a test for it

#49

Post by kobzol »

wesselbindt wrote: post #44
karparov wrote: post #23

Earlier quoted context omitted.

Pre-populate the db.

This section: > 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 w…

If I only insert data into the DB once, I could miss important states. Like, I could add non-NULL data to a NOT NULL column, then make it NULL, and then make it NOT NULL again. If I don't insert NULL into the column in-between the last two migrations, I won't trigger the issue.

Re: Just write a test for it

#50

Post by conradludgate »

kobzol wrote: post #46
bobnamob wrote: post #29

And now you've pulled in a full sql parser as a dependency (admittedly a dev/build time dependency, but a dependency nonetheless) in a project that has no business parsing sql. In this day and age of increasingly rampant supply chain attacks & dependency vulnerabilities, I'd definitely be second guessing the approach of "just write a test for it" if that test involved blowing up your attack/vuln surface

I don't really see an attack surface for a dev dependency.

Your development machine, potentially with API keys and access tokens in `$HOME`, is the attack surface
Post reply on HN