hardhat icon indicating copy to clipboard operation
hardhat copied to clipboard

Nested calls to loadFixture() do not always work

Open m1cm1c opened this issue 1 year ago • 5 comments

When passing a function to loadFixture() that itself calls loadFixture(), this sometimes fails with an error message saying that loaded fixtures are not supported. This happens especially often if nesting more than once.

     FixtureSnapshotError: There was an error reverting the snapshot of the fixture.

This might be caused by using nested loadFixture calls in a test, for example by using multiple beforeEach calls. This is not supported yet.
      at loadFixture (node_modules/@nomicfoundation/hardhat-network-helpers/src/loadFixture.ts:47:15)
      at runMicrotasks (<anonymous>)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
      at async recoverViaDistributor (test/Wallet2.ts:111:9)

m1cm1c avatar Jul 21 '22 20:07 m1cm1c

This issue is also being tracked on Linear.

We use Linear to manage our development process, but we keep the conversations on Github.

LINEAR-ID: 8b4add10-05ff-4ebc-bb1a-abba664fa085

github-actions[bot] avatar Jul 21 '22 20:07 github-actions[bot]

I propose a simple way of circumventing this issue in #2981.

m1cm1c avatar Jul 21 '22 20:07 m1cm1c

Running into this as well. Even if all "describe" blocks are removed the error still happens; so the error message isn't the clearest.

faces-of-eth avatar Aug 07 '22 04:08 faces-of-eth

Hey @m1cm1c, thanks for opening this. I'm a bit confused though. The way I would think about composing fixtures is that you would have something like:

function deployTokenFixture() {
  // deploy a token
}

function deployWrappedTokenFixture() {
  const { token } = await deployTokenFixture();

  const wrappedToken = ...;

  return { token, wrappedToken }
}

And then in some tests you do loadFixture(deployTokenFixture) and in others you do loadFixture(deployWrappedTokenFixture), but you don't use loadFixture inside a fixture function. Does that make sense? And if this doesn't work for your use case, can you explain why?

fvictorio avatar Aug 08 '22 18:08 fvictorio

@fvictorio This is exactly what my proposed fix boils down to, except it has the advantage that if true nested fixture support were to ever be implemented (i.e. not executing deployTokenFixture() again when deployWrappedTokenFixture() gets executed), tests would already be implemented in the way that can take advantage of that future performance improvement.

However, documenting how to do it would be almost equivalent to the fix. When first encountered fixtures, I thought you could use them in any way to skip duplicate chain mutations, including of course inside another sequence of chain mutations. If the way to avoid the problem were documented, there'd be no need for the fix.

m1cm1c avatar Aug 09 '22 07:08 m1cm1c

When first encountered fixtures, I thought you could use them in any way to skip duplicate chain mutations, including of course inside another sequence of chain mutations. If the way to avoid the problem were documented, there'd be no need for the fix.

This is very useful feedback, thank you

fvictorio avatar Aug 10 '22 13:08 fvictorio

I've already captured this in our documentation backlog, so I'm going to close this issue now. Thanks again.

fvictorio avatar Aug 11 '22 08:08 fvictorio

I am a bit confused @fvictorio - is calling loadFixture inside another fixture function to apply chain mutations on top of the first fixture's snapshot now supported or ever will be? This could definitely lead to some potentially large performance improvements.

sebastianst avatar Aug 20 '22 14:08 sebastianst

It's not supported yet. Maybe it could be supported in the future, but it feels like an anti-pattern to me. IMO each fixture should be about deploying contracts and sending transactions. Executing a fixture with loadFixture should be orthogonal to that (in my mind at least, I'm open to being wrong here).

I'm not sure if supporting this would lead to large performance improvements. Take my example from before:

function deployTokenFixture() {
  // deploy a token
}

function deployWrappedTokenFixture() {
  const { token } = await deployTokenFixture();

  const wrappedToken = ...;

  return { token, wrappedToken }
}

If you use the first fixture in 10 tests and the second fixture in other 10 tests, the first fixture would be executed twice (when it's called with loadFixture for the first time, and when it's called through the other fixture also for the first time) and the second fixture once.

If it was possible to call loadFixture inside the second fixture, then both fixtures would be executed only once. While this is (kind of) a 33% improvement, it's a small change compared to the speedup you get when you go from deploying in each test (where the fixtures are executed 20 and 10 times, respectively) to deploying only once via the loadFixture helper.

Does that make sense?

That being said, we do want to support nested calls to loadFixture for other reasons (nested describes with beforeEachs that deploy things) and when that is done, then I think what you are asking for will also be possible.

fvictorio avatar Aug 20 '22 22:08 fvictorio

Yes that makes sense and in the scenario you described (10 tests for each fixture) the performance improvements are indeed marginal.

I just encountered a different scenario where the first fixture is large and the additional steps in the second fixture are small but still worthwhile putting into a fixture. Also, the second fixture would only be called 2-3 times. In this scenario, the cost of running the first=main fixture is not so easily amortized.

But happy to hear that it will soon be properly supported!

sebastianst avatar Aug 22 '22 09:08 sebastianst