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][compiler-v2] Coverage testing DevEx issues #15117

Open
alnoki opened this issue Oct 30, 2024 · 6 comments
Open

[Bug][compiler-v2] Coverage testing DevEx issues #15117

alnoki opened this issue Oct 30, 2024 · 6 comments
Assignees
Labels
bug Something isn't working compiler-v2 stale-exempt Prevents issues from being automatically marked and closed as stale

Comments

@alnoki
Copy link
Contributor

alnoki commented Oct 30, 2024

@brmataptos @fEst1ck @georgemitenkov @rahxephon89 @runtian-zhou @vineethk @wrwg

--move-2 flag DevEx

I am trying to run aptos move test --coverage for this commit of an ongoing Move v2 red-black tree implementation, but it is failing with the following trace:

aptos move test --coverage      
INCLUDING DEPENDENCY AptosFramework
INCLUDING DEPENDENCY AptosStdlib
INCLUDING DEPENDENCY MoveStdlib
BUILDING RedBlackMap
error[E01013]: unsupported language construct
  ┌─ /Users/alex/repos/econia-v5/src/move/research/red-black-map/variants/b/sources/red_black_map.move:6:5
  │
6 │     enum Color has copy, drop {
  │     ^^^^ Move 2 language construct is not enabled: enum types

error[E01013]: unsupported language construct
   ┌─ /Users/alex/repos/econia-v5/src/move/research/red-black-map/variants/b/sources/red_black_map.move:60:38
   │
60 │             if (parent_ref_mut.color is Color::Black)
   │                                      ^^ Move 2 language construct is not enabled: `is` expression

error[E01013]: unsupported language construct
   ┌─ /Users/alex/repos/econia-v5/src/move/research/red-black-map/variants/b/sources/red_black_map.move:81:70
   │
81 │             if (uncle_index == NIL || (self.nodes[uncle_index].color is Color::Black)) {
   │                                                                      ^^ Move 2 language construct is not enabled: `is` expression

EDIT: Per #15117 (comment), tests pass for aptos move test --coverage --move-2 --dev, but is there a way to eliminate the need to pass --move-2 all over the place? (now captured at #15124)

Coverage against source is broken

  1. aptos move coverage source --module red_black_map --dev --move-2 is broken (but displaying against bytecode works)
@alnoki alnoki added the bug Something isn't working label Oct 30, 2024
@alnoki
Copy link
Contributor Author

alnoki commented Oct 30, 2024

Per https://x.com/wgrieskamp/status/1851647160040948098, I gathered that this requires a --move-2 flag

I just verified that coverage does indeed work with aptos move test --coverage --dev --move-2, so thanks for the quick fix - but is there anyway the CLI could do automatic inference to prevent the requirement of passing --move-2?

Note too that coverage against source is also broken, I've updated the issue description/title accordingly

@alnoki alnoki changed the title [Bug][compiler-v2] enum type and is expression break the coverage tool [Bug][compiler-v2] enum type and is expression break the coverage tool for default cause Oct 30, 2024
@alnoki alnoki changed the title [Bug][compiler-v2] enum type and is expression break the coverage tool for default cause [Bug][compiler-v2] Coverage testing DevEx issues Oct 30, 2024
@vineethk
Copy link
Contributor

vineethk commented Oct 30, 2024

@alnoki

Could you say in what way is the aptos move coverage source --module .. --move-2 is broken? Did you also generate the coverage information using aptos move test --coverage --move-2?

For some simple examples using enums, using --move-2 for running the tests with coverage, as well as for the source coverage display seems to work.

@alnoki
Copy link
Contributor Author

alnoki commented Oct 30, 2024

Could you say in what way is the aptos move coverage source --module .. --move-2 is broken?

@vineethk here's shell output for reproducing

(base) ~ % cd repos/econia-v5/src/move/research/red-black-map/variants/b   
(base) b % aptos move test --coverage --move-2 --dev                         
INCLUDING DEPENDENCY AptosFramework
INCLUDING DEPENDENCY AptosStdlib
INCLUDING DEPENDENCY MoveStdlib
BUILDING RedBlackMap
warning: unused alias
  ┌─ /Users/alex/repos/econia-v5/src/move/research/red-black-map/variants/b/sources/red_black_map.move:4:14
  │
4 │     use std::debug;
  │              ^^^^^ Unused 'use' of alias 'debug'. Consider removing it

Running Move unit tests
[ PASS    ] 0xabc::red_black_map::test_bulk_insertions
[ PASS    ] 0xabc::red_black_map::test_search_insert_cases
Test result: OK. Total tests: 2; passed: 2; failed: 0
warning: unused alias
  ┌─ /Users/alex/repos/econia-v5/src/move/research/red-black-map/variants/b/sources/red_black_map.move:4:14
  │
4 │     use std::debug;
  │              ^^^^^ Unused 'use' of alias 'debug'. Consider removing it

warning: unused alias
  ┌─ /Users/alex/repos/econia-v5/src/move/research/red-black-map/variants/b/sources/red_black_map.move:5:14
  │
5 │     use std::vector;
  │              ^^^^^^ Unused 'use' of alias 'vector'. Consider removing it

+-------------------------+
| Move Coverage Summary   |
+-------------------------+
Module 0000000000000000000000000000000000000000000000000000000000000abc::red_black_map
>>> % Module coverage: 96.86
+-------------------------+
| % Move Coverage: 96.86  |
+-------------------------+
Please use `aptos move coverage -h` for more detailed source or bytecode test coverage of this package
{
  "Result": "Success"
}
(base) b % aptos move coverage source --module red_black_map --dev --move-2  
warning: unused alias
  ┌─ /Users/alex/repos/econia-v5/src/move/research/red-black-map/variants/b/sources/red_black_map.move:4:14
  │
4 │     use std::debug;
  │              ^^^^^ Unused 'use' of alias 'debug'. Consider removing it

warning: unused alias
  ┌─ /Users/alex/repos/econia-v5/src/move/research/red-black-map/variants/b/sources/red_black_map.move:5:14
  │
5 │     use std::vector;
  │              ^^^^^^ Unused 'use' of alias 'vector'. Consider removing it

thread 'main' panicked at third_party/move/tools/move-coverage/src/source_coverage.rs:333:69:
attempt to subtract with overflow
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@vineethk
Copy link
Contributor

Summary:

Using --move-2 should generally work with aptos move test --coverage and aptos move coverage source --module <module_name> when involving code with Move 2 features.

But there are 2 related issues brought up in the discussions above:

  1. A feature request for making it easier to work with Move 2 programs, without having to explicitly pass --move-2 everywhere. A possibility here is to use Move.toml to specify the use of Move 2, which various Move tools can use directly.
  2. Move coverage tool panics in the scenario given here: [Bug][compiler-v2] Coverage testing DevEx issues #15117 (comment), which is bug.

@alnoki
Copy link
Contributor Author

alnoki commented Oct 30, 2024

@vineethk

  1. A feature request for making it easier to work with Move 2 programs, without having to explicitly pass --move-2 everywhere. A possibility here is to use Move.toml to specify the use of Move 2, which various Move tools can use directly.

Captured at #15124

Move coverage tool panics in the scenario given here: #15117 (comment), which is bug.

Should this issue suffice for tracking this?

@vineethk
Copy link
Contributor

Should this issue suffice for tracking this?

Yes, thank you.

@brmataptos brmataptos added the stale-exempt Prevents issues from being automatically marked and closed as stale label Oct 31, 2024
@rahxephon89 rahxephon89 moved this from 🆕 New to Assigned in Move Language and Runtime Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler-v2 stale-exempt Prevents issues from being automatically marked and closed as stale
Projects
Status: Assigned
Development

No branches or pull requests

4 participants