-
Notifications
You must be signed in to change notification settings - Fork 25
feat: add statement coverage instrumentation #879
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
base: feat/code-coverage
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 42ac089 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to 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.
Pull Request Overview
This PR introduces statement coverage instrumentation to the project by adding a new API and integrating it into the edr_napi and edr_instrument crates. The changes include updating dependency declarations across Cargo.toml files to use workspace versions, adding the edr_instrument crate with coverage functionality, and exposing the instrumentation API via edr_napi with accompanying tests.
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
crates/edr_solidity/Cargo.toml | Updated dependency declarations to use workspace versions. |
crates/edr_scenarios/Cargo.toml | Aligned serde dependency with workspace settings. |
crates/edr_rpc_eth/Cargo.toml | Transitioned serde and thiserror to workspace configuration. |
crates/edr_rpc_client/Cargo.toml | Updated dependency declarations with workspace versions. |
crates/edr_provider/Cargo.toml | Adjusted dependency versions to use workspace settings. |
crates/edr_op/Cargo.toml | Synced dependency declarations with workspace configurations. |
crates/edr_napi_core/Cargo.toml | Updated serde, serde_json, and thiserror to workspace usage. |
crates/edr_napi/test/instrument.ts | Added tests validating the instrumentation output. |
crates/edr_napi/src/lib.rs | Exposed the new instrument module via the public API. |
crates/edr_napi/src/instrument.rs | Implemented the wrapper and conversion for instrumentation results. |
crates/edr_napi/index.js | Exported the instrumentation API for use in JavaScript. |
crates/edr_napi/Cargo.toml | Updated dependency declarations to align with workspace settings. |
crates/edr_instrument/src/lib.rs | Added the instrument module for code coverage instrumentation. |
crates/edr_instrument/src/coverage.rs | Implemented the instrumentation logic and tests for deterministic hashing. |
crates/edr_instrument/Cargo.toml | Configured edr_instrument with workspace dependencies. |
crates/edr_generic/Cargo.toml | Updated dependencies to align with the workspace configuration. |
crates/edr_evm/Cargo.toml | Transitioned serde and thiserror to workspace versions. |
crates/edr_eth/Cargo.toml | Updated multiple dependencies to workspace-based versions. |
Cargo.toml | Added new members and top-level dependency overrides for workspace. |
.changeset/lemon-pumas-walk.md | Documented the new instrumentation feature in the changeset. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/code-coverage #879 +/- ##
=====================================================
Coverage ? 54.41%
=====================================================
Files ? 250
Lines ? 29864
Branches ? 29864
=====================================================
Hits ? 16251
Misses ? 12683
Partials ? 930 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 LGTM with some comments, but none of them blockers.
content_hash: &B256, | ||
statement_counter: u64, | ||
) -> B256 { | ||
let mut hasher = Keccak256::new(); |
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.
I'm curious about the choice of the hash function. Is the purpose just to generate unique ids deterministically? If yes, we could consider using Blake3 instead of Keccak, as it's an order of magnitude faster: https://github.com/BLAKE3-team/BLAKE3?tab=readme-ov-file#blake3
}) | ||
|
||
// serde_json::to_string_pretty(&metadata) | ||
//.context("Failed to serialize metadata")? |
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.
Leftover?
@@ -0,0 +1,2 @@ | |||
/// Types for instrumenting code for the purpose of code coverage. | |||
pub mod coverage; |
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.
Could potentially live in edr_solidity
This PR adds an API for instrumenting statement code coverage, based on this research spike.
Should be merged after #866!