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 get_counts RPC method (RIPD-1399) #2011

Closed
wants to merge 4 commits into from
Closed

Add unit test for get_counts RPC method (RIPD-1399) #2011

wants to merge 4 commits into from

Conversation

mellery451
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented Feb 10, 2017

Codecov Report

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

@@             Coverage Diff             @@
##           develop    #2011      +/-   ##
===========================================
+ Coverage    67.25%   67.54%   +0.29%     
===========================================
  Files          683      683              
  Lines        49231    49231              
===========================================
+ Hits         33111    33255     +144     
+ Misses       16120    15976     -144
Impacted Files Coverage Δ
src/ripple/app/ledger/PendingSaves.h 93.54% <0%> (-6.46%)
src/ripple/basics/impl/make_SSLContext.cpp 52.22% <0%> (-1.28%)
src/ripple/app/misc/NetworkOPs.cpp 62.56% <0%> (+0.15%)
src/ripple/app/ledger/impl/LedgerMaster.cpp 44.64% <0%> (+0.22%)
src/ripple/app/main/Application.cpp 60.63% <0%> (+0.26%)
src/ripple/protocol/STObject.h 91.56% <0%> (+0.42%)
src/ripple/net/impl/RPCCall.cpp 36.15% <0%> (+0.82%)
src/ripple/basics/TaggedCache.h 79.42% <0%> (+2.28%)
src/ripple/app/ledger/impl/InboundLedgers.cpp 27.81% <0%> (+2.36%)
src/ripple/protocol/STArray.h 87.8% <0%> (+2.43%)
... and 17 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 0b605b3...914616f. Read the comment docs.

Copy link
Collaborator

@bachase bachase left a comment

Choose a reason for hiding this comment

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

👍

BEAST_EXPECT(jrr[jss::status] == "success");
BEAST_EXPECT(! jrr.isMember("Transaction"));
BEAST_EXPECT(! jrr.isMember("STObject"));
BEAST_EXPECT(! jrr.isMember("HashRouterEntry"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

How did you decide which fields to look for?

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 looked for counted objects in the subsequent request (after there are some actual counts reported) and then just verified that those same fields are not present in this initial request. This is definitely not exhaustive and probably could be improved.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems reasonable to me since the RPC doesn't guarantee much.

BEAST_EXPECT(jrr[jss::status] == "success");
BEAST_EXPECT(
jrr.isMember("Transaction") &&
jrr["Transaction"].asInt() > 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a little surprised that you're using > 0 comparisons. Since you have complete control of the environment shouldn't most of these values be repeatable? I'm just surprised, not necessarily objecting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are "counted objects" which I didn't feel I had direct enough control over to warrant an exact check. That said, I can very likely access the CountedObjects singleton and validate directly -- do you think that would worthwhile?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm. I'm a little surprised that you don't think the CountedObjects are predicable enough to give reliable results. But I've never looked at them very hard. I certainly accept your assessment.

Interesting thought about comparing to the CountedObject values. It would not be worth heroic efforts to have an exact value to compare against. But if it's relatively easy to do it might be nice. Your call.

Copy link
Collaborator

@scottschurr scottschurr left a comment

Choose a reason for hiding this comment

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

What's here looks good to me. 👍

BEAST_EXPECT(jrr[jss::status] == "success");
BEAST_EXPECT(
jrr.isMember("Transaction") &&
jrr["Transaction"].asInt() > 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm. I'm a little surprised that you don't think the CountedObjects are predicable enough to give reliable results. But I've never looked at them very hard. I certainly accept your assessment.

Interesting thought about comparing to the CountedObject values. It would not be worth heroic efforts to have an exact value to compare against. But if it's relatively easy to do it might be nice. Your call.

@mellery451 mellery451 added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Feb 16, 2017
@scottschurr
Copy link
Collaborator

Nice! Revised tests look even better. 👍

@seelabs
Copy link
Collaborator

seelabs commented Mar 1, 2017

In 0.60.0-b7

@seelabs seelabs closed this Mar 1, 2017
@mellery451 mellery451 deleted the getcounts branch March 1, 2017 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants