search

cosmos / interchain-security

Replicated security (aka interchain security V1) is an open sourced IBC application which allows cosmos blockchains to lease their proof-of-stake security to one another.
https://cosmos.github.io/interchain-security/
Other
155 stars 115 forks source link

Rewrite some of legacy integration tests #629

Open shaspitz opened 1 year ago

shaspitz commented 1 year ago

EDIT: e2e tests were renamed to integration tests

Problem

A lot of our current integration tests were migrated over in PRs similar to https://github.com/cosmos/interchain-security/pull/297, which tried to clean up testing conventions without changing functionality. The original tests were written in various non-standardized ways, in various files, and may be testing outdated functionality. Some of these testing pitfalls are still present in main, and we need to fix this where applicable.

Closing criteria

All tests (legacy integration tests especially), are relevant to the current implementation of the ccv protocol, and have clear intention of what functionality is actually being tested.

Problem details

Here is an example of an integration test in which the functionality being tested is outdated? @mpoke created a more up-to-date test that fails. See https://github.com/cosmos/interchain-security/commit/5928146642857ff4bfc73db115c00ededcf61928

shaspitz commented 1 year ago

Regarding lessons for the future:

Standardizing test conventions, intention, organization, etc. should be the first thing we prioritize for a new repo! Test standardization should not be an afterthought that is prioritized half way through implementing a protocol.

shaspitz commented 11 months ago

My two cents on this issue -> someone with a deep understanding of the protocol would need to audit what we currently call integration tests. If the auditor cannot decipher a clear intention of what useful/relevant functionality is being tested, in any individual test, then the test should be deleted.