interchain-security icon indicating copy to clipboard operation
interchain-security copied to clipboard

Rewrite some of legacy integration tests

Open shaspitz opened this issue 3 years ago • 2 comments

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 avatar Dec 23 '22 19:12 shaspitz

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 avatar Dec 23 '22 19:12 shaspitz

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.

shaspitz avatar Oct 04 '23 07:10 shaspitz