-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[cli] Improve span merging in the coverage tool #15120
[cli] Improve span merging in the coverage tool #15120
Conversation
⏱️ 15m total CI duration on this PR
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments, will approve once fixed.
f0dc645
to
edc960b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the comments!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok, though the coverage problems are deeper than this.
- Improve merging spans in the coverage tool: - The old solution failed to merge neighbouring spans e.g. `[3, 5]` and `[5, 7]` into `[3, 7]` - Also, make the `merge_spans()` function public so it could be used by the `move-mutation-test` tool which also parses the coverage output file.
7ae1151
to
41c7a06
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
We recently merged a PR in aptos-core to improve their `merge_span` function, and now we can use their implememntation: aptos-labs/aptos-core#15120
We recently merged a PR in aptos-core to improve their `merge_span` function, and now we can use their implememntation: aptos-labs/aptos-core#15120
We recently merged a PR in aptos-core to improve their `merge_span` function, and now we can use their implememntation: aptos-labs/aptos-core#15120
We recently merged a PR in aptos-core to improve their `merge_span` function, and now we can use their implememntation: aptos-labs/aptos-core#15120
Description
[3, 5]
and[5, 7]
into[3, 7]
merge_spans
function public so it could be used by themove-mutation-test
tool that also parses the coverage output file.How Has This Been Tested?
cargo clippy
hasn't introduced any new warnings for the new change.aptos move coverage
tool locally to ensure it still works as expected.Key Areas to Review
This should have no visible impact on current
aptos move coverage
tool. The old limitation was not an issue, since displaying covered lines with always worked fine although spans were not merged "perfectly". This improvement is much more important for the usage in themove-mutation-test
tool.Type of Change
Which Components or Systems Does This Change Impact?
Checklist