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

Extend runtime tests to run on testnet nodes #874

Merged
merged 7 commits into from
Apr 18, 2019
Merged

Conversation

bowenwang1996
Copy link
Collaborator

No description provided.

core/storage/src/storages/mod.rs Outdated Show resolved Hide resolved
node/configs/src/chain_spec.rs Outdated Show resolved Hide resolved
test-utils/testlib/src/user/rpc_user.rs Show resolved Hide resolved
tests/test_runtime.rs Outdated Show resolved Hide resolved
tests/test_runtime.rs Outdated Show resolved Hide resolved
assert_eq!(account_names[0], alice_account());
let mut nodes: Vec<_> = nodes
.drain(..)
.map(|cfg| match cfg {
Copy link
Contributor

Choose a reason for hiding this comment

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

.map(Node::new)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That will return Node instead of ThreadNode

Copy link
Contributor

Choose a reason for hiding this comment

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

All tests written below accept Node anyway. There is nothing specific in this code that requires ThreadNode. We just need to modify them to accept Box<dyn Node> instead of impl Node.

tests/test_runtime.rs Outdated Show resolved Hide resolved
assert_eq!(account_names[0], alice_account());
let mut nodes: Vec<_> = nodes
.drain(..)
.map(|cfg| match cfg {
Copy link
Contributor

Choose a reason for hiding this comment

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

All tests written below accept Node anyway. There is nothing specific in this code that requires ThreadNode. We just need to modify them to accept Box<dyn Node> instead of impl Node.

@bowenwang1996
Copy link
Collaborator Author

bowenwang1996 commented Apr 17, 2019

Box<Node> does not implement Node. All tests take impl Node.

@MaksymZavershynskyi
Copy link
Contributor

Box<Node> does not implement Node. All tests takes impl Node.

We should change them so that they take Box<dyn Node>.

@MaksymZavershynskyi
Copy link
Contributor

Box<Node> does not implement Node. All tests takes impl Node.

We should change them so that they take Box<dyn Node>.

Actually, I take it back. Using Box<dyn Node> is only useful when we are mixing nodes of various types, which is not the case here.

@codecov
Copy link

codecov bot commented Apr 18, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@28814f9). Click here to learn what that means.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #874   +/-   ##
=========================================
  Coverage          ?   72.61%           
=========================================
  Files             ?       78           
  Lines             ?     9779           
  Branches          ?        0           
=========================================
  Hits              ?     7101           
  Misses            ?     2678           
  Partials          ?        0
Impacted Files Coverage Δ
node/runtime/src/test_utils.rs 97.03% <100%> (ø)
core/mempool/src/lib.rs 86.11% <100%> (ø)
core/storage/src/trie/mod.rs 94.58% <100%> (ø)
node/runtime/src/lib.rs 50.99% <50%> (ø)

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 28814f9...65b8f61. Read the comment docs.

@bowenwang1996 bowenwang1996 merged commit 5aebaf0 into master Apr 18, 2019
@bowenwang1996 bowenwang1996 deleted the testnet-tests branch April 18, 2019 04:07
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