Persist Roles and Permissions between test runs

[devneill]

Problem

Roles and Permissions are cleared from the db after the first test run, so we are not able to create Users in subsequent tests.

Solution

Ensure Roles and Permissions are persisted between tests.

Approaches considered

A. Exclude Roles & Permissions tables from being dropped

We can explicitly exclude the Role, Permission and _PermissionToRole tables from being dropped between tests, by updating the SQL in cleanupDb().

This option is viable.

The downside of this approach is that if a user changes these tables in their schema, they will also need to know to update the cleanupDb function. It may be nicer to avoid these extra touch points.

B. Use prisma migrate reset

We attempted to use prisma's built in migration command - npx prisma migrate reset --force --skip-seed --skip-generate'.

The issue with this approach, is that we use execa to run this command, which needs to be run in a node environment, and some of our tests run in the jsdom vitest environment.

Because of this constraint, this option doesn't seem viable. A or C may be better options.

C. Use a custom reset script (used in this PR)

Instead of prisma migrate reset, we can manually drop all tables and run every migration by loading and running the SQL.

One potential downside of this approach is the complexity of importing, parsing and running each individual SQL statement from each migration file. We are using ; as the delimiter, which could break if any data in the SQL statment has this character anywhere except at the end of each SQL statement, but this isn't an issue right now.

An upside of this is that it doesn't add any extra touch points like we do in option A.

This is the approach used in this PR.

[kentcdodds]

I would rather stick with the previous solution we had and use spawn instead of execa.

[devneill]

Busy benchmarking to check performance of this approach

[devneill]

Benchmarks

main

Prisma reset via spawn

Custom reset script

Conclusion

Running prisma reset via spawn between test runs does work, but it is significantly slower unfortunately.

The less elegant approach of manually running the migrations in the custom reset script is still quite fast though. Should we go with that approach?
Or try and optimise the prisma reset somehow 🤔

[kentcdodds]

Running prisma reset via spawn between test runs does work, but it is significantly slower unfortunately.

This is what I expected and I was very surprised when it looked like it was not much slower in the previous implementation. So I'm actually kind of pleased to see this and know I'm not crazy 😅

I think going with the manual migration runs would be the best option. What do you think?

[devneill]

Awesome 💪🏻

I've updated to that approach and checked everything is working as expected:
✅ npx prisma db seed runs and seed data is correct
✅ npm test runs
✅ db is being cleared between each test run
✅ Roles and Permissions are persisted between test runs

One thing I noticed when doing the seed, is prisma sometimes logs out if a query took slightly longer:

It would be nice to silence that log so the seed logs are neat, but on the other hand it may be good be leave it there so it is clear when the reset is taking longer than usual 🤔

[kentcdodds]

I'm liking this. Thanks! The log from prisma is actually coming from our own client setup and I think it's good 👍

[kentcdodds]

Ok, got it now. Thanks!

[kentcdodds]

Looks like one of the unit tests is failing in CI. Based on the error it looks like the User table isn't being created properly 🤔 https://github.com/epicweb-dev/epic-stack/actions/runs/11023715807/job/30615526236?pr=854#step:7:38

[devneill]

Hmmm. I re-triggered the CI twice and it passed again, so it seems to be flakey.

I suspect its a timing issue, where a migration line that depends on the User table existing, is run before the line that creates the table. I thought the for of would run things sequentially though. 🤔

Perhaps you're right and things are in order, but the User table is not being successfully created.

Looking into it further 🧐

[devneill]

Ok, I have done three things:

1. Used for of loops instead of maps to ensure things run sequentially. I think this will solve any race conditions that may have caused the issue.
2. Wrapped the execution of each sql statement in a try catch, so if the flakey test does fail again, we have better error logging for it.
3. Added IF EXISTS to the table drop statement to make it more robust if a table is not there for any reason.

I am not sure what else to do until we get another CI failure 🤔 I haven't been able to reproduce the issue locally.

What do you think?

[kentcdodds]

I'm happy with this. Thank you!

[kentcdodds]

😬 failed build

[devneill]

It seems like there is still a race condition happening.

The error is intermittent, and sometimes the error changes(here and here), leading me to believe things are not always executing in the same order, causing different errors depending on the order.

I'm going to continue experimenting tomorrow. Will clean up all of these commits at the end 👍🏻

If you have any ideas for how to investigate/mitigate the possible race condition, please let me know 🙂

[devneill]

Things I've tried:

• Use for of loops to keep things sequential and avoid race conditions
• Use transaction + map combination, as this combo should also run sequentially
• Explicitly setting the transaction isolation level to Serializable to ensure the executions in a transaction are run sequentially (even though it should be doing this by default for SQLite)

None managed to avoid the race condition and intermittent errors 🤔

I think we can take two paths forward:

1. Use the current approach, but wrap it in a retry, so the occasional failure is ignored. This doesn't feel great though.
2. Go back full circle and take option A: We just exclude the Role, Permission and _PermissionToRole tables from being wiped between tests.

What do you think?

[devneill]

That seems promising 🤔 Great idea to use a different db tool ⚡

[kentcdodds]

Ok, I've run this in CI several times and it looks like it's working fine so I'm gonna merge. Thanks so much for all the iteration on this. I feel really good about this change!

[devneill]

Ah that's great news!

I'm glad it worked finally 😅

Thanks for your patience with all the back and forth 🙏🏻

[kentcdodds]

I love what we've done for our seed script.

I found a drastically simpler solution for the test side of things here: 7b16bdc