ibc-rs icon indicating copy to clipboard operation
ibc-rs copied to clipboard

imp(tests): refactor table driven tests

Open rnbguy opened this issue 1 year ago • 6 comments

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 using rstest.

https://github.com/cosmos/ibc-rs/blob/1aa853bb646d37aab3848096b4221450f3a2e760/crates/ibc/src/applications/transfer/coin.rs#L144-L159

rnbguy avatar Sep 26 '23 15:09 rnbguy

hi, can i work on this

kien6034 avatar Apr 01 '24 09:04 kien6034

@kien6034 thanks for your interest! but this requires significant changes, that

  • requires decisions from maintainers
  • may conflict with the testkit rework branch (#1109).

rnbguy avatar Apr 02 '24 10:04 rnbguy

Hey has this issue un-blocked after #1109 @rnbguy

TropicalDog17 avatar Jul 23 '24 02:07 TropicalDog17

hey @TropicalDog17, thanks for taking interest. This requires some thinking from our side. But you can work on the straight-forward cases.

rnbguy avatar Jul 23 '24 09:07 rnbguy

@rnbguy can I rewrite the tests on ibc-testkit crate?

TropicalDog17 avatar Jul 28 '24 04:07 TropicalDog17

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();
}

rnbguy avatar Jul 29 '24 11:07 rnbguy