Skip to content

[ICS24] Discard Default impl for ClientId and move away from tm_client_type in unit tests #552

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

Closed
Tracked by #965
Farhad-Shabani opened this issue Mar 17, 2023 · 2 comments · Fixed by #1099
Closed
Tracked by #965
Labels
O: reliability Objective: cause to improve trustworthiness and consistent performing O: testing Objective: aims to improve testing coverage
Milestone

Comments

@Farhad-Shabani
Copy link
Member

Farhad-Shabani commented Mar 17, 2023

Problem Statement

Part of #373

Since ClientId serves a wider range of chains, it does not make sense to have a default value. As such, we should also get our unit tests client-indifference and discard tm_client_type calls

@Farhad-Shabani Farhad-Shabani added O: testing Objective: aims to improve testing coverage O: reliability Objective: cause to improve trustworthiness and consistent performing labels Mar 17, 2023
@Farhad-Shabani Farhad-Shabani moved this to 📥 To Do in ibc-rs Mar 17, 2023
@plafer
Copy link
Contributor

plafer commented Mar 17, 2023

Note that our current unit tests have no proof verifications, so even having different client names wouldn't catch bugs like #551. For now, I made sure that our integration tests use different client names.

still a good improvement to have though in the long term.

@Farhad-Shabani
Copy link
Member Author

Farhad-Shabani commented Mar 17, 2023

Definitely. This one caught my eye again while fixing tests for #531.
Unit tests mainly use the MockCotext but having tm_client_type imported, we circumvent the mock in some places

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O: reliability Objective: cause to improve trustworthiness and consistent performing O: testing Objective: aims to improve testing coverage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants