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

Disable MBT tests if the "mocks" feature is not enabled #643

Merged
merged 4 commits into from
Feb 12, 2021

Conversation

vitorenesduarte
Copy link
Contributor

Closes: #642

Description

Before, running cargo test in the root folder would run the MBT tests successfully. However, running cargo test inside the modules/ folder doesn't work because the "mocks" is missing. This hints that running cargo test in the root folder is equivalent to running cargo test --all-features, but I couldn't find the corresponding documentation.

Ideally we would like to always enable the "mocks" feature when running tests, independently from where, but unfortunately this seems to not be supported: rust-lang/cargo#2911
This PR disables the MBT tests if the "mocks" feature is not enabled, which seemed to be the best workaround suggested in the previous issue: rust-lang/cargo#2911 (comment)

Now, running cargo test in the root folder still runs the MBT tests successfully, while running cargo test inside the modules/ folder simply ignores them. To also run the MBT tests inside the modules/ folder one has to:

cargo test --features mocks

This PR also adds more structure to the MBT tests, as suggested by @adizere in #601 (comment). This is a good suggestion since each file in the tests/ folder is compiled as it own crate. As a result, step.rs, modelator.rs and model_based.rs were all compiled individually, even though only the latter had a test in it. model_based.rs was renamed to mbt.rs just to minimize the diff in this PR.


For contributor use:

  • Updated the Unreleased section of CHANGELOG.md with the issue.
  • If applicable: Unit tests written, added test to CI.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

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

This makes sense. Thanks Vitor for the quick turnaround!

Could you also take care of the Changelog?

@codecov-io
Copy link

Codecov Report

Merging #643 (bb1062a) into master (b1b37f5) will increase coverage by 32.7%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #643      +/-   ##
=========================================
+ Coverage    13.6%   46.4%   +32.7%     
=========================================
  Files          69     152      +83     
  Lines        3752   10165    +6413     
  Branches     1374       0    -1374     
=========================================
+ Hits          513    4722    +4209     
- Misses       2618    5443    +2825     
+ Partials      621       0     -621     
Impacted Files Coverage Δ
modules/src/address.rs 100.0% <ø> (ø)
...application/ics20_fungible_token_transfer/error.rs 0.0% <ø> (ø)
...pplication/ics20_fungible_token_transfer/events.rs 0.0% <ø> (ø)
...ion/ics20_fungible_token_transfer/msgs/transfer.rs 0.0% <ø> (ø)
modules/src/events.rs 0.0% <ø> (ø)
modules/src/handler.rs 100.0% <ø> (ø)
modules/src/ics02_client/client_def.rs 48.3% <ø> (ø)
modules/src/ics02_client/client_type.rs 79.1% <ø> (+31.5%) ⬆️
modules/src/ics02_client/context.rs 100.0% <ø> (ø)
modules/src/ics02_client/error.rs 100.0% <ø> (ø)
... and 260 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca33863...361e046. Read the comment docs.

@vitorenesduarte
Copy link
Contributor Author

Could you also take care of the Changelog?

Done in 361e046. There were two entries for BUG FIXES; I also fixed that.

@adizere adizere merged commit 1b80bc7 into master Feb 12, 2021
@adizere adizere deleted the vitor/test-features branch February 12, 2021 17:57
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
…ems#643)

* Add structure to MBT

* Only run model_based test if mocks feature is enabled

* Remove model_based test from the executor

* update CHANGELOG
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.

MBT tests failing if run from inside modules
3 participants