Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cli) Generated services use Date objects for DateTime types in the tests #6394

Merged
merged 30 commits into from
Sep 30, 2022

Conversation

esteban-url
Copy link
Contributor

@esteban-url esteban-url commented Sep 15, 2022

This PR fixes the issue #3493 and replaces #6330

Problem

The main problem being fixed is that the generated service tests with a DateTime type fail one the first test run because they are being treated as strings and compared to date objects. 🚨

Read the detailed explanation in the original issue #3493 or a slightly briefer explanation down below

Solution

I modified as little as possible in the following files:

packages/cli/src/commands/generate/service/service.js

  1. The buildStringifiedScenario function now has more readable code.
  2. The scenarioFieldValue and fieldsToUpdate functions use Date objects instead of string values.

packages/cli/src/commands/generate/service/templates/test.ts.template

  1. The test template now looks for Dates and replaces them with actual new Date(...) declarations.

packages/cli/src/commands/generate/service/__tests__/scenario.test.js

  1. The generator tests evaluates that the scenarios return a valid Date instance.

✨ All the tests now pass right out of the box ✨

The generated test files now look like this:

notifications.test.js

  scenario('creates a notification', async () => {
    const result = await createNotification({
      input: {
        date: '2022-09-15T03:22:12.997Z',
        title: 'String',
        body: 'String',
      },
    })

    expect(result.date).toEqual(new Date('2022-09-15T03:22:12.997Z'))
    expect(result.title).toEqual('String')
    expect(result.body).toEqual('String')
  })

  scenario('updates a notification', async (scenario) => {
    const original = await notification({
      id: scenario.notification.one.id,
    })

    const result = await updateNotification({
      id: original.id,
      input: { date: '2022-09-16T03:22:12.997Z' },
    })

    expect(result.date).toEqual(new Date('2022-09-16T03:22:12.997Z'))
  })

Detailed problem explanation

Read the detailed explanation

when you add a model such as this:

model Notification {
  id        Int @id @default(autoincrement())
  date      DateTime
  title     String
  body      String
  createdAt DateTime @default(now())
}

Generate an sdl

yarn rw g sdl Notification

Run the tests

yarn rw test 

😫 the tests fail 😫

They fail because the generated test compare string values to date objects.

The generated files look like this (I added the comments):

notifications.test.js

  scenario('creates a notification', async () => {
    const result = await createNotification({
      input: {
        date: '2022-09-15T03:22:12Z',
        title: 'String',
        body: 'String',
      },
    })

    // THIS LINE CAUSES THE TEST TO FAIL!!!
    expect(result.date).toEqual('2022-09-15T03:22:12Z')
    expect(result.title).toEqual('String')
    expect(result.body).toEqual('String')
  })

  scenario('updates a notification', async (scenario) => {
    const original = await notification({
      id: scenario.notification.one.id,
    })

    const result = await updateNotification({
      id: original.id,
      input: { date: '2022-09-16T03:22:12Z' },
    })

    // THIS LINE CAUSES THE TEST TO FAIL!!!
    expect(result.date).toEqual('2022-09-16T03:22:12Z')
  })

Edits

  • 2022-09-23: general edit to reflect the new solution.
  • 2022-09-24: updated the generated files. it only uses new Date(...) declarations in the assertions.

@esteban-url
Copy link
Contributor Author

"Everything" is passing now! I think this PR will greatly improve the dev experience, as it will make the generated tests pass right off the bat when adding DateTime types to their models. as opposed to now that they are causing failed test runs/

@Tobbe do the comments in packages/cli/src/commands/generate/service/service.js here and in packages/cli/src/commands/generate/service/templates/test.ts.template here explain the regex and string replacement well, or how could i make it better/clearer?

Is there anything else I can do to move this forward?

Thanks for looking into it ☺️

@Tobbe
Copy link
Member

Tobbe commented Sep 23, 2022

I've played around with this a bit now, and I think we should only use new Date(...) in one place. Like this

image

image

To do that I only updated the template

