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

[Bug?]: Scenarios broken with redwood 7 #10068

Closed
1 task
razzeee opened this issue Feb 27, 2024 · 15 comments · Fixed by #10112
Closed
1 task

[Bug?]: Scenarios broken with redwood 7 #10068

razzeee opened this issue Feb 27, 2024 · 15 comments · Fixed by #10112
Assignees
Labels
bug/confirmed We have confirmed this is a bug topic/testing

Comments

@razzeee
Copy link
Contributor

razzeee commented Feb 27, 2024

What's not working?

We have some tests that fail after the update to redwood 7.

It seems, like "sub objects" in scenarios only get created some times and not every time as expected.

How do we reproduce the bug?

What's your environment? (If it applies)

System:
    OS: Linux 6.7 Fedora Linux 39 (Workstation Edition)
    Shell: 5.9 - /usr/bin/zsh
  Binaries:
    Node: 20.10.0 - /tmp/xfs-e5f62a34/node
    Yarn: 4.1.0 - /tmp/xfs-e5f62a34/yarn
  Databases:
    SQLite: 3.42.0 - /usr/bin/sqlite3
  npmPackages:
    @redwoodjs/core: 7.0.3 => 7.0.3 
    @redwoodjs/project-config: 7.0.3 => 7.0.3

Are you interested in working on this?

  • I'm interested in working on this
@razzeee razzeee added the bug/needs-info More information is needed for reproduction label Feb 27, 2024
@ahaywood
Copy link
Contributor

@razzeee Thanks for taking the time to submit the bug. Our team will take a look and get back with you ASAP.

@cjreimer
Copy link
Contributor

