Skip to content

Commit

Permalink
remove cleanup db util
Browse files Browse the repository at this point in the history
  • Loading branch information
kentcdodds committed Oct 28, 2024
1 parent 3507073 commit d52803a
Show file tree
Hide file tree
Showing 5 changed files with 47 additions and 69 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/deploy.yml
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ jobs:

- name: 🌱 Seed Database
if: steps.db-cache.outputs.cache-hit != 'true'
run: npx prisma db seed
run: npx prisma migrate reset

- name: 🏗 Build
run: npm run build
Expand Down
45 changes: 45 additions & 0 deletions docs/decisions/038-remove-cleanup-db.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# Remove Cleanup DB

Date: 2024-10-28

Status: accepted

## Context

We have a utility called `cleanupDb` that removes all tables from the database
except for prisma migration tables. The reference to prisma migration tables is
unfortunate because those are an implementation detail that we should not have
to think about.

The goal of `cleanupDb` was to make it easy for tests to reset the database
without having to run `prisma migrate reset` which is too slow for lower level
tests.

We also used `cleanupDb` in the seed file to reset the database before seeding
it.

However, after a lot of work on the tests, we found a much simpler solution to
resetting the database between tests: simply copy/paste the `base.db` file
(which is a fresh database) to `test.db` before each test. We were already doing
this before all the tests. It takes nanoseconds and is much simpler.

For the seed script, it's nice to have the database be completely reset when
running `prisma db seed` (in fact, our seed expects the database to be empty),
but you can get the same behavior as our current `seed` with a fresh database by
running `prisma migrate reset` (which runs the seed script after resetting the
database).

It would be nice to ditch the implementation detail of prisma's tables, so we'd
like to remove this utility.

## Decision

Remove the `cleanupDb` utility and update our CI to run `prisma migrate reset`
instead of `prisma db seed`.

## Consequences

Running `prisma db seed` will fail because the seed script expects the database
to be empty. We could address this by using upsert or something, but really
people should just run `prisma migrate reset` to seed the database (which is
effectively what we used to do before removing `cleanupDb`).
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"prisma:studio": "prisma studio",
"format": "prettier --write .",
"lint": "eslint .",
"setup": "npm run build && prisma generate && prisma migrate deploy && prisma db seed && playwright install",
"setup": "npm run build && prisma generate && prisma migrate reset && playwright install",
"start": "cross-env NODE_ENV=production node .",
"start:mocks": "cross-env NODE_ENV=production MOCKS=true tsx .",
"test": "vitest",
Expand Down
5 changes: 0 additions & 5 deletions prisma/seed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { promiseHash } from 'remix-utils/promise'
import { prisma } from '#app/utils/db.server.ts'
import { MOCK_CODE_GITHUB } from '#app/utils/providers/constants'
import {
cleanupDb,
createPassword,
createUser,
getNoteImages,
Expand All @@ -16,10 +15,6 @@ async function seed() {
console.log('🌱 Seeding...')
console.time(`🌱 Database has been seeded`)

console.time('🧹 Cleaned up the database...')
await cleanupDb()
console.timeEnd('🧹 Cleaned up the database...')

const totalUsers = 5
console.time(`👤 Created ${totalUsers} users...`)
const noteImages = await getNoteImages()
Expand Down
62 changes: 0 additions & 62 deletions tests/db-utils.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import fs from 'node:fs'
import { faker } from '@faker-js/faker'
import bcrypt from 'bcryptjs'
import Database from 'better-sqlite3'
import { UniqueEnforcer } from 'enforce-unique'

const uniqueUsernameEnforcer = new UniqueEnforcer()
Expand Down Expand Up @@ -114,64 +113,3 @@ export async function img({
blob: await fs.promises.readFile(filepath),
}
}

let _migrationSqls: Array<Array<string>> | undefined
async function getMigrationSqls() {
if (_migrationSqls) return _migrationSqls

const migrationSqls: Array<Array<string>> = []
const migrationPaths = (await fs.promises.readdir('prisma/migrations'))
.filter((dir) => dir !== 'migration_lock.toml')
.map((dir) => `prisma/migrations/${dir}/migration.sql`)

for (const path of migrationPaths) {
const sql = await fs.promises.readFile(path, 'utf8')
const statements = sql
.split(';')
.map((statement) => statement.trim())
.filter(Boolean)
migrationSqls.push(statements)
}

_migrationSqls = migrationSqls

return migrationSqls
}

export async function cleanupDb() {
const db = new Database(process.env.DATABASE_URL!.replace('file:', ''))

try {
// Disable FK constraints to avoid relation conflicts during deletion
db.exec('PRAGMA foreign_keys = OFF')

// Get all table names
const tables = db
.prepare(
`
SELECT name FROM sqlite_master
WHERE type='table' AND name NOT LIKE 'sqlite_%' AND name NOT LIKE '_prisma_migrations'
`,
)
.all() as { name: string }[]

// Delete tables except the ones that are excluded above
for (const { name } of tables) {
db.exec(`DROP TABLE IF EXISTS "${name}"`)
}

// Get migration SQLs and run each migration
const migrationSqls = await getMigrationSqls()
for (const statements of migrationSqls) {
// Run each sql statement in the migration
db.transaction(() => {
for (const statement of statements) {
db.exec(statement)
}
})()
}
} finally {
db.exec('PRAGMA foreign_keys = ON')
db.close()
}
}

3 comments on commit d52803a

@devneill
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, that's much better 👏

@JMartinCollins
Copy link

Choose a reason for hiding this comment

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

That was a quick response! Thanks!

@kentcdodds
Copy link
Member Author

Choose a reason for hiding this comment

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

I've been thinking about this for a while 😅

Please sign in to comment.