When auditing a smart contract project, the security reviewer should have a close look at the existing tests (assess the quality of the entire test suite) and make recommendations about what should be improved.
There are many ways in which developers choose to test their code. In some cases it’s really easy to give recommendations for improvement, but other times it is not clear what can be improved or how much it should be improved. Here are just a few examples of what I’ve seen in the wild:
No tests whatsoever. In this case it’s easy to recommending adding automated tests that cover all the implemented functionality, e.g. tests such as those that can be run via
Tests which cover all lines of code but have almost no assertions for checking the effects of the executed lines of code. Recommending adding more assertions is the general thing to do here, but it’s hard to quantify the amount and quality of assertions, because tools like solidity-cover do not provide any metrics for assertions. One interesting research topic here would be to come up with metrics for the quality of assertions in test suites.
Tests with 100% line coverage, but <100% branch coverage. This generally means that some code paths are not tested, which might be exactly those edge cases which will lead to an unexpected loss of funds. For such test suites, it is generally recommended to add tests that will pass through those code paths which were not tested. However, it’s important to note that 100% branch coverage could still mean that some paths are not executed, because there is a significant difference between branch coverage and path coverage (see explanation here). Therefore, it’s up to the software devs/testers to think about all plausible paths and test them accordingly. One interesting research topic would be to come up with quality metrics for path coverage. Percentage is not always feasible, because many programs have an infinite number of paths.
Only unit tests for all functions, but no integration tests. For such projects it’s important to recommend adding integration and end-to-end tests which combine sequences of smart contract function calls, possibly from different smart contracts. Such tests would ideally cover happy and unhappy paths in all use-cases of the project.
Tests which have poorly structured code and no comments and which do not have a proper description of what the test is doing, e.g.
it("should always pass"). In such cases the auditors should recommend adding comments and representative descriptions for tests, such that if a test failure occurs it can be easily understood.
This list of examples and recommendations is not exhaustive. Therefore, it would be interesting to hear more examples from other devs/auditors and discuss other possible recommendations that can be given to developers to improve their test suites.