refactor(tests): Improve test structure and implement lowering test #6915
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @Odomey)
tests/examples_test.rs line 42 at r1 (raw file):
} #[rstest]
you misunderstood the meaning of this test.
remove all changes related to it.
you may possibly leave the changes regarding the test config, and merge the two test-cases runners.
Code quote:
#[rstest]tests/examples_test.rs line 50 at r1 (raw file):
/// Expected gas cost expected_cost: Option<usize>, }
consider adding specific constructors that are repeated between test cases.
Code quote:
/// Whether to automatically add gas withdrawal
auto_add_withdraw_gas: bool,
/// Available gas for the function
available_gas: Option<usize>,
/// Expected gas cost
expected_cost: Option<usize>,
}
|
@orizi Updated. Thanks for the answer |
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @Odomey)
tests/examples_test.rs line 42 at r1 (raw file):
Previously, orizi wrote…
you misunderstood the meaning of this test.
remove all changes related to it.you may possibly leave the changes regarding the test config, and merge the two test-cases runners.
you haven't reverted the change of the lowering test.
please review changes at https://reviewable.io/reviews/starkware-libs/cairo/6915#-
tests/examples_test.rs line 379 at r2 (raw file):
) { run_test_function(name, params, config, expected_result, example_dir_data); }
Suggestion:
#[case::fib_pass(
"fib",
&[1, 1, 10].map(Felt252::from),
TestConfig::with_gas(200000, None),
RunResultValue::Success([89].map(Felt252::from).into_iter().collect())
)]
#[case::fib_fail(
"fib",
&[1, 1, 10].map(Felt252::from),
TestConfig::with_gas(10000, None),
RunResultValue::Panic(vec![Felt252::from_bytes_be_slice(b"Out of gas")])
)]
fn run_function_with_gas_test(
#[case] name: &str,
#[case] params: &[Felt252],
#[case] config: TestConfig,
#[case] expected_result: RunResultValue,
example_dir_data: &ExampleDirData,
) {
// inline and remove the called function.
}
|
@orizi updated |
There was a problem hiding this comment.
Please answer in the reviewable threads.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @Odomey)
|
Previously, orizi wrote…
/// Configuration for running test functions struct TestConfig { /// Whether to automatically add gas withdrawal auto_add_withdraw_gas: bool, /// Available gas for the function available_gas: Option<usize>, /// Expected gas cost expected_cost: Option<usize>, } impl TestConfig { /// Creates a new configuration for tests without gas fn without_gas() -> Self { Self { auto_add_withdraw_gas: false, available_gas: None, expected_cost: None, } } /// Creates a new configuration for tests with gas fn with_gas(available_gas: usize, expected_cost: Option<usize>) -> Self { Self { auto_add_withdraw_gas: true, available_gas: Some(available_gas), expected_cost, } } } // ... (остальной код остается без изменений до тестовых функций) #[rstest] #[case::fib("fib", &[1, 1, 7].map(Felt252::from), TestConfig::without_gas(), RunResultValue::Success(vec![Felt252::from(21)]))] // ... (остальные тест-кейсы без газа) #[case::hash_chain_gas("hash_chain_gas", &[3].map(Felt252::from), TestConfig::with_gas(100000, Some(9880 + 3 * token_gas_cost(CostTokenType::Pedersen))), RunResultValue::Success(vec![Felt252::from_hex_unchecked( "2dca1ad81a6107a9ef68c69f791bcdbda1df257aab76bd43ded73d96ed6227d")]))] #[case::fib_pass("fib", &[1, 1, 10].map(Felt252::from), TestConfig::with_gas(200000, None), RunResultValue::Success([89].map(Felt252::from).into_iter().collect()))] #[case::fib_fail("fib", &[1, 1, 10].map(Felt252::from), TestConfig::with_gas(10000, None), RunResultValue::Panic(vec![Felt252::from_bytes_be_slice(b"Out of gas")]))] fn run_function_test( #[case] name: &str, #[case] params: &[Felt252], #[case] config: TestConfig, #[case] expected_result: RunResultValue, example_dir_data: &ExampleDirData, ) { pretty_assertions::assert_eq!( run_function( name, params, config.available_gas, config.expected_cost, example_dir_data, config.auto_add_withdraw_gas ), expected_result ); } #[rstest] fn lowering_test(example_dir_data: &ExampleDirData) {} This is how i corrected it |
|
Previously, orizi wrote…
Corrected |
|
Done as it seems to me |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Odomey)
tests/examples_test.rs line 42 at r1 (raw file):
Previously, Odomey (Odomey) wrote…
/// Configuration for running test functions
struct TestConfig {
/// Whether to automatically add gas withdrawal
auto_add_withdraw_gas: bool,
/// Available gas for the function
available_gas: Option<usize>,
/// Expected gas cost
expected_cost: Option<usize>,
}
impl TestConfig {
/// Creates a new configuration for tests without gas
fn without_gas() -> Self {
Self {
auto_add_withdraw_gas: false,
available_gas: None,
expected_cost: None,
}
}
/// Creates a new configuration for tests with gas
fn with_gas(available_gas: usize, expected_cost: Option<usize>) -> Self {
Self {
auto_add_withdraw_gas: true,
available_gas: Some(available_gas),
expected_cost,
}
}
}
// ... (остальной код остается без изменений до тестовых функций)
#[rstest]
#[case::fib("fib", &[1, 1, 7].map(Felt252::from), TestConfig::without_gas(),
RunResultValue::Success(vec![Felt252::from(21)]))]
// ... (остальные тест-кейсы без газа)
#[case::hash_chain_gas("hash_chain_gas", &[3].map(Felt252::from),
TestConfig::with_gas(100000, Some(9880 + 3 * token_gas_cost(CostTokenType::Pedersen))),
RunResultValue::Success(vec![Felt252::from_hex_unchecked(
"2dca1ad81a6107a9ef68c69f791bcdbda1df257aab76bd43ded73d96ed6227d")]))]
#[case::fib_pass("fib", &[1, 1, 10].map(Felt252::from),
TestConfig::with_gas(200000, None),
RunResultValue::Success([89].map(Felt252::from).into_iter().collect()))]
#[case::fib_fail("fib", &[1, 1, 10].map(Felt252::from),
TestConfig::with_gas(10000, None),
RunResultValue::Panic(vec![Felt252::from_bytes_be_slice(b"Out of gas")]))]
fn run_function_test(
#[case] name: &str,
#[case] params: &[Felt252],
#[case] config: TestConfig,
#[case] expected_result: RunResultValue,
example_dir_data: &ExampleDirData,
) {
pretty_assertions::assert_eq!(
run_function(
name,
params,
config.available_gas,
config.expected_cost,
example_dir_data,
config.auto_add_withdraw_gas
),
expected_result
);
}
#[rstest]
fn lowering_test(example_dir_data: &ExampleDirData) {}
This is how i corrected it
you just move it to the bottom.
move it back up please to remove this snippet from being touched in this unrelated PR.
|
Previously, orizi wrote…
Done |
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @Odomey)
tests/examples_test.rs line 252 at r4 (raw file):
#[rstest] #[case::fib("fib", &[1, 1, 7].map(Felt252::from), TestConfig::without_gas(),
please make sure to not reorder the tests (as it makes it impossible to ascertain non were dropped).
additionally - prevent lines from exceeding 100 chars (as was before)
|
Previously, orizi wrote…
Done |
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @Odomey)
tests/examples_test.rs line 230 at r5 (raw file):
#[case::fib( "fib_loop", &[1, 1, 7].map(Felt252::from), None, None,
this test seem to have dropped.
Code quote:
#[case::fib(
"fib_loop",
&[1, 1, 7].map(Felt252::from), None, None,tests/examples_test.rs line 362 at r5 (raw file):
&[1, 1, 7].map(Felt252::from), TestConfig::without_gas(), RunResultValue::Success(vec![Felt252::from(21)])
these does not look the proper args for these tests.
Code quote:
#[case::pedersen_test(
"pedersen_test",
&[1, 1, 7].map(Felt252::from),
TestConfig::without_gas(),
RunResultValue::Success(vec![Felt252::from(21)])
)]
#[case::match_or(
"match_or",
&[1, 1, 7].map(Felt252::from),
TestConfig::without_gas(),
RunResultValue::Success(vec![Felt252::from(21)])tests/examples_test.rs line 488 at r5 (raw file):
// `r.low` Felt252::from(1 + 2 + 3 + 4 + 6 + 8 + 10 + 12 + 14 + 16 + 18), // `r.high`
this seems very broken.
Code quote:
// `r.low`
Felt252::from(1 + 2 + 3 + 4 + 6 + 8 + 10 + 12 + 14 + 16 + 18),
// `r.high`
|
Previously, orizi wrote…
Resolved |
|
Previously, orizi wrote…
Resolved |
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @Odomey)
tests/examples_test.rs line 488 at r5 (raw file):
Previously, Odomey (Odomey) wrote…
Resolved
no it isnt.
make sure to run:
cargo t --profile=ci-dev -p tests and ./scripts/rust_fmt.sh.
|
Previously, orizi wrote…
Done |
|
Previously, orizi wrote…
Done, as it seems to me(( |
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @Odomey)
tests/examples_test.rs line 323 at r7 (raw file):
TestConfig::without_gas(), RunResultValue::Success(vec![Felt252::from(21)]) )]
put it back in order.
Code quote:
#[case::fib_loop(
"fib_loop",
&[1, 1, 7].map(Felt252::from),
TestConfig::without_gas(),
RunResultValue::Success(vec![Felt252::from(21)])
)]tests/examples_test.rs line 423 at r7 (raw file):
]) ); }
please run again - this does not actually parse.
Code quote:
}
done |
Description
This PR improves the test infrastructure by reducing code duplication and implementing proper lowering test functionality in
examples_test.rs.Changes
Code Duplication Reduction
TestConfigstruct to encapsulate test configuration parameters:auto_add_withdraw_gas: boolavailable_gas: Option<usize>expected_cost: Option<usize>run_test_functionto handle both regular and gas-enabled test casesLowering Test Implementation
lowering_testfunctionality to verify compiler behaviorTesting
Related Issues