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

Add Unit Test for peers RPC Request [RIPD-1419] #2060

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
4 participants
@mellery451
Copy link
Contributor

commented Mar 21, 2017

No description provided.

@codecov-io

This comment has been minimized.

Copy link

commented Mar 21, 2017

Codecov Report

Merging #2060 into develop will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2060      +/-   ##
===========================================
+ Coverage    68.88%   68.89%   +0.01%     
===========================================
  Files          676      680       +4     
  Lines        49495    49596     +101     
===========================================
+ Hits         34095    34170      +75     
- Misses       15400    15426      +26
Impacted Files Coverage Δ
src/ripple/basics/DecayingSample.h 77.77% <0%> (-8.34%) ⬇️
src/ripple/peerfinder/impl/Logic.h 41.41% <0%> (-7.94%) ⬇️
src/ripple/app/tx/impl/PayChan.h 100% <0%> (ø) ⬆️
src/ripple/json/Writer.h 100% <0%> (ø) ⬆️
src/ripple/basics/impl/Time.cpp 100% <0%> (ø) ⬆️
src/protobuf/vsprojects/config.h
src/beast/extras/beast/unit_test/main.cpp
src/ripple/app/main/Tuning.h
src/protobuf/src/google/protobuf/message.h
src/ripple/basics/ByteOrder.h
... and 23 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 c6b6d82...9a495e4. Read the comment docs.

@mellery451 mellery451 requested review from miguelportilla and nbougalis Mar 22, 2017

@bachase
Copy link
Contributor

left a comment

Minor nits 👍

it != peers[jss::cluster].end();
++it)
{
auto key = it.key().asString();

This comment has been minimized.

Copy link
@bachase

bachase Mar 30, 2017

Contributor

Nit: Use key in the next line or remove it?

This comment has been minimized.

Copy link
@mellery451

mellery451 Mar 31, 2017

Author Contributor

incomplete refactoring - will fix

auto search = nodes.find(it.key().asString());
if(! BEAST_EXPECTS(search != nodes.end(), it.key().asString()))
continue;
auto tag = (*it)[jss::tag].asString();

This comment has been minimized.

Copy link
@bachase

bachase Mar 30, 2017

Contributor

Nit: Check that tag is a member?

This comment has been minimized.

Copy link
@mellery451

mellery451 Mar 31, 2017

Author Contributor

👍

(*it)[jss::tag].asString() == nodes[it.key().asString()],
it.key().asString());
}
BEAST_EXPECT(peers.isMember(jss::peers) &&

This comment has been minimized.

Copy link
@bachase

bachase Mar 30, 2017

Contributor

I'm guessing jtx doesn't have a nice way to add peers in a way to exercise this part of the call?

This comment has been minimized.

Copy link
@mellery451

mellery451 Mar 31, 2017

Author Contributor

yeah - looks like peers comes from context.app.overlay() and I couldn't find a sensible way to manipulate that in Env.

@mellery451 mellery451 force-pushed the mellery451:peers-test branch from d76473a to e2854b3 Mar 31, 2017

@mellery451 mellery451 force-pushed the mellery451:peers-test branch from e2854b3 to 749244f Apr 3, 2017

@nbougalis
Copy link
Member

left a comment

Left a μnit - no need for a change and it's fine as is.

for(auto i =0; i < 3; ++i)
{

auto seed = generateSeed("seed" + std::to_string(i));

This comment has been minimized.

Copy link
@nbougalis

nbougalis Apr 17, 2017

Member

Not change needed, but you could replace 50-53 with:

auto kp = generateKeyPair (KeyType::secp256k1,
    generateSeed("seed" + std::to_string(i)));

You'd also have the SecretKey in kp which you don't strictly need, but, overall, I think it would simpify the code.

This comment has been minimized.

Copy link
@mellery451

mellery451 Apr 17, 2017

Author Contributor

👍 will do

@nbougalis

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

Merged as af66c62

@nbougalis nbougalis closed this Apr 19, 2017

@mellery451 mellery451 deleted the mellery451:peers-test branch Apr 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.