Save Role seeds between integration tests

[devneill]

I added a test to $username.test.tsx which required a User and the user to have a Role. The test failed because it couldn't create a new user and connect it to a Role, because there were no longer any Roles in the db.

Upon some deeper digging, I realised that the Role, Permission and PermissionToRole tables that we seed via manual seeds in the init migration, are being wiped between each single test run, due to cleanupDb() running in the afterEach().

This means the very first test is able to create a user with a role, but every subsequent test in the file can't.

To resolve this and and enable users with roles to be created in any test, I added a hard flag to cleanupDb(), to persist the tables above between test runs, if set to false.

I tried to do this more elegantly initially, by passing in the exact table names that should be persisted, but I got caught up with trying to interpolate variables into the raw SQL query, which didn't work. (See the source of my struggles here)

I resorted to the basic hard boolean argument. Please let me know if anyone can see a more elegant way to write this 🙏🏻

Test Plan

Checklist

• [ ] Tests updated
• [ ] Docs updated

Screenshots

[devneill]

Great point about reducing the number of touch points 👌🏻

I've attempted what I think you meant. It seems to work 🤔

Let me know your thoughts 🙂

[devneill]

I'll look into it!

[devneill]

Conclusion

The conclusion of the tests is that the custom reset approach is slightly slower, by ~10-30ms, when the 8 test migrations were added.

That time difference seems marginal to me, so I would recommend using the custom script to reduce the touch points. What do you think?

Results

Set up

To build a test case for more migrations, I made 8 migrations.

Each one either added or dropped an entire duplicate of the database (duplicates of the same tables, but with version suffixes, added alongside the original tables).

For example, each migration did this, but for every table in the db

Base case

With 8 migrations

[devneill]

On a separate note, I realised that the test migrations that dropped the tables, weren't being run correctly with the custom reset, because those SQL statements don't end with );

I've pushed a fix to use ; instead.

[devneill]

Yeah, using the prisma reset would be the cleanest and most reliable if it isn't too slow.

Trying to separate and rejoin each sql statement by detecting a ; doesn't feel nice and could lead to potential issues I think.

I did another batch of tests where we don't use cleanupDb() at all and we rather run setup() between each test. It seems like setup() does the prisma reset we want so I just re-used that function.

It seems to work well, there was no major slowdown by using setup instead 👍🏻

[alcpereira]

Just sharing a very useful library for benchmarking: hyperfine.

[kentcdodds]

This is great!

The only other place we use cleanupDb is in the seed script. I believe you can use reset with --no-seed so maybe that's what we should do in the seed script and then we can delete cleanupDb. Thoughts?

[devneill]

I removed cleanupDb, but it made me realise some potential downsides. It may make sense to leave cleanupDb for the seed script.

The potential issue with using a prisma reset for seeding, is that because the roles and permissions are created during migration, they can't be created again in the seed file, so those lines need to be removed - due to their unique constraint.

This makes it harder for people to customise roles and permissions for their own app's needs.

In my case, for example, I customised the roles/permission I wanted in the seed file, then used that generated SQL in the migration file.

Removing those lines in the seed file means someone will need to manually create their custom roles/permissions in the migration file, which is a lot more effort due to the uuids and relations.

Perhaps it's fine to leave cleanupDb as is for seeding, so that people can still customise the exact role and permission seeds they need, and then copy the sql into the migration when they are ready to test things.

It would be super nice to remove cleanupDb but I think it still has a use case 🤔

[devneill]

One potential simplification could be to move the contents of global-setup.ts into db-setup.ts as that variable and function are only used there.

I see it is used in the vite config 👍🏻

[kentcdodds]

This makes it harder for people to customise roles and permissions for their own app's needs.

This is true, but if they do wish to customize it they need to update the migration script anyway because the seed script doesn't run in production. So those lines should actually be removed anyway!

[kentcdodds]

Yeah man, this is awesome! Thank you so much for going through so many iterations!

[kentcdodds]

Part of me is wondering whether the global setup is necessary. If it's so fast to perform the migrations and stuff, maybe we could just do that in a beforeAll and avoid the global setup stuff. That would improve things because if the schema changes while we're running the tests we need to restart the tests to update the base database file. But if it's fast enough to do a resetDb in a beforeAll then the global setup can be removed. What do you think?

[devneill]

While messing around with removing the global setup, I've realised there is actually an issue with this change.

When setup() is called from the afterEach(), it never gets past this check, which means the db is never actually being reset between each test.

I tried replacing setup() with resetDb() in the afterEach(), but unfortunately the execa command can only run in a node environment. This means it fails for the tests that use the jsdom vitest environment. You can see this issue for more info.

Sorry for not catching this sooner 🤦🏻

Shall we revert this to minimise the impact on new users? Or I can make a new PR to take us back to the custom reset script approach?

[kentcdodds]

Let's revert and then we can take our time to decide what to do

[devneill]

I'm struggling to push commits while on the plane - will try again a bit later and when I land in ±4 hours