Ensure all data is removed during the teardown step #1714

tctrautman · 2021-02-02T22:26:51Z

This PR attempts to fix three issues with the removeScenario function that's run after a backend test is run:

Tests that use scenarios currently fail on Postgres databases that have database tables with PascalCasing.
removeScenario does not teardown records of data models that are not included in a scenario.
The removeScenario function would only remove records from the database if a scenario is present

We fix these issues by getting all prisma data models with their exact casing and issuing DELETE statements for each each of them, wrapping the model name in double quotes to maintain exact casing in Postgres. We do this regardless of whether or not a scenario is present, so I propose we rename this function from removeScenario to teardown.

At some point in the future we will likely want to replace this with a future Prisma implementation of transactions since issuing DELETE statements for every data model on every test run feels wasteful.

github-actions · 2021-02-02T22:31:33Z

📦 PR Packages

Click to Show Package Download Links

Install this PR by running yarn rw upgrade --pr 1714:0.24.0-8ff5f70

tctrautman · 2021-02-03T05:26:18Z

Hm. After using this change in development for the past few hours I believe I now understand why we are deleting data from tables using queryRaw -- it seems that the above change doesn't work with scenarios that contain relationships between models.

For example, in testing a signup flow (where a "signup" is an intermediate data model that gets updated through a multi-step process) I received the following error:

Invalid `prisma.signup.deleteMany()` invocation:


      The change you are trying to make would violate the required relation 'SignupToUser' between the `Signup` and `User` models.

      at PrismaClientFetcher.request (node_modules/@prisma/client/runtime/index.js:78130:15)
      at removeScenario (../node_modules/@redwoodjs/core/dist/configs/node/jest.setup.js:88:7)
      at Object.<anonymous> (../node_modules/@redwoodjs/core/dist/configs/node/jest.setup.js:136:7)

I'll give this some more thought and see if I can come up with a better solution. Thoughts / preferences for how to solve would be welcome!

thedavidprice · 2021-02-04T19:42:44Z

Ah, understood @tctrautman Thanks for getting this started!

Looping in @cannikin for all the potentially amazing ideas he (probably) has about this.

cannikin · 2021-02-04T19:58:39Z

So I would love it if this behaved the way that Rails does—the fixtures are put into the database inside of a transaction, and then at the end of the test the transaction is rolled back, leaving the database in the same state it was in when we started. But I don't think transactions were supported in Prisma when I wrote that code, or at least not in the form we would need them to be in (some way to save the transaction to a variable so it could be rolled back later). Maybe there's a way to define a named transaction in queryRaw() and then reference that same name later in a rollback? I haven't had to manually do transactions in SQL in years (Rails handles them really well automatically).

So I went more heavy handed and just tracked all models that had records created for them at the start of the test, and then delete all the records in those tables at the end of the test. I can't remember the exact reason I went with queryRaw() instead of deleteMany(), but being able to delete records without Prisma's integrity checks getting in the way may have been the reason. Also performance?

But yeah now that I'm looking I can see this being an issue in databases that enforce case sensitivity in table/column names (I ran all of this against SQLite and it didn't complain when I tried to DELETE FROM post even though the table name is Post). HMMM There's a function we use from prisma called getDMMF() that actually parses the schema file and gets all the stats out...maybe there's a way to use that to figure out the actual table name's case from the camelCase model name we use in the scenario itself?

tctrautman · 2021-02-06T03:32:51Z

Thanks for such a thoughtful response @cannikin! And thanks for connecting, @thedavidprice -- this was really helpful.

I don't think transactions were supported in Prisma when I wrote that code, or at least not in the form we would need them to be in (some way to save the transaction to a variable so it could be rolled back later).

I just took a look at the Prisma docs around and as far as I can tell this is still the case 😞

I can't remember the exact reason I went with queryRaw() instead of deleteMany(), but being able to delete records without Prisma's integrity checks getting in the way may have been the reason. Also performance?

Ah, yeah I assume this is what I ran into with the above error.

There's a function we use from prisma called getDMMF() that actually parses the schema file and gets all the stats out...maybe there's a way to use that to figure out the actual table name's case from the camelCase model name we use in the scenario itself?

Edit: The below conclusions are a bit out-dated after I had a chance to take a closer look at this.

Thanks, this was a great idea. I just dug into this a bit and as far as I can tell we can easily get an array of the Prisma data models (which, in Postgres, will be the names of the tables, but not other databases as you mentioned.). I haven't yet found a way to get table names specifically, but I'll take a closer look this weekend with a sqlite project to see if that turns anything else up.

If I don't have any luck with that, I could take stab at writing a function to turn prisma data models into table names like so:

In postgres: the data model "BlogPost" would return the table name "BlogPost"
In sqlite: the data model "BlogPost" would return the table name "blog_post"
etc..

This assumes that I can find a way to get the database type that a user is using (seems like a safe assumption, but it's not entirely clear to me where to get that.)

It would be nice to avoid this, though, so hopefully I can just find a way to get table names from Prisma.

tctrautman · 2021-02-08T00:59:59Z

@cannikin whenever you have a moment I'd love to get your feedback on this. I've just updated the PR description above with my updated approach.

tctrautman · 2021-02-08T01:47:15Z

Ah, I believe I've discovered another situation where removeScenario doesn't quite do what we want it to: cases where the tested function creates a record of a model that is not in any of your fixtures. This happens because (if I understand correctly,) we're only iterating through and deleting models that appear in you fixtures.

It wouldn't be the most performant, but a simple solution to this would be to issue a DELETE statement for every Prisma model. If that's what we'd like to do I'd be happy to include that in this PR.

And I'm rusty here, but maybe there's a way to do this in a single statement?

jvanbaarsen · 2021-02-09T13:24:32Z

@tctrautman I can confirm two of your assumptions:

If you don't have a scenario for every model in your system the DB is not fully cleared
If you have a model that starts with a capital letter (Or PascalCasing as you said), things don't work for postgresql, in sqlite that isn't a problem.

tctrautman · 2021-02-09T16:33:51Z

Thanks @jvanbaarsen! I've updated this PR to fix the second issue with a change I've been using in local development for the last few days.

jvanbaarsen · 2021-02-09T17:50:16Z

@tctrautman Thanks! I'll make sure to check that out against my codebase to see if this solves the problem.

jvanbaarsen · 2021-02-09T17:54:18Z

packages/core/src/configs/node/jest.setup.js

@@ -50,8 +51,12 @@ const removeScenario = async (scenario) => {
    // get unique model names only
    models = Array.from(new Set(models))

-    for (const model of models) {
-      await db.$queryRaw(`DELETE FROM ${model}`)
+    const prismaModelNames = (


We are now running this for every scenario, would it make sense to instead of attaching this to if a scenario is present, run this every time a test has ran? So basically make it a poor mans transaction?

Great catch -- I hadn't realized it would miss cases where we weren't using scenarios, but were adding records to the database. I believe that case is now taken care of, and I've renamed the function to reflect this.

cannikin · 2021-02-11T01:13:35Z

@tctrautman I still see code changes coming in so I've been holding off on reviewing. Let me know when you think it's ready!

tctrautman · 2021-02-11T01:22:23Z

Thanks @cannikin!

The scope of the PR is now a little wider than it was last week (description above is up to date,) but it's ready for your review whenever you have a moment.

And for what it's worth, consider it battle tested on ~35 tests I have running locally on Postgres.

cannikin · 2021-02-11T01:37:39Z

Oh damn that's way simpler. Thanks!

tctrautman · 2021-02-11T01:49:36Z

@cannikin happy to do it -- thanks for the direction!

thedavidprice · 2021-02-11T02:25:39Z

Ah, I see this one got merged. All good -- was just going to ask if it was appropriate to use the new Prisma migrate reset command for this in the place of what is currently the teardown().

No need to change at this point if it's even applicable.

cannikin · 2021-02-11T04:03:35Z

This code runs after every test to remove any records that were created. We definitely don’t want to do a migrate reset at that point, it would take forever. I was suggesting doing it when the test suite starts to make sure the DB starts in the right state.

thedavidprice · 2021-02-11T04:11:10Z

Roger that.

I was suggesting doing it when the test suite starts to make sure the DB starts in the right state.

I start the test DB setup now with prisma db push so we never have to mess with migration steps / shadow DB. Been working well in local testing.

…ender-p1 * 'main' of github.com:redwoodjs/redwood: v0.25.0 yarn rw prisma commands now show a little tip (redwoodjs#1772) FIx typo (redwoodjs#1779) Ensure all data is removed during the teardown step (redwoodjs#1714) [v0.25] revert to @prisma/cli; recreate yarn.lock (redwoodjs#1774) use prisma db push for test DB (redwoodjs#1768) Disable --version and -v for prisma alias. (redwoodjs#1767) Rebuild Prisma Client between upgrades (redwoodjs#1155) upgrade Prisma v2.16.1 (redwoodjs#1766) [v0.25] Remove warning if you scaffold a model with a relation (redwoodjs#1757) [v0.25] Scaffolded many-to-many relationship fixes (redwoodjs#1758) add forrest and kim-adeline; update all contribs (redwoodjs#1763) Run clean and then build the js. Fix: applying afterQuery in storybook mocks (redwoodjs#1740) restructure and update Deploy command and change Setup Deploy command template [breaking] (redwoodjs#1747) Fixes prisma command when path has spaces Make tutorial work with workspaces. Add "packages/*" to the template.

Delete model regardless of capitalization

38ba18c

thedavidprice requested review from cannikin and removed request for cannikin February 4, 2021 19:41

thedavidprice marked this pull request as draft February 4, 2021 19:41

tctrautman added 5 commits February 7, 2021 15:59

Use PascalCasing in DELETE sql statement

789c9c2

Use exact casing of prisma models

8b56d9b

Fix linting

64c4aef

Formatting

4cafc23

One more formatting change

9ee7146

cannikin mentioned this pull request Feb 8, 2021

DB is not truncated after Scenario's were added #1756

Closed

DELETE each prisma model, even if it isn't in a scenario

6e1f224

tctrautman marked this pull request as ready for review February 9, 2021 16:33

Merge branch 'main' into fix-scenario-deletion

a403b45

tctrautman changed the title ~~Improve scenario teardown casing resiliency~~ Improve scenario teardown resiliency Feb 9, 2021

jvanbaarsen reviewed Feb 9, 2021

View reviewed changes

tctrautman added 3 commits February 9, 2021 18:05

Delete all records regardless of whether there is a scenario

3c9f9a9

Styling, remove unused code

8d4ac1a

Merge remote-tracking branch 'upstream/main' into fix-scenario-deletion

cb28084

tctrautman changed the title ~~Improve scenario teardown resiliency~~ Ensure all data is removed during the teardown step Feb 10, 2021

Merge branch 'main' into fix-scenario-deletion

8ff5f70

cannikin approved these changes Feb 11, 2021

View reviewed changes

cannikin merged commit b78132d into redwoodjs:main Feb 11, 2021

cannikin added next release bug/confirmed We have confirmed this is a bug labels Feb 11, 2021

thedavidprice added this to the v0.25.0 milestone Feb 11, 2021

thedavidprice removed bug/confirmed We have confirmed this is a bug next release labels Feb 11, 2021

thedavidprice mentioned this pull request Feb 12, 2021

Running tests while pointed to a postgresql database causes sql error 42P01 relation "post" does not exist #1782

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure all data is removed during the teardown step #1714

Ensure all data is removed during the teardown step #1714

tctrautman commented Feb 2, 2021 •

edited

Loading

github-actions bot commented Feb 2, 2021 •

edited

Loading

tctrautman commented Feb 3, 2021

thedavidprice commented Feb 4, 2021

cannikin commented Feb 4, 2021

tctrautman commented Feb 6, 2021 •

edited

Loading

tctrautman commented Feb 8, 2021

tctrautman commented Feb 8, 2021

jvanbaarsen commented Feb 9, 2021

tctrautman commented Feb 9, 2021

jvanbaarsen commented Feb 9, 2021

jvanbaarsen Feb 9, 2021

tctrautman Feb 10, 2021

cannikin commented Feb 11, 2021

tctrautman commented Feb 11, 2021

cannikin commented Feb 11, 2021

tctrautman commented Feb 11, 2021

thedavidprice commented Feb 11, 2021

cannikin commented Feb 11, 2021

thedavidprice commented Feb 11, 2021 •

edited

Loading

Ensure all data is removed during the teardown step #1714

Ensure all data is removed during the teardown step #1714

Conversation

tctrautman commented Feb 2, 2021 • edited Loading

github-actions bot commented Feb 2, 2021 • edited Loading

📦 PR Packages

tctrautman commented Feb 3, 2021

thedavidprice commented Feb 4, 2021

cannikin commented Feb 4, 2021

tctrautman commented Feb 6, 2021 • edited Loading

tctrautman commented Feb 8, 2021

tctrautman commented Feb 8, 2021

jvanbaarsen commented Feb 9, 2021

tctrautman commented Feb 9, 2021

jvanbaarsen commented Feb 9, 2021

jvanbaarsen Feb 9, 2021

Choose a reason for hiding this comment

tctrautman Feb 10, 2021

Choose a reason for hiding this comment

cannikin commented Feb 11, 2021

tctrautman commented Feb 11, 2021

cannikin commented Feb 11, 2021

tctrautman commented Feb 11, 2021

thedavidprice commented Feb 11, 2021

cannikin commented Feb 11, 2021

thedavidprice commented Feb 11, 2021 • edited Loading

tctrautman commented Feb 2, 2021 •

edited

Loading

github-actions bot commented Feb 2, 2021 •

edited

Loading

tctrautman commented Feb 6, 2021 •

edited

Loading

thedavidprice commented Feb 11, 2021 •

edited

Loading