Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] aptos move coverage is incorrectly calculated #14448

Open
brmataptos opened this issue Aug 28, 2024 · 3 comments
Open

[Bug] aptos move coverage is incorrectly calculated #14448

brmataptos opened this issue Aug 28, 2024 · 3 comments
Assignees
Labels
bug Something isn't working move-cli Issues related to aptos move command stale-exempt Prevents issues from being automatically marked and closed as stale

Comments

@brmataptos
Copy link
Contributor

🐛 Bug

The stats generated by aptos move coverage summary use incorrect covered/unconvered numbers because inlined function line numbers are attributed to the calling module.

The summaries shown by aptos move coverage source likely fail to show inline functions called from tests in other modules.

This will take some effort to sort out.

@brmataptos
Copy link
Contributor Author

The problem may be more severe than this. The summary view is inconsistent with the source view. In the following listing, summary shows module acl has 0.00 Module coverage, but the source view shows that all lines are covered.
Perhaps the interpretation of covered/uncovered is reversed in the source view.

brm@brms-mbp-2 move-stdlib % aptos move coverage summary                    
+-------------------------+
| Move Coverage Summary   |
+-------------------------+
Module 0000000000000000000000000000000000000000000000000000000000000001::bcs
>>> % Module coverage: NaN
Module 0000000000000000000000000000000000000000000000000000000000000001::fixed_point32
>>> % Module coverage: 100.00
Module 0000000000000000000000000000000000000000000000000000000000000001::hash
>>> % Module coverage: NaN
Module 0000000000000000000000000000000000000000000000000000000000000001::vector
>>> % Module coverage: 99.02
Module 0000000000000000000000000000000000000000000000000000000000000001::error
>>> % Module coverage: 0.00
Module 0000000000000000000000000000000000000000000000000000000000000001::acl
>>> % Module coverage: 0.00
Module 0000000000000000000000000000000000000000000000000000000000000001::bit_vector
>>> % Module coverage: 97.39
Module 0000000000000000000000000000000000000000000000000000000000000001::signer
>>> % Module coverage: 100.00
Module 0000000000000000000000000000000000000000000000000000000000000001::features
>>> % Module coverage: 27.60
Module 0000000000000000000000000000000000000000000000000000000000000001::option
>>> % Module coverage: 94.76
Module 0000000000000000000000000000000000000000000000000000000000000001::string
>>> % Module coverage: 81.82
+-------------------------+
| % Move Coverage: 69.67  |
+-------------------------+
{
  "Result": "Success"
}
brm@brms-mbp-2 move-stdlib % aptos move coverage source --module acl
Code coverage per line of code:
  + indicates the line is not executable or is fully covered during execution
  - indicates the line is executable but NOT fully covered during execution
Source code follows:
+ /// Access control list (acl) module. An acl is a list of account addresses who
+ /// have the access permission to a certain object.
+ /// This module uses a `vector` to represent the list, but can be refactored to
+ /// use a "set" instead when it's available in the language in the future.
+ 
+ module std::acl {
+     use std::vector;
+     use std::error;
+ 
+     /// The ACL already contains the address.
+     const ECONTAIN: u64 = 0;
+     /// The ACL does not contain the address.
+     const ENOT_CONTAIN: u64 = 1;
+ 
+     struct ACL has store, drop, copy {
+         list: vector<address>
+     }
+ 
+     /// Return an empty ACL.
+     public fun empty(): ACL {
+         ACL{ list: vector::empty<address>() }
+     }
+ 
+     /// Add the address to the ACL.
+     public fun add(self: &mut ACL, addr: address) {
+         assert!(!vector::contains(&mut self.list, &addr), error::invalid_argument(ECONTAIN));
+         vector::push_back(&mut self.list, addr);
+     }
+ 
+     /// Remove the address from the ACL.
+     public fun remove(self: &mut ACL, addr: address) {
+         let (found, index) = vector::index_of(&mut self.list, &addr);
+         assert!(found, error::invalid_argument(ENOT_CONTAIN));
+         vector::remove(&mut self.list, index);
+     }
+ 
+     /// Return true iff the ACL contains the address.
+     public fun contains(self: &ACL, addr: address): bool {
+         vector::contains(&self.list, &addr)
+     }
+ 
+     /// assert! that the ACL has the address.
+     public fun assert_contains(self: &ACL, addr: address) {
+         assert!(contains(self, addr), error::invalid_argument(ENOT_CONTAIN));
+     }
+ }
{
  "Result": "Success"
}

@brmataptos brmataptos changed the title [Bug] aptos move coverage misattributes inlined code to the calling module [Bug] aptos move coverage is incorrectly calculated Aug 29, 2024
@brmataptos brmataptos self-assigned this Aug 30, 2024
@brmataptos brmataptos moved this from 🆕 New to Assigned in Move Language and Runtime Sep 6, 2024
@brmataptos brmataptos added the move-cli Issues related to aptos move command label Sep 6, 2024
Copy link
Contributor

This issue is stale because it has been open 45 days with no activity. Remove the stale label or comment - otherwise this will be closed in 15 days.

@github-actions github-actions bot added the Stale label Oct 22, 2024
@brmataptos brmataptos added stale-exempt Prevents issues from being automatically marked and closed as stale and removed Stale labels Oct 31, 2024
@brmataptos
Copy link
Contributor Author

To reduce effort debugging, note that you can test changes to the coverage code by running the aptos binary built in the current project:

cd aptos-move/framework/move-stdlib
cargo run --locked --profile ci -p aptos --bin aptos -- move test --coverage |& tee move-test-coverage.out
cargo run --locked --profile ci -p aptos --bin aptos -- move coverage source --module acl |& tee move-coverage-source.out

The first build took 21 minutes for me, but after that it is not rebuilt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working move-cli Issues related to aptos move command stale-exempt Prevents issues from being automatically marked and closed as stale
Projects
Status: Assigned
Development

No branches or pull requests

1 participant