Skip to content
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

Prepare to move test code to Validation/ExecutionContext API #417

Merged
merged 24 commits into from
Feb 14, 2023

Conversation

plafer
Copy link
Contributor

@plafer plafer commented Feb 10, 2023

Work towards: #413
Work towards: #426

Description

  • Demonstrates how test code is refactored for the timeout_on_close handler only
    • New tests are written using the rstest crate
  • Sets up next PR by moving all "ICS-26 router" functions to their own submodule to facilitate unit testing

Ultimate goal (finished in next PR) is to remove old Reader/Keeper context implementations from MockContext, which requires updating all tests to test validate() and execute() functions as opposed to process().


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests.
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Base: 55.93% // Head: 62.21% // Increases project coverage by +6.28% 🎉

Coverage data is based on head (d182a79) compared to base (d5a4416).
Patch coverage: 85.05% of modified lines in pull request are covered.

❗ Current head d182a79 differs from pull request most recent head 5c602f1. Consider uploading reports for the commit 5c602f1 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #417      +/-   ##
==========================================
+ Coverage   55.93%   62.21%   +6.28%     
==========================================
  Files         122      131       +9     
  Lines       17093    17789     +696     
==========================================
+ Hits         9561    11068    +1507     
+ Misses       7532     6721     -811     
Impacted Files Coverage Δ
crates/ibc/src/core/context.rs 80.39% <ø> (+76.49%) ⬆️
crates/ibc/src/core/ics26_routing/handler.rs 0.00% <ø> (-91.12%) ⬇️
crates/ibc/src/mock/ics18_relayer/context.rs 65.00% <ø> (ø)
crates/ibc/src/mock/context.rs 54.63% <71.31%> (-7.47%) ⬇️
crates/ibc/src/core/context/recv_packet.rs 80.23% <80.23%> (ø)
crates/ibc/src/core/context/acknowledgement.rs 90.47% <90.47%> (ø)
crates/ibc/src/core/context/chan_open_ack.rs 92.72% <92.72%> (ø)
crates/ibc/src/core/context/chan_open_confirm.rs 92.98% <92.98%> (ø)
crates/ibc/src/core/context/chan_close_confirm.rs 93.10% <93.10%> (ø)
crates/ibc/src/core/context/chan_close_init.rs 93.22% <93.22%> (ø)
... and 36 more

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@plafer plafer marked this pull request as ready for review February 13, 2023 19:46
@plafer plafer changed the title Move test code to Validation/ExecutionContext API Prepare to move test code to Validation/ExecutionContext API Feb 13, 2023
Copy link
Member

@Farhad-Shabani Farhad-Shabani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@@ -668,6 +679,7 @@ impl RouterBuilder for MockRouterBuilder {
}
}

// TODO: REMOVE BEFORE MERGING
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any action needed for this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, that will be removed in next PR now. Removed comment: 5c602f1

crates/ibc/src/core/context.rs Show resolved Hide resolved
/// messages to the correct module handler, and more importantly: (2) the ability of ICS handlers
/// to work with the context and correctly store results (i.e., the `ClientKeeper`,
/// `ConnectionKeeper`, and `ChannelKeeper` traits).
fn routing_module_and_keepers() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we gonna also use rstest fixtures here later?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a better way to write it using rstest yes; not sure exactly which feature to use. I'll open an issue for it.

@plafer plafer merged commit 0f3e03f into main Feb 14, 2023
@plafer plafer deleted the plafer/413-test-code branch February 14, 2023 17:04
Farhad-Shabani pushed a commit that referenced this pull request Sep 9, 2024
* impl ExecutionContext for MockContext

* Make MockContext::ValidationContext independent

* Change relayer test

* re-implement `host_timestamp` in  `MockContext`

* ics26_routing test changed

* fix upgrade_client feature

* move ics26_routing dispatch tests

* clippy

* timeout_on_close rstest for validation

* move timeout routing helpers in mod

* timeout_on_close_execute test

* timeout execute tests

* Remove old processing test

* fix test

* move acknowledgement

* move recv_packet

* move chan_close_confirm

* move chan_close_init

* move chan_open_confirm

* move chan_open_ack

* move chan_open_try

* move chan_open_init

* remove comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants