-
Notifications
You must be signed in to change notification settings - Fork 100
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
imp(tests): refactor table driven tests #881
Comments
hi, can i work on this |
@kien6034 thanks for your interest! but this requires significant changes, that
|
hey @TropicalDog17, thanks for taking interest. This requires some thinking from our side. But you can work on the straight-forward cases. |
@rnbguy can I rewrite the tests on |
Don't try to refactor the complicated tests with fixtures. If you come across some tests which look like the following, you may refactor. #[test]
fn test_my_function() {
my_function("hello world").expect("no error");
my_function("hello mars").expect_err("error");
} #[rstest]
#[case("hello world")]
#[should_panic]
#[case("hello mars)]
fn test_my_function_with_rstest(value: &str) {
my_function(value).unwrap();
} |
Improvement Summary
Currently, some of the rust tests contain a group of table/data-driven tests. These tests will fail if one in the group fails.
https://github.com/cosmos/ibc-rs/blob/c83f26aeb87004ca464917e250da62ba3e92238b/crates/ibc/src/applications/transfer/coin.rs#L125-L148
We want to treat each testcases individually. If one fails, the others in the group may continue.
Proposal
The current Rust release doesn't support this feature. The common opinion in the Rust community is to keep each testcases in a separate test method, potentially using a macro.
We don't want to implement/maintain our macro. So we will use
rstest
as a solution. The following is a refactored code of the above example usingrstest
.https://github.com/cosmos/ibc-rs/blob/1aa853bb646d37aab3848096b4221450f3a2e760/crates/ibc/src/applications/transfer/coin.rs#L144-L159
The text was updated successfully, but these errors were encountered: