Skip to content
This repository has been archived by the owner on Mar 17, 2024. It is now read-only.

Add option to collect gas data in before / beforeEach blocks #97

Closed
vongohren opened this issue Aug 14, 2018 · 13 comments
Closed

Add option to collect gas data in before / beforeEach blocks #97

vongohren opened this issue Aug 14, 2018 · 13 comments

Comments

@vongohren
Copy link

So i just want to check what are the dependencies to get this to run.
I have done a simple unbox, getting metacoin example.
~/code/truffle/node_modules/.bin/truffle unbox
Yes im running from the cloned main repo cause im testing some other stuff that needs new developments

Then I got this lib
npm install --save-dev eth-gas-reporter
I started a truffle develop instance on the side.
~/code/truffle/node_modules/.bin/truffle develop
I configured the truffle.js with mock example and added network truffle to target truffle develop instance.

module.exports = {
  networks: {
    development: {
      host: "localhost",
      port: 8545,
      network_id: "*" // Match any network id
    },
    truffle: {
      host: "localhost",
      port: 9545,
      network_id: "*" // Match any network id
    },
    mocha: {
      reporter: 'eth-gas-reporter'
    }
  }
};

The I ran truffle test
~/code/truffle/node_modules/.bin/truffle test --network truffle

But no report.

What did I miss along the way?

@cgewecke
Copy link
Owner

@vongohren The mocha key should be outside the networks key in the config:

module.exports = {
  networks: {
    development: {
      host: "localhost",
      port: 8545,
      network_id: "*" // Match any network id
    },
    truffle: {
      host: "localhost",
      port: 9545,
      network_id: "*" // Match any network id
    }
  },
  mocha: {
    reporter: 'eth-gas-reporter'
  }
};

@vongohren
Copy link
Author

@cgewecke Trololo, those late nights of coding!
Thanks.

Quick question though.
Im running some test on https://github.com/uport-project/ethr-did-registry
I just wonder why the output table does not show any values. What shall be accumulated there?
All methods are clearly called as you can see in the tests of the repo. And in the bottom picture you can see it displays some gas cost of the current tests

screen shot 2018-08-14 at 23 27 05

screen shot 2018-08-14 at 23 28 51

@cgewecke
Copy link
Owner

@vongohren Ah, weird. I'll check it out.....

@andreafspeziale
Copy link

@cgewecke @vongohren Looking at the contracts most of the functions are just views so not changing the state and e.g changeOwner is an internal function so maybe related to this #94 :)

@vongohren
Copy link
Author

vongohren commented Aug 14, 2018

@andreafspeziale so yeah there are alot of view methods in this test, but there is state change on all three methods, changeOwner, addDelegate, addAttribute, but they are internal methods. They are called during the test, and probably where the gas estimation pops up during mocha reporting.

They are called from methods that either does a sign check before, or methods that calls them directly, but using msg.sender onwards.

But yeah @andreafspeziale, it seems that its mentioned in that issue that methods that are not directly called is troublesome. Any further comments on that @cgewecke in regards to #94

@cgewecke
Copy link
Owner

@vongohren @andreafspeziale I'm not sure....just scanning the tests it seems like the public methods call the internal methods and it should be working. I'm going pull the uport repo down and look at what's happening. Will report back tomorrow, sorry :)

Get some sleep!

@cgewecke
Copy link
Owner

cgewecke commented Aug 14, 2018

@vongohren It turns out the problem is that the gas reporter intentionally excludes transactions that happen in a before or beforeEach. The idea is to not pollute the per test measurements with setup costs (see issue #2). That way you can also use the reporter to derive the costs of a complex set of interactions with some contracts without having the deployment costs of the latter show up in the total.

It's quite common for tests like changeOwner to be written like this (although I think the Uport pattern is elegant and makes sense too).

it('should change owner mapping', async () => {
  tx = await didReg.changeOwner(identity, delegate, {from: identity})
  owner = await didReg.owners(identity)
  assert.equal(owner, delegate)
})

@vongohren
Copy link
Author

@cgewecke I see, it is a good choice of architecture, but if one see this pattern happening more and more, maybe add a flag or something in config?

@cgewecke
Copy link
Owner

@vongohren Yes I think you're right - it might be good idea even if that pattern is really rare. Thanks for raising this.

@vongohren
Copy link
Author

@cgewecke yeah i can agree. But if you include this I was going to to a PR adding this to their repo. Free marketing towards uPort :P

@cgewecke
Copy link
Owner

Ah ok! I'll ping you.

@cgewecke cgewecke changed the title What is the dependencies? Simple unbox will not show gas prices Add option to collect gas data in before / beforeEach blocks Sep 14, 2018
@vongohren
Copy link
Author

@cgewecke did anything happen here :)? Was doing some cleaning of open issues

@cgewecke
Copy link
Owner

Hi! eth-gas-reporter is being deprecated in favor of hardhat-gas-reporter.

The latest version at Hardhat has been completely decoupled from mocha.

https://github.com/cgewecke/hardhat-gas-reporter/releases/tag/v2.0.0

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

No branches or pull requests

3 participants