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

Finish adapting unit tests to new API #430

Merged
merged 29 commits into from
Feb 17, 2023
Merged

Conversation

plafer
Copy link
Contributor

@plafer plafer commented Feb 14, 2023

Closes: #413


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 14, 2023

Codecov Report

Base: 61.12% // Head: 60.04% // Decreases project coverage by -1.09% ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #430      +/-   ##
==========================================
- Coverage   61.12%   60.04%   -1.09%     
==========================================
  Files         131      131              
  Lines       18114    18777     +663     
==========================================
+ Hits        11073    11275     +202     
- Misses       7041     7502     +461     
Impacted Files Coverage Δ
crates/ibc/src/applications/transfer/context.rs 34.96% <0.00%> (-0.45%) ⬇️
crates/ibc/src/applications/transfer/error.rs 0.00% <0.00%> (ø)
...ibc/src/core/ics02_client/handler/create_client.rs 50.46% <ø> (-32.87%) ⬇️
...ibc/src/core/ics02_client/handler/update_client.rs 51.61% <ø> (-33.65%) ⬇️
...ore/ics04_channel/handler/write_acknowledgement.rs 0.00% <ø> (-83.64%) ⬇️
crates/ibc/src/mock/ics18_relayer/context.rs 65.00% <ø> (ø)
.../ibc/src/core/ics04_channel/handler/send_packet.rs 78.40% <33.33%> (-3.52%) ⬇️
...c/core/ics04_channel/handler/chan_close_confirm.rs 50.69% <75.00%> (-37.04%) ⬇️
.../src/core/ics04_channel/handler/chan_close_init.rs 50.80% <75.00%> (-28.54%) ⬇️
crates/ibc/src/test_utils.rs 67.50% <76.47%> (+0.19%) ⬆️
... and 46 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 17, 2023 17:07
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.

Thank you so much for this brave job! 🙌

I just noticed the assertion panic messages should to be adjusted for failure cases, I pointed this out in some places, but I’d appreciate it if you go back and take a look at them all.
Also, there are parts and codes that can be shrunk and optimized later. I was thinking we might set “testing improvements” as one of the main objectives for next quarters. WDYT?

@plafer
Copy link
Contributor Author

plafer commented Feb 17, 2023

Also, there are parts and codes that can be shrunk and optimized later. I was thinking we might set “testing improvements” as one of the main objectives for next quarters. WDYT?

Definitely.

I just noticed the assertion panic messages should to be adjusted for failure cases, I pointed this out in some places, but I’d appreciate it if you go back and take a look at them all.

There are many things to improve with our tests, and since we're about to do an overhaul, I don't see to value in having "perfect" error messages on imperfect tests. Instead of "fixing" something that will be removed and greatly improved in < 3 months, I would just leave as is for now an lump that in the #429 work.

@Farhad-Shabani
Copy link
Member

There are many things to improve with our tests, and since we're about to do an overhaul, I don't see to value in having "perfect" error messages on imperfect tests. Instead of "fixing" something that will be removed and greatly improved in < 3 months, I would just leave as is for now an lump that in the #429 work.

That's fine with me. I just wanted to mention that the error messages are actually written to reflect successful scenarios, so if an assertion fails, the message will be displayed accordingly wrong. We should keep this in mind until then if a test fails during our development works.

@plafer plafer merged commit ea9a832 into main Feb 17, 2023
@plafer plafer deleted the plafer/413-mockcontext-finish branch February 17, 2023 20:32
Farhad-Shabani pushed a commit that referenced this pull request Sep 9, 2024
* remove redundant msg in test

* adapt timeout test

* clippy

* timeout no-op case

* ack validation tests

* ack tests

* recv_packet validation

* recv_packet execute test

* chan_open_init_validation

* chan_open_init tests done

* `chan_open_try` validate tests

* chan_open_try tests

* chan_open_ack tests

* chan_open_confirm tests

* chan_close_init tests

* chan_close_confirm tests

* create_client tests

* update_client tests

* misbehaviour tests

* upgrade client tests

* Remove Readers and Keepers from MockContext

* remove write_ack tests

they are now covered in `recv_packet` tests

* adapt send_packet

* changelog

* fix validation msgs

* fmt
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.

Convert all test code to test the Validation/ExecutionContext API
2 participants