fix: support hardhat_reset

[adjisb]

No description provided.

[nchamo]

Hey @adjisb , just wanted to say that this change also fixed the problem for me 😄

[jummy123]

Hi @adjisb this patch also works in my use case where I reset hardhat and fork to a specific block number before each test. Are you still trying to get this merged or is there another accepted way to get solidity-coverage working with hardhat_reset.

[adjisb]

I don't know if there is a better way and what is needed to get this PR merged.

[jummy123]

@cgewecke sorry for calling you out on this PR. This is blocking us from using coverage ATM, is there any feedback on this PR. I'm happy to help out fixing this if you had an alternate fix in mind.

[cgewecke]

Apologies, will take a look this weekend.

[Shelvak]

I can confirm that the patch is working as expected too =) Thanks for the easy patch @adjisb =)

[cgewecke]

Looks good! Thank you.

[cgewecke]

@adjisb @Shelvak

I have written a test for this in #681 but it fails. My expectation is that given a contract with 2 simple functions I can:

• call function 1
• hardhat_reset
• call function 2
  ... and have 100% line coverage.

Is this what you are seeing when you use the solution in this PR?

In the hardhat code it looks like the hardhat node is destroyed and recreated on each reset event. For solidity-coverage to re-attach a step listener to its vm it's important that we get a reference to the new node object. Have found this to be impossible so far.

Event listeners attached to the new (or is it old for some reason?) vm never fire.

Could either of you (or anyone in this thread) comment on whether this is correct? It could be that I've made a wrong assumption somewhere here or something about the test environment in this repo is resulting in a false failure.

Note: I've slightly modified the logic in the new PR to use an hre scoped reference to the network provider but the incorrect behavior exists for this PR's approach as well. Have also tried:

• using setTimeout in various places to make sure I'm not running into an event related race condition.
• re-running nomiclabsUtils.setupHardhatNetwork(env, api, ui) in the reset event call back
  • interestingly, during the attachment sequence this discovered a root provider object with no hardhat node object. It's possible we may not be able to get a new reference to the node because of the way the provider object is proxied on reset in hardhat-ethers

[Shelvak]

Ho @cgewecke I'll pull your branch and test locally. In my case the coverage fails just with one file and one test with the reset in the before callback. Will back to you after I found a way to fix or to make the test pass =)

[Shelvak]

@cgewecke it's maybe because of the "multiple describe"... This is the way it's failing me:

const { expect } = require("chai");
const { ethers } = require("hardhat");

describe("contractA", function() {
  let instance;
  let startBlockNumber;

  before(async () => {
    await hre.network.provider.request({
      method: "hardhat_reset",
      params: [
        {
          forking: {
            jsonRpcUrl: hre.network.config.forking.url,
            blockNumber: hre.network.config.forking.blockNumber
          },
        },
      ],
    });

    const factory = await ethers.getContractFactory("ContractA");
    instance = await factory.deploy();
  });

  it('sends', async function(){
    await instance.sendFn();
    await instance.sendFn2(); // call 2nd fn to pass the coverage part =)
  });
});

And it's confirmed with this test that if I comment the patch the test fails with 0% coverage

Let me know if you need anything else 👍 thanks for the lib, it's amazing =D

Cheers

[cgewecke]

Thanks so much @Shelvak! I think I misunderstood what range of block numbers are ok to pass to hardhat_reset. Your suggestion to restructure the test fixes things.