OpenZeppelin / openzeppelin-contracts

OpenZeppelin Contracts is a library for secure smart contract development.
https://openzeppelin.com/contracts
MIT License
24.89k stars 11.78k forks source link

Potential Issue with Test Coverage and Functionality in Finance Module #5167

Open techvoyagerX opened 2 months ago

techvoyagerX commented 2 months ago

During my review of the finance module's test suite in the OpenZeppelin Contracts repository, I observed potential concerns regarding the adequacy and reliability of the tests, particularly in achieving full coverage. Specifically:

Test Coverage: While attempting to run the coverage report using brownie coverage, I encountered errors that prevented the successful generation of the full coverage report. This issue may hinder the ability to assess the robustness of the test suite fully.

Test Functionality: I noticed that some tests might not be validating the correct behavior. For instance, certain tests are using the wrong accounts or parameters, which could lead to false positives/negatives during test execution.

Given the importance of maintaining high standards for test coverage and accuracy, especially in a widely-used library like OpenZeppelin Contracts, I recommend reviewing and enhancing the existing test suite in the finance module. This will ensure that all critical paths are covered and that the tests are correctly validating the intended functionality.

Amxx commented 2 months ago

Hello @techvoyagerX

We run test coverage using hardhat and solidity-coverage. Coverage run is done using scripts/checks/coverage.sh. This is integrated into the CI, which validates a high level of coverage.

Last commit on master is documented here

As for the tests, if you find any of our test to be erroneous, please let us know which one exactly so we can fix that.