[Feature]: Investigate & Increase Test Coverage

[0xNeshi]

What is the feature you would like to see?

• Find out why our test coverage suddenly fell from ~80% to ~65%
• Add additional tests to increase this % as close to 95% as possible

Problems and Recommendations on Increasing Coverage %

NOTE: for more details on each point, see #426 (comment). The below checklist is taken and updated from there.

• Solidity code used in sol! macros (events, errors, storage) is included in test coverage. feat: improve code coverage #438
• Missing unit tests:
  • functions that interact with other contracts.
  • ./lib/motsu/, ./lib/motsu-proc/, ./lib/e2e/, and ./lib/e2e-proc/. feat: improve code coverage #438
  • re-exported functions.
  • certain flows, and even whole functions in certain contracts and in contracts/src/utils.
• Using internal functions instead of public ones (e.g. IErc20::transfer is tested by calling _update, causing code coverage to register transfer as "unconvered").
• IErc165-related tests exist verifying the generated INTERFACE_ID value is correct, but no tests exist that call <ContractName>::supports_interface.
• MethodError impls uncovered.
• #[public] attr is included in coverage. feat: improve code coverage #438
• #[cfg(test)] mod tests (unit tests) are included in coverage.
  • #[motsu::test] attributes included in test coverage.

Contribution Guidelines

• I agree to follow this project's Contribution Guidelines

[0xNeshi]

Pasting the findings here for future reference.

Reason for Coverage % Nosedive

Codecov shows that the % drop happened somewhere between October 28th and November 4th. Further investigation shows that the "problematic" commit was introduced by PR #379 (the commit is not actually "problematic", read further).

Below is the link to all the files for which coverage was affected in this commit:
https://app.codecov.io/github/OpenZeppelin/rust-contracts-stylus/commit/9c2b1b4b23145acddafb7b888e9e69a09fa6134c/indirect-changes

Explaining the coverage file diff on Codecov in case it helps (link above): the image below shows the coverage change for contracts/src/token/erc20/extensions/capped.rs - it shows that previously this motsu test was included in the coverage report (the highlighted left column shows line numbers included in previous coverage, while the non-highlighted right column shows line numbers now excluded from coverage).

In short, it seems that up until this commit, all motsu tests were included in the coverage %! This wrongly increased our coverage %. After this commit, the motsu tests were excluded from coverage. That means that the sudden 12% drop in coverage was not an anomaly, but a correction.

The commit did not manually change anything to affect coverage in this way. The reason why tests were magically excluded is not yet absolutely clear, but it seems most likely that this motsu::test implementation change, which no longer made the fn block wrapped by with_context, made llvm-cov (our coverage tool) aware that this is in fact test code, and that it shouldn't be a part of the coverage report.
There were also some changes to the llvm-cov dependencies in this period, which could've affected the coverage step in our CI, but this is much less likely imo.

Problems and Recommendations on Increasing Coverage %

Below is a list of problems affecting our test coverage and possible mitigation steps:

1. Solidity code used in sol! macros (events, errors, storage) is included in test coverage (effect: high).
   • Mitigation 1: group Solidity code in an internal module annotated with #[cfg_attr(coverage, coverage(off))] (see example, and pseudo code is below).

   #[cfg_attr(coverage, coverage(off))]
   mod sol_defs {
       sol! {
           event SomeEvent();
           // ...
       }
   
       sol! {
           error SomeError();
           // ...
       }
   
       // NOTE: has to be included, as `SolidityError` affects coverage,
       // so has to be ignored
       #[derive(SolidityError, Debug)]
       pub enum Error {
           // ...
       }
   
       sol_storage! {
           pub struct State {
               // ...
           }
       }
   }
   
   // use in the contract and also reexport
   pub use sol_defs::{State, Error, SomeError, SomeEvent};

   The effect of this single change on VestingWallet is a ~6% increased coverage.
   • Mitigation 2: we kindly ask the alloy and stylus teams to update their macros to figure out a way to apply attributes to the generated code, and once they do, we annotate each of the problematic macros with coverage(off).
   • NOTE: I did just recommend we increase our code coverage by... ignoring code. The irony is not lost on me :kek:
2. Missing unit tests for functions that interact with other contracts (effect: high).
   • Mitigation 1: will be possible to add these after feat(test): unit test external call mock #423.
   • Mitigation 2: including integration tests in the report might solve this issue.
3. Missing tests for ./lib/motsu/, ./lib/motsu-proc/, ./lib/e2e/, and ./lib/e2e-proc/ (effect: high).
   • NOTE: only ./lib/e2e/ is not included in the coverage report, and the reason is that it is not included in Cargo.toml > default-members and we do not include it explicitly in our coverage command. This should be addressed.
   • Mitigation 1: add missing tests.
   • Mitigation 2: exclude ./lib/ from coverage.
4. Re-exported functions missing unit tests (effect: medium/high).
   • Mitigation 1: duplicate missing unit tests.
   • Mitigation 2: annotate relevant functions with #[cfg_attr(coverage, coverage(off))].
5. Missing unit tests for certain flows, and even whole functions in certain contracts and in contracts/src/utils (effect: medium).
   • Mitigation: add missing unit tests.
6. Using internal functions instead of public ones (e.g. IErc20::transfer is tested by calling _update, causing code coverage to register transfer as "unconvered") (effect: medium).
   • Mitigation: test publicly exposed fns, not the internal ones.
7. IErc165-related tests exist verifying the generated INTERFACE_ID value is correct, but no tests exist that call <ContractName>::supports_interface (effect: low).
   • Mitigation: add missing supports_interface tests.
8. MethodError impls uncovered (effect: low).
   • Mitigation: annotate with #[cfg_attr(coverage, coverage(off))].
9. #[public] attr is included in coverage (effect: low). - this PR solves this
   • Mitigation: open to suggestions.
   • NOTE: it seems switching llvm-cov to nightly in this PR removed this from coverage (e.g. see nonces.rs) 👀
10. #[cfg(test)] mod tests (unit tests) are included in coverage (effect: none). NOTE: technically not a problem atm, as the effect of this is positive or neutral. If we still wanted to exclude them, see mitigation steps below.
    • Mitigation 1: annotate tests module with #[cfg_attr(coverage, coverage(off))]
    • Mitigation 2: move all unit tests to separate tests.rs files, which would be automatically ignored (see Ignore all #[test]-related code automatically taiki-e/cargo-llvm-cov#123 (comment)) (this ties into our previous discussion on unit test organization where this was one of the proposals https://openzeppelin.slack.com/archives/C06R5AGFE9G/p1732625889805619?thread_ts=1732618832.030739&cid=C06R5AGFE9G)
    • Mitigation 3: Integrate https://github.com/taiki-e/coverage-helper into motsu
11. #[motsu::test] attributes included in test coverage (effect: unknown). - this PR solves this
    • Explanation: nothing from mod tests should be included in coverage (see previous point). However, since ./lib/motsu/, and ./lib/motsu-proc/ do not have their own tests (already covered in previous points), we are inadvertently covering those in this roundabout way. I think this is a bug though, and not a feature.
    • Mitigation: see previous point about ignoring mod tests.
    • NOTE: it seems switching llvm-cov to nightly in this PR removed this from coverage (e.g. see nonces.rs) 👀 But for some reason, motsu-proc crate code is still "covered".

---

Took the liberty to investigate why the number of ignored integration tests jumped 2->10, namely in Erc721 and Erc1155 contracts. The reason for this is that e2e does not support asserting that a call reverted with stylus_sdk::call::Error, which these tests are trying to assert.
This issue is already known and is being tracked here: #228