-
Notifications
You must be signed in to change notification settings - Fork 80
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
Refactor connection handler unit tests to adapt with new API #441
Conversation
Codecov ReportBase: 62.21% // Head: 61.14% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #441 +/- ##
==========================================
- Coverage 62.21% 61.14% -1.08%
==========================================
Files 131 131
Lines 17789 18105 +316
==========================================
+ Hits 11068 11070 +2
- Misses 6721 7035 +314
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@@ -246,8 +244,7 @@ mod tests { | |||
|
|||
let context = MockContext::default(); | |||
|
|||
let msg_conn_init = | |||
MsgConnectionOpenInit::try_from(get_dummy_raw_msg_conn_open_init(None)).unwrap(); | |||
let msg_conn_init = MsgConnectionOpenInit::new_dummy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove all changes in the ics04_channel
module? This will cause a merge conflict with #430
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There still would be conflict, because of the get_dummy_raw_msg_conn_open_*()
changes and it does not take None
anymore, How if we merge this PR after #430?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, in that case let's just merge as is. We need to merge this first because the last step of #430 is to remove impl Readers/Keepers for MockContext
, which needs this. They won't be big conflicts anyways
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for these high-quality tests! We can use them as a baseline when we implement #429
* Refactor connection handler unit tests * Move generate_error_msg under Fixture struct
Closes: #440
Remark
core::ics03_connection::handler
. If we decide to apply such a writing test approach for the rest of the parts, it should be moved to a higher level insidecore::handler
Option<ContextError>
type for theExpect::Failure
should beContextError
to know what error exactly we catch in a failure scenario. But here we aimed to fulfill the refactoring purpose and this (along with other tests) should be revised with the next improvement iterationPR author checklist:
unclog
.docs/
).Reviewer checklist:
Files changed
in the GitHub PR explorer.