Solidity-coverage: Issue insturmenting abstract contracts

[nventuro]

After upgrading to solidity-coverage 0.6.4 on the OpenZeppelin Contracts repository, I encountered the following error:

There was a problem instrumenting ./coverageEnv/contracts/GSN/IRelayHub.sol: TypeError: Cannot read property 'stop' of null

Note that IRelayHub is an abstract contract: there's no implementation for it on the repo, which I suspect may be related to this.

[nventuro]

I've now noticed other abstract contracts are instrumented with no issues, so that may not be the root cause. For reference, this is IRelayHub's source: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/GSN/IRelayHub.sol

[cgewecke]

@nventuro Yes, it looks like it's something stemming from recent change in solidity-parser-antlr (possibly natspec related) but I'm not sure what. Am fixing it rn by pinning the parser to 0.4.7 for the time being. Will publish a patch shortly...

[cgewecke]

@nventuro 0.6.5 should fix the instrumentation fault but ... I see one failing test in the Zeppelin CI run here...

1) Contract: GSNBouncerERC20Fee
       when relay-called
         charges the sender for GSN fees in tokens:

      AssertionError: expected '166621' to be within '19413' of '194136'
      + expected - actual

      -166621
      +194136

      at Context.<anonymous> (test/GSN/GSNBouncerERC20Fee.test.js:66:44)
      at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:189:7)

The test is here. Does this failure make sense because of a variance in how much gas is consumed by coverage instrumented contracts?

[nventuro]

That test is very gas-related, yes, but I'm surprised that the effect is so large (a 30k difference from the expected value). I'd have to look at it in some more detail.

But that's fine, we can simply skip coverage on that one for now. I'll give 0.6.5 a try tomorrow and report back, thanks so much for the quick turnaround!

[cgewecke]

Yes it looks large to me as well - the coverage client should no longer be charging for events so that difference seems weird.

[alcuadrado]

Yes, it looks like it's something stemming from recent change in solidity-parser-antlr (possibly natspec related) but I'm not sure what. Am fixing it rn by pinning the parser to 0.4.7 for the time being. Will publish a patch shortly...

I got similar problems on truffle-flattener, and someone suggested to pin that same version. There's this issue about it.

It'd be great if solidity-coverage could show the stack traces of those errors. That would help with reporting issues, debugging and implementing workarounds. Maybe those should be behind a flag?

[cgewecke]

It'd be great if solidity-coverage could show the stack traces of those errors.

@alcuadrado Yes this is definitely necessary - all the info is being lost rn.

[nventuro]

This fixed it, thanks!