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

Add aroundEach Hook Similar to RSpec around #5728

Open
4 tasks done
klondikemarlen opened this issue May 15, 2024 · 10 comments
Open
4 tasks done

Add aroundEach Hook Similar to RSpec around #5728

klondikemarlen opened this issue May 15, 2024 · 10 comments

Comments

@klondikemarlen
Copy link

klondikemarlen commented May 15, 2024

Clear and concise description of the problem

I want an aroundEach so that I can run my database tests inside a transaction and roll back the transaction after the test completes. This would vastly speed up my test runs.

Currently I need to wipe each table in my database, on each test run, to be sure that test state is clean.
Naturally, due to having a great many migrations, this is more performant than dropping the whole database and then rebuilding the schema.

code I want to replace with aroundEach wrapped in a transaction
import dbLegacy from "@/db/db-client-legacy"

async function getTableNames() {
  const query = /* sql */ `
    SELECT
      table_name AS "tableName"
    FROM
      information_schema.tables
    WHERE
      table_schema = 'dbo'
      AND table_type = 'BASE TABLE'
      AND table_name NOT IN ('knex_migrations' , 'knex_migrations_lock');
  `

  try {
    const result = await dbLegacy.raw<{ tableName: string }[]>(query)
    const tableNames = result.map((row) => row.tableName)
    return tableNames
  } catch (error) {
    console.error("Error fetching table names:", error)
    throw error
  }
}

async function getTableNamesWithoutIdentityColumn() {
  const query = /* sql */ `
    SELECT
      tables.name AS "tableName"
    FROM
      sys.tables AS tables
    WHERE
      tables.schema_id = SCHEMA_ID('dbo')
      AND NOT EXISTS (
        SELECT 1
        FROM sys.columns AS columns
        WHERE columns.object_id = tables.object_id AND columns.is_identity = 1
      );
  `

  try {
    const result = await dbLegacy.raw<{ tableName: string }[]>(query)
    const tableNames = result.map((row) => row.tableName)
    return tableNames
  } catch (error) {
    console.error("Error fetching table names without identity columns:", error)
    throw error
  }
}

/**
 * Example of generated SQL commands used for cleaning database:
 *
 * ```sql
 * ALTER TABLE table1 NOCHECK CONSTRAINT ALL;
 * ALTER TABLE table2 NOCHECK CONSTRAINT ALL;
 * ...
 * DELETE FROM table1 WHERE 1=1;
 * DELETE FROM table2 WHERE 1=1;
 * ...
 * DBCC CHECKIDENT ('table1', RESEED, 0);
 * DBCC CHECKIDENT ('table2', RESEED, 0);
 * ...
 * ALTER TABLE table1 CHECK CONSTRAINT ALL;
 * ALTER TABLE table2 CHECK CONSTRAINT ALL;
 * ...
 * ```
 */
async function buildCleanupQuery() {
  const tableNames = await getTableNames()
  const tableNamesWithoutIdentityColumn = await getTableNamesWithoutIdentityColumn()
  const tableNamesWithIdentityColumn = tableNames.filter(
    (tableName) => !tableNamesWithoutIdentityColumn.includes(tableName)
  )
  const disableAllConstraintsQuery = tableNames
    .map((tableName) => /* sql */ `ALTER TABLE ${tableName} NOCHECK CONSTRAINT ALL;`)
    .join("\n")
  const deleteAllInAllTablesQuery = tableNames
    .map((tableName) => /* sql */ `DELETE FROM ${tableName} WHERE 1=1;`)
    .join("\n")
  const resetIdentityColumnsQuery = tableNamesWithIdentityColumn
    .map((tableName) => /* sql */ `DBCC CHECKIDENT ('${tableName}', RESEED, 0);`)
    .join("\n")
  const enableAllConstraintsQuery = tableNames
    .map((tableName) => /* sql */ `ALTER TABLE ${tableName} CHECK CONSTRAINT ALL;`)
    .join("\n")
  const cleanupQuery = [
    disableAllConstraintsQuery,
    deleteAllInAllTablesQuery,
    resetIdentityColumnsQuery,
    enableAllConstraintsQuery,
  ].join("\n")
  return cleanupQuery
}

async function cleanDatabase() {
  const cleanupQuery = await buildCleanupQuery()
  try {
    await dbLegacy.raw(cleanupQuery).catch(console.error)
    return true
  } catch (error) {
    console.error(error)
    return false
  }
}

beforeEach(async () => {
  await cleanDatabase()
})

It's also possible that this is already possible, but after spending an entire day trying to get it to work, I'm at least reasonably sure it isn't.

Suggested solution

Add support for a simple aroundEach hook.

// sequelize being a sequelize 7 (or 6) instance that is using managed transactions

aroundEach(async (example) => {
   await sequelize.transaction(async (transaction) => {
     await example.run()
     await transaction.rollback()
   })
})

This would vastly speed up my test runs from

 tests/services/position-teams/create-service.test.ts (3) 1968ms
   ✓ api/src/services/position-teams/create-service.ts (3) 1967ms
     ✓ CreateService (3) 1967ms
       ✓ #perform (3) 1967ms
         ✓ when passed valid attributes, it creates a position team 770ms
         ✓ when teamId is missing, errors informatively 614ms
         ✓ when positionId is missing, errors informatively 582ms

To something much more reasonable.

Its true that I can do this in each test, but who wants to clutter up their test code with setup logic?

Alternative

Implement my own aroundEach that looks like

async function aroundEach(fn: (runExample: () => Promise<void>) => Promise<void>) {
  beforeEach(async () => {
    console.log("beforeEach start")
    let resolveBeforeEachWaiter: (value: void | PromiseLike<void>) => void
    const exampleRunWaiter = new Promise<void>((resolve) => {
      resolveBeforeEachWaiter = resolve
    })

    fn(() => exampleRunWaiter)

    return async () => {
      console.log("beforeEach cleanup start")
      resolveBeforeEachWaiter()
      await exampleRunWaiter
      console.log("beforeEach cleanup end")
    }
  })
}

aroundEach(async (runExample) => {
  console.log("aroundEach start")
  await db.transaction(async () => {
    console.log('before runExample')
    await runExample()
    // TODO: implement rollback
    console.log('after runExample')
  })
  console.log("aroundEach end")
})

Additional context

Will be used in all of the projects I'm working; pretty much anything in https://github.com/orgs/icefoganalytics/repositories?type=public (subset of https://github.com/orgs/ytgov/repositories?type=all).

Example of tests that would benefit from beforeEach transaction wrapping
import { UserTeam } from "@/models"
import { CreateService } from "@/services/user-positions"
import {
  organizationFactory,
  positionFactory,
  positionTeamFactory,
  teamFactory,
  userFactory,
} from "@/factories"

describe("api/src/services/user-positions/create-service.ts", () => {
  describe("CreateService", () => {
    describe("#perform", () => {
      test("when passed valid attributes, it creates a user position", async () => {
        // Arrange
        const user = await userFactory.create()
        const organization = await organizationFactory.create()
        const position = await positionFactory.create({
          organizationId: organization.id,
        })
        const attributes = {
          userId: user.id,
          positionId: position.id,
        }

        // Act
        const userPosition = await CreateService.perform(attributes)

        // Assert
        expect(userPosition).toEqual(
          expect.objectContaining({
            userId: user.id,
            positionId: position.id,
          })
        )
      })

      test("when userId is missing, errors informatively", async () => {
        // Arrange
        const organization = await organizationFactory.create()
        const position = await positionFactory.create({
          organizationId: organization.id,
        })
        const attributes = {
          positionId: position.id,
        }

        // Assert
        expect.assertions(1)
        await expect(
          // Act
          CreateService.perform(attributes)
        ).rejects.toThrow("User ID is required")
      })
    })

    test("when positionId is missing, errors informatively", async () => {
      // Arrange
      const user = await userFactory.create()
      const attributes = {
        userId: user.id,
      }

      // Assert
      expect.assertions(1)
      await expect(
        // Act
        CreateService.perform(attributes)
      ).rejects.toThrow("Position ID is required")
    })

    test("when position has teams, it creates user teams for all teams position has access to", async () => {
      // Arrange
      const user = await userFactory.create()
      const organization = await organizationFactory.create()
      const position = await positionFactory.create({
        organizationId: organization.id,
      })
      const team1 = await teamFactory.create({
        organizationId: organization.id,
      })
      const team2 = await teamFactory.create({
        organizationId: organization.id,
      })
      const team3 = await teamFactory.create({
        organizationId: organization.id,
      })
      const positionTeam1 = await positionTeamFactory.create({
        teamId: team1.id,
        positionId: position.id,
      })
      const positionTeam2 = await positionTeamFactory.create({
        teamId: team2.id,
        positionId: position.id,
      })
      const positionTeam3 = await positionTeamFactory.create({
        teamId: team3.id,
        positionId: position.id,
      })
      const attributes = {
        userId: user.id,
        positionId: position.id,
      }

      // Act
      const userPosition = await CreateService.perform(attributes)

      // Assert
      expect(userPosition).toEqual(
        expect.objectContaining({
          userId: user.id,
          positionId: position.id,
        })
      )
      const userTeams = await UserTeam.findAll({
        where: { userId: user.id },
        order: [["teamId", "ASC"]],
      })
      expect(userTeams).toHaveLength(3)
      expect(userTeams).toEqual([
        expect.objectContaining({
          userId: user.id,
          teamId: team1.id,
          positionId: position.id,
          positionRole: positionTeam1.role,
          sources: UserTeam.Sources.POSITION,
        }),
        expect.objectContaining({
          userId: user.id,
          teamId: team2.id,
          positionId: position.id,
          positionRole: positionTeam2.role,
          sources: UserTeam.Sources.POSITION,
        }),
        expect.objectContaining({
          userId: user.id,
          teamId: team3.id,
          positionId: position.id,
          positionRole: positionTeam3.role,
          sources: UserTeam.Sources.POSITION,
        }),
      ])
    })

    test("when position has teams and user is already on a team, it creates user teams for all teams position has access to and does not duplicate existing user teams", async () => {
      // Arrange
      const user = await userFactory.create()
      const organization = await organizationFactory.create()
      const position = await positionFactory.create({
        organizationId: organization.id,
      })
      const team1 = await teamFactory.create({
        organizationId: organization.id,
      })
      const team2 = await teamFactory.create({
        organizationId: organization.id,
      })
      const team3 = await teamFactory.create({
        organizationId: organization.id,
      })
      const positionTeam1 = await positionTeamFactory.create({
        teamId: team1.id,
        positionId: position.id,
        role: "Role 1",
      })
      const positionTeam2 = await positionTeamFactory.create({
        teamId: team2.id,
        positionId: position.id,
        role: "Role 2",
      })
      const positionTeam3 = await positionTeamFactory.create({
        teamId: team3.id,
        positionId: position.id,
        role: "Role 3",
      })
      await UserTeam.create({
        userId: user.id,
        teamId: team1.id,
        sources: UserTeam.Sources.DIRECT,
      })
      await UserTeam.create({
        userId: user.id,
        teamId: team2.id,
        sources: UserTeam.Sources.DIRECT,
      })
      const attributes = {
        userId: user.id,
        positionId: position.id,
      }

      // Act
      const userPosition = await CreateService.perform(attributes)

      // Assert
      expect(userPosition).toEqual(
        expect.objectContaining({
          userId: user.id,
          positionId: position.id,
        })
      )
      const userTeams = await UserTeam.findAll({
        where: { userId: user.id },
        order: [["teamId", "ASC"]],
      })
      expect(userTeams).toHaveLength(3)
      expect(userTeams).toEqual([
        expect.objectContaining({
          userId: user.id,
          teamId: team1.id,
          positionId: position.id,
          positionRole: positionTeam1.role,
          sources: UserTeam.Sources.DIRECT_AND_POSITION,
        }),
        expect.objectContaining({
          userId: user.id,
          teamId: team2.id,
          positionId: position.id,
          positionRole: positionTeam2.role,
          sources: UserTeam.Sources.DIRECT_AND_POSITION,
        }),
        expect.objectContaining({
          userId: user.id,
          teamId: team3.id,
          positionId: position.id,
          positionRole: positionTeam3.role,
          sources: UserTeam.Sources.POSITION,
        }),
      ])
    })
  })
})

Validations

@hi-ogawa
Copy link
Contributor

I think I get the idea, but can you also provide more concrete code and references? For example, what is "RSpec around"? also I would assume you're using sequelize.transaction with async hook mode https://sequelize.org/docs/v6/other-topics/transactions/#automatically-pass-transactions-to-all-queries but it's not entirely clear from the given code.

We are not necessary familiar with all those stuff, so providing as much context as possible would help triage the request better without misunderstanding.

@hi-ogawa
Copy link
Contributor

I think what people do so far to wrap a whole test function is to either create an own wrapper of it like effect https://github.com/Effect-TS/effect/blob/40c2b1d9234bcfc9ab4039282b621ca092f800cd/packages/vitest/src/index.ts#L55-L64 or I think it's also possible to use custom task function https://vitest.dev/advanced/runner.html#your-task-function

@klondikemarlen klondikemarlen changed the title Add aroundEach Hook Similar to RSpec around Add aroundEach Hook Similar to [RSpec](https://rspec.info/features/3-13/rspec-core/hooks/around-hooks/) around May 15, 2024
@klondikemarlen klondikemarlen changed the title Add aroundEach Hook Similar to [RSpec](https://rspec.info/features/3-13/rspec-core/hooks/around-hooks/) around Add aroundEach Hook Similar to RSpec around May 15, 2024
@klondikemarlen
Copy link
Author

I think I get the idea, but can you also provide more concrete code and references? For example, what is "RSpec around"? also I would assume you're using sequelize.transaction with async hook mode https://sequelize.org/docs/v6/other-topics/transactions/#automatically-pass-transactions-to-all-queries but it's not entirely clear from the given code.

We are not necessary familiar with all those stuff, so providing as much context as possible would help triage the request better without misunderstanding.

Thanks for the suggestions! I've updated the request to include some references. Also realized that the repo links I shared are still private (they'll be public soon, just hasn't happened yet). I'll shared some code examples directly.

@klondikemarlen
Copy link
Author

I think what people do so far to wrap a whole test function is to either create an own wrapper of it like effect https://github.com/Effect-TS/effect/blob/40c2b1d9234bcfc9ab4039282b621ca092f800cd/packages/vitest/src/index.ts#L55-L64 or I think it's also possible to use custom task function https://vitest.dev/advanced/runner.html#your-task-function

I'll see if I can make this work.
Maybe I can add a describe({ type: 'database' }, () => {} or something that using that effect pattern you shared.

@sheremet-va
Copy link
Member

Last year I gave this example: #3404 (comment)

Since then, the API has changed a little bit:

import { createTaskCollector, getCurrentSuite } from 'vitest/suite'

export const withTransaction = createTaskCollector(
  function (name, fn, timeout) {
    const handler = async (...args) => {
      await database.transaction(() => fn(...args))
    }
    getCurrentSuite().task(name, {
      ...this, // so "todo"/"skip"/... is tracked correctly
      handler,
      timeout,
    })
  }
)

@sheremet-va
Copy link
Member

Maybe you can also do something like this:

import { test as baseTest } from 'vitest'

export const withTransaction = baseTest.extend({
  transaction: ({ database }, use) => {
    await database.transaction(() => {
      await use()
    })
  }
})

I haven't tried it, but I think it should work 🤔

@klondikemarlen
Copy link
Author

klondikemarlen commented May 15, 2024

That looks like a huge step closer to what I was trying do to, though I don't think it quite solves the usability issue. The issue in question is: If the the next developers on the project forget to use the transaction helper, it will lead to hard to understand bugs. Ideally the everyday developer wouldn't have to worry about database cleanup as it should be handled at the config level (i.e https://vitest.dev/config/#setupfiles).

Given that, I'm going to see if I can implement an aroundEach, using the task collector? (or something), so database cleanup is handled at the highest level. For context I'm writing tests for the back-end of a web app, so 90% of tests will be database tests. I'd like to operate so that tests run the database cleaner by default, and if there are some non-database tests that we want maximum speed for, we can set a flag to disable them.

e.g.

describe(() => {
    // some database test
})

Would always run in a transaction, and auto-rollback after completion.

If we have a library or utility test that doesn't use the database we could turn the database cleaner off via

describe({ type: "library" }, () => {
   // some test that doesn't required the database
})

Or whatever Vitest supports.

@klondikemarlen
Copy link
Author

After all my efforts and this probably? working aroundEach

async function aroundEach(fn: (runExample: () => Promise<void>) => void) {
  beforeEach(async () => {
    let resolveRunExample: (value: void | PromiseLike<void>) => void
    const runExample = new Promise<void>((resolve) => {
      resolveRunExample = resolve
    })

    fn(() => runExample)

    return async () => {
      resolveRunExample()
      await runExample
    }
  })
}

The following code does not pass the transaction to the various Sequelize model actions performed in the "runExample()".

aroundEach(async (runExample) => {
  try {
    await sequelize.transaction(async () => {
      await runExample()
      return Promise.reject("TRIGGER DATABASE CLEANUP")
    })
  } catch (error) {
    if (error !== "TRIGGER DATABASE CLEANUP") {
      throw error
    }
  }
})

@klondikemarlen
Copy link
Author

klondikemarlen commented May 15, 2024

Unless anyone still thinks this feature is valuable, I'm going to close the request, as my primary use case fails for unrelated reasons.

@mdnorman
Copy link

mdnorman commented Jun 8, 2024

Maybe you can also do something like this:

import { test as baseTest } from 'vitest'



export const withTransaction = baseTest.extend({

  transaction: ({ database }, use) => {

    await database.transaction(() => {

      await use()

    })

  }

})

I haven't tried it, but I think it should work 🤔

I was hoping to be able to use a fixture for something like this, but apparently the async context doesn't get passed to the underlying test function. See #5858

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants