cgewecke / hardhat-gas-reporter

Gas Usage Analytics for Hardhat
MIT License
412 stars 57 forks source link

Support for parallel tests #98

Closed fvictorio closed 2 years ago

fvictorio commented 2 years ago

We are working on adding support for parallel tests in Hardhat and one of our concerns is how it works with hardhat-gas-reporter. I tested it locally and found some errors when Mocha executes in parallel.

The first problem looks like this:

TypeError [ERR_INVALID_ARG_TYPE]: The "listener" argument must be of type function. Received undefined

And it's thrown here. This is weird and I couldn't find the reason why it happens. But I did find that adding this line in the overridden subtask fixes the problem:

       if (hre.network.name === HARDHAT_NETWORK_NAME || options.fast){
         const wrappedDataProvider= new EGRDataCollectionProvider(hre.network.provider,mochaConfig);
+        await hre.network.provider.send("eth_blockNumber");
         hre.network.provider = new BackwardsCompatibilityProviderAdapter(wrappedDataProvider);

         const asyncProvider = new EGRAsyncApiProvider(hre.network.provider);

That call forces the existing provider to be initialized, and so the function exists. Obviously, this is not the right fix, but it's a hint that the problem could be related to the custom providers created by the plugin.

The second problem looks like this:

  1) Uncaught error outside test suite:
     Uncaught TypeError: Converting circular structure to JSON
    --> starting at object with constructor 'Contract'
    |     property 'location' -> object with constructor 'SourceLocation'
    |     property 'file' -> object with constructor 'SourceFile'
    |     property 'contracts' -> object with constructor 'Array'
    --- index 0 closes the circle

I'm more confident about what's causing this one. I think it's this line in the plugin:

        mochaConfig.reporterOptions.provider = asyncProvider;

My guess is that Mocha stringifies the config for some reason (maybe to send it to the workers it creates to run the tests?) and then an error is thrown because the provider is a non-serializable object.


All that being said, I think a first step would be to just detect that parallel mode is being used and throw a proper error. That can be done like this:

    if (args.parallel === true) {
      throw new HardhatPluginError("hardhat-gas-reporter", `Parallel tests are not supported`);
    }
cgewecke commented 2 years ago

Thanks so much for looking into this @fvictorio.

I think a first step would be to just detect that parallel mode is being used and throw a proper error

Agree, will do that right now.

Is there a task hook (executed before test) where a plugin that is wrapping the provider could initialize the globally injected provider? Perhaps that would be a way around the serialization issue - e.g if we stopped passing it as an in-memory arg to mocha via the config...

(Would need some other changes here as well but seems like the biggest obstacle).

I can't do any setup asynchronously in the mocha reporter because those hooks are all sync (unless they've improved this recently).

fvictorio commented 2 years ago

I can't do any setup asynchronously in the mocha reporter because those hooks are all sync

I think you could use a root hook plugin for that. I'm not 100% sure if it fits your use case, but I think it would be something like:

exports.mochaHooks = {
  beforeAll(done) {
    // wrap the provider
    done();
  }
};

And then you add that to the require array of the mocha config.

Is there a task hook (executed before test) where a plugin that is wrapping the provider could initialize the globally injected provider?

I don't think that's possible right now (cc @alcuadrado)

cgewecke commented 2 years ago

I think you could use a root hook plugin for that

Oh!!! That looks really useful.

alcuadrado commented 2 years ago

Is there a task hook (executed before test) where a plugin that is wrapping the provider could initialize the globally injected provider?

I don't think that's possible right now (cc @alcuadrado)

Confirming this

cgewecke commented 2 years ago

This is published (with suggested revision to use a warning instead of an error) in 1.0.8.

Just lmk if you see this not working for some reason...thanks again for advice, etc :)