Branch coverage decreases after upgrading to 0.8.7

[mariogutval]

After upgrading solidity-coverage dependency from 0.8.6 to 0.8.7, my branch coverage percentage decreased from 100% to 91.67%.

With 0.8.6:

With 0.8.7:

Apparently, the issue is related to the onlyOwner modifier from OpenZeppelin's Ownable.sol contract. According to the coverage report, the else branch is not being covered.

However, I have logged the results of the following require statement and both of the results are being checked.

The smart contract code has not been modified.

Any thoughts on this?

[cgewecke]

@mariogutval Thanks for reporting. Some questions:

• Are you using viaIR=true?
• Have you run npx hardhat clean since updating the package?

Also any info you can share about your compiler config in hardhat.config.ts and your .solcover.js would help to reproduce and debug this.

[mariogutval]

• Are you using viaIR=true?

No, viaIR is not set to true anywhere in the configuration.

• Have you run npx hardhat clean since updating the package?

Yes, I did it.

Also any info you can share about your compiler config in hardhat.config.ts and your .solcover.js would help to reproduce and debug this.

Sure, this is my compiler config in hardhat.config.ts:

solidity: {
  compilers: [
    {
      version: "0.8.17",
      settings: {
        optimizer: {
          enabled: true,
          runs: 200,
        },
      },
    },
  ],
}

Apart from that, I don't have any .solcover.js file.

[cgewecke]

@mariogutval Ok all of that looks good. Will investigate...

[mariogutval]

Thank you!

[cgewecke]

@mariogutval The difficulty looking into this is that we have extensive unit tests for the modifier coverage and we also run an E2E integration test where the latest commit here is injected into Zeppelin's own unit tests. Results have remained constant across the most recent releases.

For example I just opened a PR to double-check this (#864) and the coverage for latest solidity-coverage on Zeppelin vs. v0.8.5 running in Zeppelin's own CI is identical.

Idk if you're open to sharing the repo you're working on but I'm not sure how to proceed with this without a reproduction case...the tests I would use to verify the modifier cov works as expected LGTM.

[mariogutval]

Okay it's so weird then. Unfortunately, I can't share the repo with you, but let me try to create a sample repo to reproduce the error.

Thanks for the help.

[doublesharp]

@cgewecke I just ran into this same bug in this public repo https://github.com/PaintSwap/samwitch-vrf/blob/a6b3fb916960ffbda2d0a23f481574e68a9ec482/contracts/SamWitchVRF.sol#L164

It shows the onlyOwner modifier on the _authorizeUpgrade() function as being an uncovered branch, although there is a test for it. If I make a "local" modifier to check do the same check the coverage goes to 100%.

Uncovered branch:

  function _authorizeUpgrade(address newImplementation) internal override onlyOwner {}

Fully covered:

  modifier onlyOperator() {
    _checkOwner();
    _;
  }

  function _authorizeUpgrade(address newImplementation) internal override onlyOperator {}

I was able to reproduce this behavior by copying the OwnableUpgradeable.sol from OpenZeppelin locally, inheriting from it, and commenting out all of the uncovered branches/lines to get to 100% coverage.

If I then update .solcover.js to add to skipFiles, the modifier branch becomes uncovered in the contract I am testing.

Further modifying the .solcover.js to add modifierWhitelist: ["onlyOwner"], does get back to 100% branch coverage but isn't ideal as we do want to ensure this modifier has coverage.