fix(native_blockifier): activate testing feature only in dev dependencies #2624
Conversation
|
This change is |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2624 +/- ##
===========================================
+ Coverage 40.10% 60.44% +20.33%
===========================================
Files 26 153 +127
Lines 1895 19579 +17684
Branches 1895 19579 +17684
===========================================
+ Hits 760 11834 +11074
- Misses 1100 6992 +5892
- Partials 35 753 +718 ☔ View full report in Codecov by Sentry. |
dorimedini-starkware
force-pushed
the
12-10-fix_native_blockifier_activate_testing_feature_only_in_dev_dependencies
branch
4 times, most recently
from
06da31a to
4acec9b
Compare
dorimedini-starkware
force-pushed
the
12-10-fix_native_blockifier_activate_testing_feature_only_in_dev_dependencies
branch
from
4acec9b to
96c2043
Compare
There was a problem hiding this comment.
Reviewed 3 of 5 files at r1, 3 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware, @noaov1, and @Yoni-Starkware)
crates/blockifier/src/test_utils.rs line 152 at r3 (raw file):
NonzeroGasPrice::new_unchecked(GasPrice(100 * u128::pow(10, 9))); // Given in units of Wei. pub const DEFAULT_STRK_L1_GAS_PRICE: NonzeroGasPrice = NonzeroGasPrice::new_unchecked(GasPrice(100 * u128::pow(10, 9))); // Given in units of Fri.
Is this related to this PR?
Code quote:
NonzeroGasPrice::new_unchecked(GasPrice(100 * u128::pow(10, 9))); // Given in units of Fri.crates/native_blockifier/src/py_state_diff.rs line 25 at r3 (raw file):
const DEFAULT_STRK_L1_GAS_PRICE: GasPrice = GasPrice(100 * u128::pow(10, 9)); const DEFAULT_ETH_L1_DATA_GAS_PRICE: GasPrice = GasPrice(u128::pow(10, 6)); const DEFAULT_STRK_L1_DATA_GAS_PRICE: GasPrice = GasPrice(u128::pow(10, 9));
Also not part of this PR, right?
Code quote:
const DEFAULT_ETH_L1_GAS_PRICE: GasPrice = GasPrice(100 * u128::pow(10, 9));
const DEFAULT_STRK_L1_GAS_PRICE: GasPrice = GasPrice(100 * u128::pow(10, 9));
const DEFAULT_ETH_L1_DATA_GAS_PRICE: GasPrice = GasPrice(u128::pow(10, 6));
const DEFAULT_STRK_L1_DATA_GAS_PRICE: GasPrice = GasPrice(u128::pow(10, 9));crates/native_blockifier/Cargo.toml line 47 at r3 (raw file):
[dev-dependencies] blockifier = { workspace = true, features = ["cairo_native", "native_blockifier", "testing"] }
Why do we need the native blockifier config?
Code quote:
"native_blockifier
There was a problem hiding this comment.
Reviewed 3 of 5 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)
crates/native_blockifier/Cargo.toml line 14 at r2 (raw file):
# https://pyo3.rs/v0.19.1/faq#i-cant-run-cargo-test-or-i-cant-build-in-a-cargo-workspace-im-having-linker-issues-like-symbol-not-found-or-undefined-reference-to-_pyexc_systemerror extension-module = ["pyo3/extension-module"] testing = ["blockifier/testing", "papyrus_storage/testing", "starknet_api/testing"]
Why is this feature needed?
Code quote:
testing = ["blockifier/testing", "papyrus_storage/testing", "starknet_api/testing"]crates/native_blockifier/Cargo.toml line 47 at r2 (raw file):
[dev-dependencies] blockifier = { workspace = true, features = ["cairo_native", "native_blockifier", "testing"] }
Is the "cairo_native" feature needed?
Code quote:
blockifier = { workspace = true, features = ["cairo_native", "native_blockifier", "testing"] }There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @Yoni-Starkware)
crates/blockifier/src/test_utils.rs line 152 at r3 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
Is this related to this PR?
not really, but removing the dependency on these constants is part of this PR, so I'm in favor of keeping it (it's just a doc change)
crates/native_blockifier/Cargo.toml line 14 at r2 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Why is this feature needed?
note that this PR only fixes the testing feature:
previously the testing feature in these 3 crates was always activated, even in regular cargo build.
now, it's only activated in dev-dependencies (cargo test) and when the testing feature is activated
crates/native_blockifier/Cargo.toml line 47 at r2 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Is the "cairo_native" feature needed?
remove in the next PR
crates/native_blockifier/Cargo.toml line 47 at r3 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
Why do we need the native blockifier config?
you mean feature?
because TransactionExecutorConfig::create_for_testing is needed in python tests.
I don't think it's wise to expose all blockifier test utils to the native blockifier, but this specific test util is needed in python so I added another feature to turn it on specifically.
crates/native_blockifier/src/py_state_diff.rs line 25 at r3 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
Also not part of this PR, right?
actually, this is part of this PR.
the constants were originally in the blockifier test utils,
but we want to be able to build the native_blockifier business logic without the testing feature of the blockifier,
so we redefine these constants for the native blockifier
There was a problem hiding this comment.
Reviewable status:
No description provided.