<%
const stringifySingleValue = (value) => {
  return JSON.stringify(value).replace(/['"].*?['"]/g, (string) => {
    // Remove quotes around `scenario` variables
    if (string.match(/^scenario\./)) {
      return string.replace(/['"]/g, '')
    }

    // BigInt
    if (string.match(/^\"\d+n\"$/)) {
      return string.substr(1, string.length - 2)
    }

    // Creates a new Date declaration if the string matches this ISO8601 date format.
    // ex: "2022-09-01T00:21:58Z" will return `new Date("2022-09-01T00:21:58Z")`
    if (/^\"\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z\"$/.test(string)) {
      return `new Date(${string})`
    }

    return string
  })
} %>
import type { ${prismaModel} } from '@prisma/client'

import { ${pluralCamelName}<% if (crud) { %>,${singularCamelName}, create${singularPascalName}, update${singularPascalName}, delete${singularPascalName}<% } %> } from './${pluralCamelName}'
import type { StandardScenario } from './${pluralCamelName}.scenarios'

// Generated boilerplate tests do not account for all circumstances
// and can fail without adjustments, e.g. Float and DateTime types.
//           Please refer to the RedwoodJS Testing Docs:
//       https://redwoodjs.com/docs/testing#testing-services
// https://redwoodjs.com/docs/testing#jest-expect-type-considerations

describe('${pluralCamelName}', () => {
  scenario('returns all ${pluralCamelName}', async (scenario: StandardScenario) => {
    const result = await ${pluralCamelName}()

    expect(result.length).toEqual(Object.keys(scenario.${singularCamelName}).length)
  })<% if (crud) { %>

  scenario('returns a single ${singularCamelName}', async (scenario: StandardScenario) => {
    const result = await ${singularCamelName}({ id: scenario.${singularCamelName}.one.id })

    expect(result).toEqual(scenario.${singularCamelName}.one)
  })

  <% if (create) { %>scenario('creates a ${singularCamelName}', async (${JSON.stringify(create).includes('scenario.') ? 'scenario: StandardScenario' : ''}) => {
    const result = await create${singularPascalName}({
      input: ${JSON.stringify(create)},
    })

    <% for (const [name, value] of Object.entries(create)) { %>
    expect(result.${name}).toEqual(${stringifySingleValue(value)})<% } %>
  })<% } %>

  <% if (update) { %>scenario('updates a ${singularCamelName}', async (scenario: StandardScenario) => {<% rand = parseInt(Math.random() * 10000000) %>
    const original = await (${singularCamelName}({ id: scenario.${singularCamelName}.one.id })) as ${prismaModel}
    const result = await update${singularPascalName}({
      id: original.id,
      input: ${JSON.stringify(update)},
    })

    <% for (const [name, value] of Object.entries(update)) { %>
    expect(result.${name}).toEqual(${stringifySingleValue(value)})<% } %>
  })<% } %>

  scenario('deletes a ${singularCamelName}', async (scenario: StandardScenario) => {
    const original = (await delete${singularPascalName}({ id: scenario.${singularCamelName}.one.id })) as ${prismaModel}
    const result = await ${singularCamelName}({ id: original.id })

    expect(result).toEqual(null)
  })<% } %>
})

@esteban-url esteban-url changed the title fix(cli) Generated services use Date objects for DateTime types in the tests and scenarios fix(cli) Generated services use Date objects for DateTime types in the tests Sep 24, 2022
@esteban-url
Copy link
Contributor Author

esteban-url commented Sep 24, 2022

Yes, thats so much cleaner @Tobbe! Thank you so much, This helped me understand an aspect of how the scenarios work that I hadn't grasped yet.

Updated the PR to only have new Date(...) declarations in the tests files, as @Tobbe pointed out.

I also kept:

  • The changes to buildStringifiedScenario so that it's more readable
  • The use of Date objects so that they can be evaluated directly without the need for workarounds such as date.toISOString().replace(/\.\d{3}/, '')

Added a validity test to the dates

and updated the solution in the first PR comment, to reflect the new solution

@Tobbe
Copy link
Member

Tobbe commented Sep 24, 2022

When I try your updated code I still get a date object both in the create-call and in the expected test values

image

I'd like only the expected value to be a date object. In the create call I want it to just be a string.

Like this

image

@esteban-url
Copy link
Contributor Author

I'm sorry @Tobbe, my bad, I've fixed this behavior.

Instead of using JSON.stringify directly, I added an extra asDate parameter to the stringifyValue function (which I also renamed for clarity). If the parameter is true it will return a new Date(...) declaration, otherwise a string representation of the value.

Using JSON.stringify keeps BigInt types as a string, so prisma errors out:

 Invalid `db.notification.create()` invocation in
    /Users/.../notifications.js:14:26

      11 }
      12 
      13 export const createNotification = ({ input }) => {
    → 14   return db.notification.create({
             data: {
               date: '2022-09-24T18:29:34.928Z',
               title: 'String',
               body: 'String',
               bool: true,
               intNumber: 1289246,
               bigIntNumber: '448927n'
                             ~~~~~~~~~
             }
           })

    Argument bigIntNumber: Got invalid value '448927n' on prisma.createOneNotification. Provided String, expected BigInt.
  scenario('creates a notification', async () => {
    const result = await createNotification({
      input: {
        date: '2022-09-24T18:29:34.928Z',
        title: 'String',
        body: 'String',
        bool: true,
        intNumber: 1289246,
// This needs to be converted into a BigInt. The stringifyValue function  takes care of it
        bigIntNumber: '448927n', 
      },
    })

    expect(result.date).toEqual(new Date('2022-09-24T18:29:34.928Z'))
    expect(result.title).toEqual('String')
    expect(result.body).toEqual('String')
    expect(result.bool).toEqual(true)
    expect(result.intNumber).toEqual(1289246)
    expect(result.bigIntNumber).toEqual(448927n)
  })

this is the schema:

model Notification {
  id            Int      @id @default(autoincrement())
  date          DateTime
  title         String
  body          String
  bool          Boolean
  intNumber     Int
  bigIntNumber  BigInt
  createdAt     DateTime @default(now())
}

@Tobbe
Copy link
Member

Tobbe commented Sep 24, 2022

Using JSON.stringify keeps BigInt types as a string, so prisma errors out:

That's a great catch! Can you please make sure there are tests for this, so no one messes that up in the future 🙂

@Tobbe Tobbe added the release:fix This PR is a fix label Sep 24, 2022
@Tobbe Tobbe assigned Tobbe and unassigned simoncrypta Sep 24, 2022
@esteban-url
Copy link
Contributor Author

Using JSON.stringify keeps BigInt types as a string, so prisma errors out:

That's a great catch! Can you please make sure there are tests for this, so no one messes that up in the future 🙂

I tried multiple approaches but couldn't get it quite right, because what i need to test is the actual rendered test file. I guess the ideal would be to use a snapshot. I'll give it a go tomorrow. again, thanks for all the guidance @Tobbe

@Tobbe
Copy link
Member

Tobbe commented Sep 30, 2022

Thanks for your patience on this one Esteban. I'll try to get to it later today.

@Tobbe Tobbe enabled auto-merge (squash) September 30, 2022 11:09
@Tobbe Tobbe merged commit 6e2cd52 into redwoodjs:main Sep 30, 2022
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Sep 30, 2022
@esteban-url
Copy link
Contributor Author

thanks for the help and guidance @Tobbe

@jtoar jtoar modified the milestones: next-release, v3.2.0 Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants