boolean-uk / software-developer

0 stars 0 forks source link

Database ORM test suite #168

Closed vherus closed 1 year ago

vherus commented 1 year ago

Investigate if there's a built-in way to test schema files for https://github.com/boolean-uk/database-orm-1

Alternatively, test suite that tests seed functions

vherus commented 1 year ago

Prisma has a guide for mocking the client specifically for unit testing: https://www.prisma.io/docs/guides/testing/unit-testing

dearshrewdwit commented 1 year ago

What, spinning up db servers within a GitHub action for every exercise every single time a a cohort full of students pushes work isn't cool?

julesnuggy commented 1 year ago

I went through the Prisma guide on Unit Testing and it is not suitable for what we want (testing the schema) or for our project setup (JS and not TS). The various issues are:

  1. The jest-mock-extended library does not work as intended (i.e. to mock the prisma client and its functions). It fails to even read the prisma models, so prisma.customer.create will throw an error because it is undefined despite having setup the mock prisma client.
  2. This library is intended for TS projects, and whilst it could in theory be made to work by having TS files only for the test suite, the unit testing that Prisma outline are for unit testing custom DB query functions (e.g. createCustomer), as opposed to the schema itself. So the only "model checking" is via the TS types/interfaces that we would create purely for the tests! And given we mock the resolved value, it's really quite pointless for us.

I think the only way to test the schema would be to implement Integration Testing: https://www.prisma.io/docs/guides/testing/integration-testing#prerequisites However, this is also not ideal because:

  1. We would need students to setup their own instances of a test DB.
  2. We would need to setup GitHub Actions.
  3. Students would need to correctly configure their repo secrets/env vars for the tests to be able to run.
julesnuggy commented 1 year ago

We spoke briefly about using Jasmine to test that the Prisma Client is able to generate the models/data, but if that's all we need to do then surely we could just stick with pure Jest?

Image

vherus commented 1 year ago

I personally prefer Jest so have no problem with using it but the rest of the exercises use Jasmine so it may add confusion for students when they suddenly have a different test command to run

julesnuggy commented 1 year ago

Ah OK, then I think consistency for the students will be more important in this case!

dearshrewdwit commented 1 year ago

Important: You need to re-run the prisma generate command after every change that's made to your Prisma schema to update the generated Prisma Client code.

...

Note also that prisma generate is automatically invoked when you're installing the @prisma/client npm package. So, when you're initially setting up Prisma Client, you can typically save the third step from the list above.

This sounds like in the GitHub action it will be fine - because it will install the project from scratch.

If students run the test locally, we'll have to simply include the command npx prisma generate before running the tests

eg: npx prisma generate && npx jest

Or potentially add a helper file to the test suite run which awaits a system call? https://nodejs.org/docs/latest/api/child_process.html#child_processexeccommand-options-callback

dearshrewdwit commented 1 year ago

I personally prefer Jest so have no problem with using it but the rest of the exercises use Jasmine so it may add confusion for students when they suddenly have a different test command to run

The APIs & Datastores module uses jest as the test runner for exercises and challenges - so I recommend using it.

You might be thinking of Dev Approaches?

julesnuggy commented 1 year ago

@dearshrewdwit @vherus -- I've been investigating deeper and trying various approaches, and I don't think it's as easy as we'd hoped 😢 Here are my findings:

Local testing still uses the dev DB

The DB connection is defined in schema.prisma file, which refers to the student's .env file for the DB URL. So, locally for the students, the tests will run against their dev DBs -- this could be annoying/confusing as it will pollute their DB with test data. I guess we could mitigate this by setting up teardown scripts in the tests by storing the IDs of new data and making a delete query in an afterEach block.

Env Vars for Testing via GitHub Actions

However, the issue then is how to run the tests when pushed to GitHub, as the .env file is not committed, so the Prisma Client won't know where to connect. This means we need to setup GitHub secrets/env vars -- I don't think we could make a single cloud test DB as it might be overloaded with requests when multiple Actions are being run.

We could then alternatively have the students setup a separate test DB in Elephantsql and configure the env vars in their own repos so that at most there are 2 repos (for a given pair) making requests to a test DB when an Action is run. But then this relies on the student correctly setting this up.

Multiple Prisma Clients

I have tried to setup a local sqlite DB for the tests to run against using the @databases/sqlite library, but the issues are that:

  1. We cannot use multiple providers -- this datasource property can only be defined in the schema.prisma file and cannot be overridden in the instantiated Prisma client (only the url property can).
  2. The Prisma Client doesn't recommend having multiple instances, and we'd still run into the same env var config issue above.

    Your application should generally only create one instance of PrismaClient. How to achieve this depends on whether you are using Prisma in a long-running application or in a serverless environment .

    The reason for this is that each instance of PrismaClient manages a connection pool, which means that a large number of clients can exhaust the database connection limit. This applies to all database connectors.

Multiple Schema

Lastly, I investigated creating a new schema and it's sadly not as neat/easy as the Shadow DB schema. If we want to connect to multiple schema, then the prisma.schema models will require [mapping of tables to each schema so as to avoid name conflicts}(https://www.prisma.io/docs/guides/database/multi-schema#tables-with-the-same-name-in-different-database-schemas). So, again, this would be a very fiddly approach.

vherus commented 1 year ago

I think what we should look into is not even touching a data source and focusing on asserting the objects that get generated by the prisma client. When npx prisma generate runs, prisma will create a bunch of types under the hood. E.g., the team-dev-server has these types like the one below defined from prisma generate:

export type UserSelect = {
    id?: boolean
    email?: boolean
    password?: boolean
    role?: boolean
    profile?: boolean | ProfileArgs
    cohortId?: boolean
    cohort?: boolean | CohortArgs
    posts?: boolean | PostFindManyArgs
    deliveryLogs?: boolean | DeliveryLogFindManyArgs
    _count?: boolean | UserCountOutputTypeArgs
  }

Jest has a bit of functionality called "spies", so we may be able to spy on prisma client calls and just assert that they're being called with the expected object structures: https://jestjs.io/docs/jest-object#jestspyonobject-methodname

dearshrewdwit commented 1 year ago

might even be simpler to do something like this using ts-jest - create some objects that obey the model's type in each test

import type { Customer, User } from '@prisma/client'; // we would have to declare the model names here and keep them hardcoded, as with the model properties

describe('test', () => {
  it('Customer model', () => {
    const customer: Customer = {
      id: 1,
      name: 'Ed',
      createdAt: new Date(),
      updatedAt: new Date()

    }
  })

  it('User model', () => {
    const user: User = {
      id: 1,
      name: 'Ed',
      age: 100,
      createdAt: new Date(),
      updatedAt: new Date()
    }
  })
})

Where the test script is "npx prisma generate && npx jest"

dearshrewdwit commented 1 year ago

Not sure how feasible this would be to test constraints/validations - but could be explored maybe

Model relationships should be testable with this approach

dearshrewdwit commented 1 year ago

Great investigations @julesnuggy @vherus - i think we're on the right lines here

julesnuggy commented 1 year ago

So, I wanted to try and avoid adding TypeScript to the repo just for the sake of testing, and I've found a way!

The prisma client instance contains an object which holds all of the data on the models: prisma._dmff.modelMap.<MyModelName>.

So, given a model like this:

model User {
  id          Int       @id @default(autoincrement())
  email       String
  password    String    @db.VarChar(255)
  isActive    Boolean   @default(true)
  customerId  Int
  customer    Customer  @relation(fields: [customerId], references: [id])
}

If we inspect prisma._dmmf.modelMap.User.fields, then we get back:

[
  {
    name: 'id',
    kind: 'scalar',
    isList: false,
    isRequired: true,
    isUnique: false,
    isId: true,
    isReadOnly: false,
    type: 'Int',
    hasDefaultValue: true,
    default: { name: 'autoincrement', args: [] },
    isGenerated: false,
    isUpdatedAt: false
  },
  {
    name: 'email',
    kind: 'scalar',
    isList: false,
    isRequired: true,
    isUnique: false,
    isId: false,
    isReadOnly: false,
    type: 'String',
    hasDefaultValue: false,
    isGenerated: false,
    isUpdatedAt: false
  },
  {
    name: 'password',
    kind: 'scalar',
    isList: false,
    isRequired: true,
    isUnique: false,
    isId: false,
    isReadOnly: false,
    type: 'String',
    hasDefaultValue: false,
    isGenerated: false,
    isUpdatedAt: false
  },
  {
    name: 'isActive',
    kind: 'scalar',
    isList: false,
    isRequired: true,
    isUnique: false,
    isId: false,
    isReadOnly: false,
    type: 'Boolean',
    hasDefaultValue: true,
    default: true,
    isGenerated: false,
    isUpdatedAt: false
  },
  {
    name: 'customerId',
    kind: 'scalar',
    isList: false,
    isRequired: true,
    isUnique: true,
    isId: false,
    isReadOnly: true,
    type: 'Int',
    hasDefaultValue: false,
    isGenerated: false,
    isUpdatedAt: false
  },
  {
    name: 'customer',
    kind: 'object',
    isList: false,
    isRequired: true,
    isUnique: false,
    isId: false,
    isReadOnly: false,
    type: 'Customer',
    hasDefaultValue: false,
    relationName: 'CustomerToUser',
    relationFromFields: [ 'customerId' ],
    relationToFields: [ 'id' ],
    isGenerated: false,
    isUpdatedAt: false
  }
]

It's a shame that it doesn't show us the specific native type for password (VARCHAR(255)) but I think the isRequired, default, relationTo/FromFields constraints are sufficient for writing some tests.

dearshrewdwit commented 1 year ago

That's great stuff - the assertions will still be quite straightforward with the added benefit of being able to assert on constraints and relations too.

Can you create a PR to main with an example test suite?

julesnuggy commented 1 year ago

@dearshrewdwit - I'm already most of the way through creating the entire test suite. I'll raise a draft PR for now so you can see what my approach is.

julesnuggy commented 1 year ago

PR raised here: https://github.com/boolean-uk/database-orm/pull/11

julesnuggy commented 1 year ago

@dearshrewdwit @vherus -- PR is now finalised for review.

If you want a schema which passes all tests, here is one:

generator client {
  provider = "prisma-client-js"
}

datasource db {
  provider          = "postgresql"
  url               = env("DATABASE_URL")
  shadowDatabaseUrl = env("SHADOW_DATABASE_URL")
}

model Customer {
  id        Int      @id @default(autoincrement())
  name      String
  createdAt DateTime @default(now())
  updatedAt DateTime @updatedAt
  contact   Contact?
  tickets   Ticket[]
}

model Contact {
  id         Int      @id @default(autoincrement())
  customer   Customer @relation(fields: [customerId], references: [id])
  customerId Int      @unique
  phone      String
  email      String
  createdAt  DateTime @default(now())
  updatedAt  DateTime @updatedAt
}

model Movie {
  id          Int         @id @default(autoincrement())
  createdAt   DateTime    @default(now())
  updatedAt   DateTime    @updatedAt
  runtimeMins Int
  title       String
  screenings  Screening[]
}

model Screen {
  id         Int         @id @default(autoincrement())
  number     Int
  createdAt  DateTime    @default(now())
  updatedAt  DateTime    @updatedAt
  screenings Screening[]
}

model Screening {
  id        Int      @id @default(autoincrement())
  createdAt DateTime @default(now())
  updatedAt DateTime @updatedAt
  startsAt  DateTime
  movie     Movie    @relation(fields: [movieId], references: [id])
  movieId   Int
  screen    Screen   @relation(fields: [screenId], references: [id])
  screenId  Int
  tickets   Ticket[]
}

model Ticket {
  id          Int       @id @default(autoincrement())
  createdAt   DateTime  @default(now())
  updatedAt   DateTime  @updatedAt
  customer    Customer  @relation(fields: [customerId], references: [id])
  customerId  Int
  screening   Screening @relation(fields: [screeningId], references: [id])
  screeningId Int
}
julesnuggy commented 1 year ago

The update to Prisdma 4.9.0 means this needs some adjusting 😑

julesnuggy commented 1 year ago

OK, now it's ready!

julesnuggy commented 1 year ago

@vherus @dearshrewdwit -- is this good to merge? 🙃 https://github.com/boolean-uk/database-orm/pull/11

dearshrewdwit commented 1 year ago

Yes, let's get it done and ready for cohort 9