I am running into some similar issues with api side testing as well. When I get the error, it looks like:

 PrismaClientKnownRequestError:
    Invalid `getProjectDb()[model].create()` invocation in
    D:\CENGYS\Dev\cengys-r\node_modules\@redwoodjs\testing\config\jest\api\jest.setup.js:193:64

      190 scenarios[model] = {}
      191 for (const [name, createArgs] of Object.entries(namedFixtures)) {
      192   if (typeof createArgs === 'function') {
    → 193     scenarios[model][name] = await getProjectDb()[model].create(
    Unique constraint failed on the fields: (`code`)

      at ai.handleRequestError (node_modules/@prisma/client/runtime/library.js:126:6775)
      at ai.handleAndLogRequestError (node_modules/@prisma/client/runtime/library.js:126:6109)
      at ai.request (node_modules/@prisma/client/runtime/library.js:126:5817)
      at l (node_modules/@prisma/client/runtime/library.js:131:9709)
      at seedScenario (node_modules/@redwoodjs/testing/config/jest/api/jest.setup.js:193:36)
      at Object.<anonymous> (node_modules/@redwoodjs/testing/config/jest/api/jest.setup.js:106:28)

Note that code is a field in the model that is typically created in each scenario, both for test files that run and test files that fail.

The situations that seem to generate the incorrect teardown are related to a test failing. After a test fails, all subsequent scenario tests fail with the above error. I'm thinking, but have not proven, that a failing test doesn't teardown correctly.

@razzeee Does your experience match mine, that failing tests initiate this, or is your experience different?

I guess the fix for me is to not allow any test to fail 😄 !

@cjreimer
Copy link
Contributor

I'll add that I could be somewhat to blame for this bug as I tested #9866 before it was merged. If this is due to that PR, then obviously I missed something!

@cdubz
Copy link

cdubz commented Feb 28, 2024

We have also been experiencing this and it does seem to stem from #9866 -- using the new describeScenario function seems to resolve the issue. And it also only affects some tests (i.e. not all) but so far I haven't been able to tell exactly what it is that triggers it.

@cdubz
Copy link

cdubz commented Feb 28, 2024

Here's an example of a change I made on a recent commit for what was mostly the basic generated tests on the model --

diff --git a/api/src/services/pbsShows/pbsShows.test.ts b/api/src/services/pbsShows/pbsShows.test.ts
index 494be58d..5e6bcbdf 100644
--- a/api/src/services/pbsShows/pbsShows.test.ts
+++ b/api/src/services/pbsShows/pbsShows.test.ts
@@ -15,20 +15,26 @@ import type { StandardScenario } from './pbsShows.scenarios'
 //       https://redwoodjs.com/docs/testing#testing-services
 // https://redwoodjs.com/docs/testing#jest-expect-type-considerations
 
-describe('pbsShows', () => {
-  scenario('returns all pbsShows', async (scenario: StandardScenario) => {
+describeScenario<StandardScenario>('pbsShows', (getScenario) => {
+  let scenario: StandardScenario
+
+  beforeEach(() => {
+    scenario = getScenario()
+  })
+
+  it('returns all pbsShows', async () => {
     const result = await pbsShows()
 
     expect(result.length).toEqual(Object.keys(scenario.pbsShow).length)
   })
 
-  scenario('returns a single pbsShow', async (scenario: StandardScenario) => {
+  it('returns a single pbsShow', async () => {
     const result = await pbsShow({ id: scenario.pbsShow.one.id })
 
     expect(result).toEqual(scenario.pbsShow.one)
   })
 
-  scenario('creates a pbsShow', async () => {
+  it('creates a pbsShow', async () => {
     const result = await createPbsShow({
       input: {
         pbsId: 'e9b2bb97-4d38-46f5-a229-6b79b04fd45f',
@@ -53,7 +59,7 @@ describe('pbsShows', () => {
     expect(result.fetchedAt).toEqual(new Date('2023-11-15T23:14:49.140Z'))
   })
 
-  scenario('updates a pbsShow', async (scenario: StandardScenario) => {
+  it('updates a pbsShow', async () => {
     const original = (await pbsShow({ id: scenario.pbsShow.one.id })) as PbsShow
     const result = await updatePbsShow({
       id: original.id,
@@ -67,7 +73,7 @@ describe('pbsShows', () => {
     expect(result.pbsId).toEqual('30b3f373-027b-4acb-acd6-9c81b951d54c')
   })
 
-  scenario('deletes a pbsShow', async (scenario: StandardScenario) => {
+  it('deletes a pbsShow', async () => {
     const original = (await deletePbsShow({
       id: scenario.pbsShow.one.id,
     })) as PbsShow

It seems like something about the scenario configuration triggers the issue? Here's what the scenarios look like for that test (in case this is reproducible) --

import type { Prisma, PbsShow } from '@prisma/client'

import type { ScenarioData } from '@redwoodjs/testing/api'

export const standard = defineScenario<Prisma.PbsShowCreateArgs>({
  pbsShow: {
    one: {
      data: {
        pbsId: '24b26973-60d8-4ad7-85f3-6343ddc4b2d8',
        slug: 'String1365663',
        title: 'String',
        descriptionShort: 'String',
        descriptionLong: 'String',
        pbsUpdatedAt: '2023-11-15T23:14:49.157Z',
        fetchedAt: '2023-11-15T23:14:49.157Z',
      },
    },
    two: {
      data: {
        pbsId: 'bc34ab3b-f9ff-4ad6-960e-d2abb9690498',
        slug: 'String5861917',
        title: 'String',
        descriptionShort: 'String',
        descriptionLong: 'String',
        pbsUpdatedAt: '2023-11-15T23:14:49.157Z',
        fetchedAt: '2023-11-15T23:14:49.157Z',
      },
    },
  },
})

export type StandardScenario = ScenarioData<PbsShow, 'pbsShow'>

@razzeee
Copy link
Contributor Author

razzeee commented Feb 28, 2024

@razzeee Does your experience match mine, that failing tests initiate this, or is your experience different?

No, our tests were running just fine on v6 (or still are, as we haven't updated due to this yet)
But they fail with v7

@peraltafederico
Copy link

peraltafederico commented Feb 29, 2024

I also noticed that if any test fails, the data in the database is not deleted and recreated, but only an attempt is made to create it again. This leads to a failure if your entity has a unique field like a name column. See my case

defineScenario({
  organization: {
    one: {
      data: {
        name: 'abc',
      },
    }
  }
})

When a single test fails, subsequent tests will also fail with this error:


    PrismaClientKnownRequestError: 
    Invalid `getProjectDb()[model].create()` invocation in
    /home/federico/Documents/projects/platform/node_modules/@redwoodjs/testing/config/jest/api/jest.setup.js:197:64

      194     createArgs(scenarios)
      195   )
      196 } else {
    → 197   scenarios[model][name] = await getProjectDb()[model].create(
    Unique constraint failed on the fields: (`name`)

      at ai.handleRequestError (node_modules/@prisma/client/runtime/library.js:126:6775)
      at ai.handleAndLogRequestError (node_modules/@prisma/client/runtime/library.js:126:6109)
      at ai.request (node_modules/@prisma/client/runtime/library.js:126:5817)
      at l (node_modules/@prisma/client/runtime/library.js:131:9709)
      at seedScenario (node_modules/@redwoodjs/testing/config/jest/api/jest.setup.js:197:36)
      at Object.<anonymous> (node_modules/@redwoodjs/testing/config/jest/api/jest.setup.js:106:28)

This, as I said, is because name is a unique SQL column.

Is anyone experiencing the same thing?

@dac09
Copy link
Contributor

dac09 commented Mar 2, 2024

Thanks all for the info, very likely related to the describeScenario change. Will dig into this shortly! :)

@dac09 dac09 self-assigned this Mar 2, 2024
@dac09 dac09 added bug/confirmed We have confirmed this is a bug and removed bug/needs-info More information is needed for reproduction labels Mar 5, 2024
@dac09
Copy link
Contributor

dac09 commented Mar 5, 2024

Hey all, looking into this issue, but @razzeee your repro doesn't seem to pass tests on 6.6.4 either unfortunately!

    Received promise rejected instead of resolved
    Rejected to value: [PrismaClientKnownRequestError:·
    Invalid `db.seat.delete()` invocation in
    /Users/dac09/Experiments/my-redwood-scenario-repro/api/src/services/seats/seats.ts:50:24·
      47   throw new Error("Seat can't be deleted as it has history")
      48 }

Any chance you could update the reproduction please? I'm looking to isolate whtat the problem is here first.


@cdubz, @cjreimer - wouldn't mind testing against your models either - if you could share your model and tests. I'll try and re-create the simple one here #10068 (comment) (update: cannot reproduce... help!)

I also noticed that if any test fails, the data in the database is not deleted and recreated, but only an attempt is made to create it again. This leads to a failure if your entity has a unique field like a name column.

@peraltafederico thanks for this, I'll keep this in mind too. This one I think I already have a solution for :)

@razzeee
Copy link
Contributor Author

razzeee commented Mar 5, 2024

@dac09 Will need to downgrade that solution then, it's still working fine with that code in our production repo.

You can make the test faliing test seats › deleteSeat - still active - do not delete pass, when you comment out the other scenarios in that file, which seems pretty telling

@dac09
Copy link
Contributor

dac09 commented Mar 5, 2024

Yep yep, did downgrade to 6.6.4!

The failing test use case is in PR - just wondering if there's something else too.

@razzeee
Copy link
Contributor Author

razzeee commented Mar 5, 2024

I do see that too, when downgrading the repro repo. I even tried copying yarn.lock and all package.json then installing with a frozen lockfile. Still no dice, still failing. So unsure, what's different.

I had another issue with v6, where it broke all of a sudden seemingly due to changes to v7, so I'm a bit cautious, if there is some dependency locking problem going on.

@cjreimer
Copy link
Contributor

cjreimer commented Mar 5, 2024

@dac09 Unfortunately, I don't have a ton of time to work on this now as I'm dealing with a family health crisis, but I noticed the following.

The issue appears to start when there are uncaught exceptions in the test code. This first block of code causes the following tests to fail on a scenario setup.


    scenario(
      'attempts to query a single issue not in the DB',
      async (scenario: StandardScenario) => {
        mockCurrentUser(castCurrentUser(scenario.user.userAdminOrgA))
          // The following code is throwing an uncaught exception right now
          const result = await issue({ id: -1 })
          expect(result).toEqual(null)
      }
    )

This second block of code does not cause the following tests to fail.

    scenario(
      'attempts to query a single issue not in the DB',
      async (scenario: StandardScenario) => {
        mockCurrentUser(castCurrentUser(scenario.user.userAdminOrgA))
        try {
          // The following code is throwing an uncaught exception right now
          const result = await issue({ id: -1 })
          expect(result).toEqual(null)
        } catch (e) {
        }
      }
    )

I haven't had the chance to test it yet, but do we need a try... catch in the following function of jest.setup.js? The await teardown() used to be in an afterEach() function, but was moved into this test wrapper. We need to make sure that the teardown always happens.

const buildScenario =
  (itFunc, testPath) =>
  (...args) => {
    let scenarioName, testName, testFunc

    if (args.length === 3) {
      ;[scenarioName, testName, testFunc] = args
    } else if (args.length === 2) {
      scenarioName = DEFAULT_SCENARIO
      ;[testName, testFunc] = args
    } else {
      throw new Error('scenario() requires 2 or 3 arguments')
    }

    return itFunc(testName, async () => {
      let { scenario } = loadScenarios(testPath, scenarioName)

      const scenarioData = await seedScenario(scenario)
      const result = await testFunc(scenarioData)

      if (wasDbUsed()) {
        await teardown()
      }

      return result
    })
  }

HTH!

@dac09
Copy link
Contributor

dac09 commented Mar 6, 2024

Thank you all for the input, and the reproduction!

PR merged, will be released in a patch soon - hope this resolves the issue for you guys. If not feel free to re-open the issue.

What does it solve? Best described above by Curtis

The situations that seem to generate the incorrect teardown are related to a test failing. After a test fails, all subsequent scenario tests fail with the above error. ... a failing test doesn't teardown correctly

Note @razzeee looking more carefully at your code, I think maybe in the reproduction you have some logic with seat history that makes your tests fail. I believe this is not really related - just application logic, or copy pasted wrong :) - regardless hopefully the above fix will let you upgrade your real app.

@razzeee
Copy link
Contributor Author

razzeee commented Mar 11, 2024

I'm still seeing this problem, this did not solve anything for us.

Note @razzeee looking more carefully at your code, I think maybe in the reproduction you have some logic with seat history that makes your tests fail. I believe this is not really related - just application logic, or copy pasted wrong :) - regardless hopefully the above fix will let you upgrade your real app.

Could be copied wrong. Fact is, it worked with v6 and it stopped working in our (unmerged) PR of v7. (I even think it worked with the RC first, but probably not helpful)

And I also don't think it's related to a timeout, gut feeling is, that it's due to the exceptions being thrown and us testing that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/confirmed We have confirmed this is a bug topic/testing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants