feat: improve code coverage #438

qalisander · 2024-12-08T15:56:16Z

Disable code coverage for sol! and sol_interface! macros.
Enforce #[storage] macro instead of sol_storage!. (#[storage] struct is recognized by codecov)
Add missing tests to code coverage.

Towards #426

netlify · 2024-12-08T15:56:36Z

✅ Deploy Preview for contracts-stylus canceled.

Name	Link
🔨 Latest commit	`733f2e7`
🔍 Latest deploy log	https://app.netlify.com/sites/contracts-stylus/deploys/67599c8f56a0010008e38ca2

codecov · 2024-12-08T15:58:50Z

Codecov Report

Attention: Patch coverage is 13.33333% with 26 lines in your changes missing coverage. Please review.

Project coverage is 89.6%. Comparing base (49ff333) to head (733f2e7).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
contracts/src/access/control.rs	0.0%	2 Missing ⚠️
...tracts/src/token/erc1155/extensions/uri_storage.rs	60.0%	2 Missing ⚠️
...ntracts/src/token/erc721/extensions/uri_storage.rs	0.0%	2 Missing ⚠️
contracts/src/access/ownable.rs	0.0%	1 Missing ⚠️
contracts/src/access/ownable_two_step.rs	0.0%	1 Missing ⚠️
contracts/src/finance/vesting_wallet.rs	0.0%	1 Missing ⚠️
...racts/src/token/erc1155/extensions/metadata_uri.rs	0.0%	1 Missing ⚠️
contracts/src/token/erc1155/extensions/supply.rs	0.0%	1 Missing ⚠️
contracts/src/token/erc1155/mod.rs	0.0%	1 Missing ⚠️
contracts/src/token/erc20/extensions/capped.rs	0.0%	1 Missing ⚠️
... and 13 more

Additional details and impacted files

Files with missing lines	Coverage Δ
contracts/src/utils/cryptography/ecdsa.rs	`47.1% <ø> (+14.1%)`	⬆️
contracts/src/access/ownable.rs	`94.1% <0.0%> (+35.8%)`	⬆️
contracts/src/access/ownable_two_step.rs	`99.2% <0.0%> (+26.8%)`	⬆️
contracts/src/finance/vesting_wallet.rs	`55.6% <0.0%> (+24.4%)`	⬆️
...racts/src/token/erc1155/extensions/metadata_uri.rs	`78.5% <0.0%> (+49.1%)`	⬆️
contracts/src/token/erc1155/extensions/supply.rs	`81.1% <0.0%> (+9.4%)`	⬆️
contracts/src/token/erc1155/mod.rs	`96.9% <0.0%> (+24.9%)`	⬆️
contracts/src/token/erc20/extensions/capped.rs	`83.3% <0.0%> (+64.8%)`	⬆️
contracts/src/token/erc20/extensions/metadata.rs	`22.2% <0.0%> (+18.0%)`	⬆️
contracts/src/token/erc20/extensions/permit.rs	`0.0% <0.0%> (ø)`
... and 14 more

... and 10 files with indirect coverage changes

.github/workflows/test.yml

contracts/src/finance/vesting_wallet.rs

contracts/src/utils/math/storage.rs

contracts/src/finance/vesting_wallet.rs

…ov-great-again # Conflicts: # contracts/src/token/erc721/extensions/consecutive.rs

…ov-great-again

0xNeshi

Refactoring party 😄 Nice work, added a couple of comments

contracts/src/token/erc721/mod.rs

contracts/src/utils/cryptography/ecdsa.rs

contracts/src/utils/nonces.rs

lib/motsu-proc/src/test.rs

contracts/src/access/control.rs

lib/motsu-proc/src/test.rs

0xNeshi · 2024-12-10T14:03:30Z

lib/motsu-proc/src/test.rs

-            #( #contract_declarations )*
-            let res = #fn_block;
+            let test = | #( #contract_arg_defs ),* | #fn_block;
+            let res = test( #( #contract_args ),* );
            ::motsu::prelude::Context::current().reset_storage();
            res
        }


In the PR description you mention that you added missing tests to code coverage, but there aren't any here.
Maybe you forgot to push?

That is a very good question:D
Actually these few lines of code (where you commented) did main job..
Each test case body, even marked with #[motsu::test] is included into code coverage now.
By the end we've received more covered code lines, that inflated total code coverage by 11-12%.
A bit scammy, but I feel there is more value in test cases and test functions included into coverage. Considering that, we can increase our target for a higher percentage of coverage by the end.

The "missing tests" I was referring to both here and in my findings are tests for non-covered paths in our contracts (points 4-7 in findings) 😅 Many of our code paths are not covered, even the ones that are not dependent on interacting with other contracts. If you don't plan on adding those now (I wouldn't blame you, we should add them as we go along), could you then update the description as per #438 (comment)?

I really don't see the benefit of including tests in code coverage. The primary concern of code coverage metrics is ensuring the reliability and correctness of the production code, or in our case the contracts.

This is why cargo-llvm-cov, our coverage tool, excludes test files by default.

Making sure that test lines are covered serves no practical purpose other than inflating the coverage percentage.

Yeah.. and I was referring to an actual test body:D
This pr has frequent conflicts with a new code added. Make sense to merge it sooner.

There are many uncovered places in the code base (mostly covered by e2e, but not motsu). So there is a work left to be done.

We can think about centralized approach of excluding all test cases. But not as it was before: some tests are included, other randomly excluded. Coverage of missing lines has bigger priority at the moment:D

bidzyyys

Great job @qalisander !

.github/workflows/test.yml

contracts/src/access/control.rs

contracts/src/access/ownable.rs

contracts/src/access/ownable_two_step.rs

contracts/src/utils/cryptography/ecdsa.rs

contracts/src/utils/nonces.rs

contracts/src/utils/pausable.rs

lib/motsu-proc/src/test.rs

bidzyyys · 2024-12-10T17:26:53Z

@qalisander you also need to change in antora docs sol_storage! -> #[storage]

0xNeshi

Nice work 🔥

contracts/src/token/erc1155/extensions/uri_storage.rs

qalisander · 2024-12-11T10:14:17Z

I've updated antora docs, examples and readme's. Now all of them should use #[storage] macro

bidzyyys

Great job @qalisander! Go go go 🚀

README.md

contracts/src/token/erc1155/mod.rs

qalisander added 4 commits December 8, 2024 15:07

remove sol! macros from code coverage

2c50b6d

replace sol_storage! with #[storage]

ff255cd

use nightly in codecov ci

89bc016

remove sol_interface! macro from code coverage

18e3a16

qalisander added 6 commits December 8, 2024 20:07

++

fe7f77b

++

1536d68

++

9ff05dd

include a test case in code coverage

2e1fd70

exclude sol! macros from code coverage

eca3bd4

++

8774ec3

0xNeshi reviewed Dec 9, 2024

View reviewed changes

.github/workflows/test.yml Show resolved Hide resolved

.github/workflows/test.yml Show resolved Hide resolved

contracts/src/finance/vesting_wallet.rs Show resolved Hide resolved

contracts/src/utils/math/storage.rs Outdated Show resolved Hide resolved

qalisander commented Dec 9, 2024

View reviewed changes

contracts/src/finance/vesting_wallet.rs Show resolved Hide resolved

0xNeshi mentioned this pull request Dec 9, 2024

feat: Investigate Test Coverage Drop #427

Closed

qalisander added 4 commits December 10, 2024 14:33

Merge remote-tracking branch 'origin/main' into qalisander/make-codec…

ca03c33

…ov-great-again # Conflicts: # contracts/src/token/erc721/extensions/consecutive.rs

update description for the workflow

9881bfa

Merge remote-tracking branch 'origin/main' into qalisander/make-codec…

5fc416f

…ov-great-again

use #[storage] in erc1155 supply

7060928

qalisander marked this pull request as ready for review December 10, 2024 11:13

qalisander requested a review from bidzyyys as a code owner December 10, 2024 11:13

qalisander requested a review from 0xNeshi December 10, 2024 12:16

0xNeshi reviewed Dec 10, 2024

View reviewed changes

Merge branch 'main' into qalisander/make-codecov-great-again

fadc0d0

bidzyyys reviewed Dec 10, 2024

View reviewed changes

bidzyyys and others added 3 commits December 10, 2024 18:27

Merge branch 'main' into qalisander/make-codecov-great-again

1ac80c1

format "*.yml" files

a5a2fba

++

8d41e4b

0xNeshi approved these changes Dec 11, 2024

View reviewed changes

contracts/src/token/erc1155/extensions/uri_storage.rs Outdated Show resolved Hide resolved

qalisander added 4 commits December 11, 2024 12:52

exclude e2e and motsu from codecov

49fa1e4

exclude examples from code coverage

6e7e5a4

migrate examples to #[storage] macro

200c2c0

migrate readme's to #[storage] macro

b52baa6

qalisander force-pushed the qalisander/make-codecov-great-again branch from 3a7bc3b to b52baa6 Compare December 11, 2024 09:47

qalisander added 2 commits December 11, 2024 14:09

migrate adoc's to #[storage] macro

495294d

++

6df192f

bidzyyys self-requested a review December 11, 2024 12:54

qalisander added 2 commits December 11, 2024 17:20

++

19f284d

remove unused erc1155 in test

733f2e7

bidzyyys approved these changes Dec 12, 2024

View reviewed changes

README.md Show resolved Hide resolved

contracts/src/token/erc1155/mod.rs Show resolved Hide resolved

qalisander merged commit 603e74e into main Dec 12, 2024
24 checks passed

0xNeshi mentioned this pull request Dec 16, 2024

[Feature]: Investigate & Increase Test Coverage #426

Open

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improve code coverage #438

feat: improve code coverage #438

qalisander commented Dec 8, 2024 •

edited

Loading

netlify bot commented Dec 8, 2024 •

edited

Loading

codecov bot commented Dec 8, 2024 •

edited

Loading

0xNeshi left a comment

0xNeshi Dec 10, 2024

qalisander Dec 10, 2024 •

edited

Loading

0xNeshi Dec 10, 2024 •

edited

Loading

qalisander Dec 11, 2024 •

edited

Loading

bidzyyys left a comment

bidzyyys commented Dec 10, 2024

0xNeshi left a comment

qalisander commented Dec 11, 2024

bidzyyys left a comment

feat: improve code coverage #438

feat: improve code coverage #438

Conversation

qalisander commented Dec 8, 2024 • edited Loading

netlify bot commented Dec 8, 2024 • edited Loading

✅ Deploy Preview for contracts-stylus canceled.

codecov bot commented Dec 8, 2024 • edited Loading

Codecov Report

0xNeshi left a comment

Choose a reason for hiding this comment

0xNeshi Dec 10, 2024

Choose a reason for hiding this comment

qalisander Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

0xNeshi Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

qalisander Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

bidzyyys left a comment

Choose a reason for hiding this comment

bidzyyys commented Dec 10, 2024

0xNeshi left a comment

Choose a reason for hiding this comment

qalisander commented Dec 11, 2024

bidzyyys left a comment

Choose a reason for hiding this comment

qalisander commented Dec 8, 2024 •

edited

Loading

netlify bot commented Dec 8, 2024 •

edited

Loading

codecov bot commented Dec 8, 2024 •

edited

Loading

qalisander Dec 10, 2024 •

edited

Loading

0xNeshi Dec 10, 2024 •

edited

Loading

qalisander Dec 11, 2024 •

edited

Loading