imp(tests): refactor table driven tests

[rnbguy]

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.

ibc-rs/crates/ibc/src/applications/transfer/coin.rs

Lines 125 to 148 in c83f26a

	fn test_parse_raw_coin() -> Result<(), TokenTransferError> {
	{
	let coin = RawCoin::from_str("123stake")?;
	assert_eq!(coin.denom, "stake");
	assert_eq!(coin.amount, 123u64.into());
	}
	{
	let coin = RawCoin::from_str("1a1")?;
	assert_eq!(coin.denom, "a1");
	assert_eq!(coin.amount, 1u64.into());
	}
	{
	let coin = RawCoin::from_str("0x1/:.\\_-")?;
	assert_eq!(coin.denom, "x1/:.\\_-");
	assert_eq!(coin.amount, 0u64.into());
	}
	{
	// `!` is not allowed
	let res = RawCoin::from_str("0x!");
	assert!(res.is_err());
	}

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.

ibc-rs/crates/ibc/src/applications/transfer/coin.rs

Lines 144 to 159 in 1aa853b

	#[rstest]
	#[case::nat("123stake", RawCoin::new(123, "stake"))]
	#[case::zero("0stake", RawCoin::new(0, "stake"))]
	#[case::u256_max(
	"115792089237316195423570985008687907853269984665640564039457584007913129639935stake",
	RawCoin::new(
	[u64::MAX; 4],
	"stake"
	)
	)]
	#[case::digit_in_denom("1a1", RawCoin::new(1, "a1"))]
	#[case::chars_in_denom("0x1/:._-", RawCoin::new(0, "x1/:._-"))]
	#[case::ibc_denom("1234ibc/a0B1C", RawCoin::new(1234, "ibc/a0B1C"))]
	fn test_parse_raw_coin(#[case] parsed: RawCoin, #[case] expected: RawCoin) {
	assert_eq!(parsed, expected);
	}

[kien6034]

hi, can i work on this

[rnbguy]

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

• requires decisions from maintainers
• may conflict with the testkit rework branch (imp(ibc-testkit): revamp mock testing framework #1109).

[TropicalDog17]

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

[rnbguy]

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

[TropicalDog17]

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

[rnbguy]

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