Closed devneill closed 2 weeks ago
I would rather stick with the previous solution we had and use spawn instead of execa.
Busy benchmarking to check performance of this approach
main
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 π€
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?
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 π€
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
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 π§
Ok, I have done three things:
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.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?
π¬ failed build
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 π
Things I've tried:
for of
loops to keep things sequential and avoid race conditionsSerializable
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:
Role
, Permission
and _PermissionToRole
tables from being wiped between tests.What do you think?
That seems promising π€ Great idea to use a different db tool β‘
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!
Ah that's great news!
I'm glad it worked finally π
Thanks for your patience with all the back and forth ππ»
I love what we've done for our seed script.
I found a drastically simpler solution for the test side of things here: https://github.com/epicweb-dev/epic-stack/commit/7b16bdc8800efba13fa5949133b06b6b9ce725a6
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 incleanupDb()
.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 anode
environment, and some of our tests run in thejsdom
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.