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

Add showTimeSpent option to also show how long each test took #80

Merged
merged 2 commits into from
Jun 27, 2018

Conversation

ldub
Copy link
Contributor

@ldub ldub commented Jun 27, 2018

Adds an option to the reporter options to show the time spent as well as the gas consumption.

Notes:

  1. Unit tests are currently broken (at least on my system) they fail on master branch when I run npm run test:
  1) Contract: VariableCosts prints a table at end of test suites with failures:
     AssertionError: Unspecified AssertionError
      at Context.it (test/variablecosts.js:49:5)
      at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:188:7)
  1. Would you like to default showTimeSpent to true?
  2. I had to refactor the 'Load config' section from gasStats.js to index.js

Preview:
ethgasreporter

Copy link
Owner

@cgewecke cgewecke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is nice. @chapati23 suggested the same improvement in #76 and I was a little unsure whether it made sense because gas reporter runs slower than un-reported tests. But now that I see it's great. . .

A single test fails intentionally because for a long time there was a problem getting the chart to output when any mocha test failed. So CI here is a weird visual inspection test that always passes while failing . . .

On that note, - if you have a sec could you add your new option to the reporter options for the second visual test here?

RE: should it be on by default: do you think it should be on by default?

@cgewecke
Copy link
Owner

I think it looks slightly cleaner without the time, but this is completely subjective. . . the time is useful if you're trying to make your tests run faster.

@ldub
Copy link
Contributor Author

ldub commented Jun 27, 2018

Interesting, I didn't see that issue before but I read it and I see that other projects run their tests with gas reporter and without to test time and gas separately. I suppose that makes sense, but for most of my purposes I'll be using the gas reporter and turning on showTimeSpent mostly as a sanity check.

I'd opt for on by default as it shows at a glance whether anything is taking an abnormally long time. Your choice though!

Pushing requested changes shortly.

@cgewecke
Copy link
Owner

Awesome @ldub, thank you.

@cgewecke cgewecke merged commit 44eb5c8 into cgewecke:master Jun 27, 2018
@ldub ldub deleted the show-time-spent branch June 27, 2018 01:48
@cgewecke
Copy link
Owner

Published in 0.1.8 - I left it as an option for the moment - will revisit though. . .

@cgewecke cgewecke mentioned this pull request Jun 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants