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 tx_history RPC (RIPD-1402) #2062

Closed
wants to merge 2 commits into
base: develop
from

Conversation

Projects
None yet
6 participants
@mellery451
Contributor

mellery451 commented Mar 21, 2017

No description provided.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Mar 22, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #2062      +/-   ##
==========================================
+ Coverage    68.54%   68.6%   +0.06%     
==========================================
  Files          679     679              
  Lines        49554   49554              
==========================================
+ Hits         33965   33997      +32     
+ Misses       15589   15557      -32
Impacted Files Coverage Δ
src/ripple/basics/DecayingSample.h 77.77% <0%> (-8.34%) ⬇️
src/ripple/beast/asio/io_latency_probe.h 97.01% <0%> (-1.5%) ⬇️
src/ripple/basics/impl/make_SSLContext.cpp 52.22% <0%> (-1.28%) ⬇️
src/ripple/app/misc/impl/Transaction.cpp 52.45% <0%> (+14.75%) ⬆️
src/ripple/rpc/handlers/TxHistory.cpp 96.66% <0%> (+96.66%) ⬆️

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 dbe74df...881c30b. Read the comment docs.

codecov-io commented Mar 22, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #2062      +/-   ##
==========================================
+ Coverage    68.54%   68.6%   +0.06%     
==========================================
  Files          679     679              
  Lines        49554   49554              
==========================================
+ Hits         33965   33997      +32     
+ Misses       15589   15557      -32
Impacted Files Coverage Δ
src/ripple/basics/DecayingSample.h 77.77% <0%> (-8.34%) ⬇️
src/ripple/beast/asio/io_latency_probe.h 97.01% <0%> (-1.5%) ⬇️
src/ripple/basics/impl/make_SSLContext.cpp 52.22% <0%> (-1.28%) ⬇️
src/ripple/app/misc/impl/Transaction.cpp 52.45% <0%> (+14.75%) ⬆️
src/ripple/rpc/handlers/TxHistory.cpp 96.66% <0%> (+96.66%) ⬆️

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 dbe74df...881c30b. Read the comment docs.

@mellery451 mellery451 requested review from seelabs and scottschurr Mar 22, 2017

@scottschurr

👍 Looks good to me. I left a couple of thoughts, but no required changes.

In a similar situation I might have been inclined to nose about in the returned transactions a bit more for a spot check that you got back what you expected. You could, for example, count various transaction types. But that's certainly not a requirement.

@@ -0,0 +1,128 @@
//------------------------------------------------------------------------------

This comment has been minimized.

@scottschurr

scottschurr Mar 23, 2017

Contributor

FWIW, the online documentation (https://ripple.com/build/rippled-apis/#tx-history) says tx_history is deprecated. Is it better to test it or remove it? Just for your consideration.

@scottschurr

scottschurr Mar 23, 2017

Contributor

FWIW, the online documentation (https://ripple.com/build/rippled-apis/#tx-history) says tx_history is deprecated. Is it better to test it or remove it? Just for your consideration.

Show outdated Hide outdated src/test/rpc/TransactionHistory_test.cpp
Show outdated Hide outdated src/test/rpc/TransactionHistory_test.cpp
@seelabs

Here's a commit with my feedback comments: seelabs@76138e4

It's all minor. 👍 With or without those changes

@mellery451

This comment has been minimized.

Show comment
Hide comment
@mellery451

mellery451 Mar 23, 2017

Contributor

Thanks to both Scotts - will be incorporating the suggested changes shortly

Contributor

mellery451 commented Mar 23, 2017

Thanks to both Scotts - will be incorporating the suggested changes shortly

@scottschurr

👍 Nice looking tests.

{
// test at 1 greater than the allowed non-admin limit
Json::Value params {Json::objectValue};
params[jss::start] = 10001; //limited to <= 10000 for non admin

This comment has been minimized.

@scottschurr

scottschurr Mar 24, 2017

Contributor

Cool. Testing at the boundary improved the comment too...

@scottschurr

scottschurr Mar 24, 2017

Contributor

Cool. Testing at the boundary improved the comment too...

// create enough transactions to provide some
// history...
size_t const numAccounts = 20;

This comment has been minimized.

@scottschurr

scottschurr Mar 24, 2017

Contributor

Huh. Does this work on all our compilers? I didn't think you could use a local const integer as a template argument. I would have expected it to work with a static or constexpr decorator, but not a plain const.

I tried to see if I could find something about this in the standard, but N4660 section 8.20 is too dense for me to make heads or tails out of. I'll go with the standard being headed in the direction of allowing this if all 3 compilers support this.

@scottschurr

scottschurr Mar 24, 2017

Contributor

Huh. Does this work on all our compilers? I didn't think you could use a local const integer as a template argument. I would have expected it to work with a static or constexpr decorator, but not a plain const.

I tried to see if I could find something about this in the standard, but N4660 section 8.20 is too dense for me to make heads or tails out of. I'll go with the standard being headed in the direction of allowing this if all 3 compilers support this.

This comment has been minimized.

@HowardHinnant

HowardHinnant Mar 24, 2017

Contributor

All three compilers allow it. However I would prefer const / constexpr.

@HowardHinnant

HowardHinnant Mar 24, 2017

Contributor

All three compilers allow it. However I would prefer const / constexpr.

This comment has been minimized.

@HowardHinnant

HowardHinnant Mar 24, 2017

Contributor

For the curious, it is legal with the const, as long as the rhs-expression is a compile-time integral constant. But if you replace that rhs-expression with a run-time integral constant (e.g. by calling a non-constexpr function), then it fails when you try to use numAccounts in the template expression. If numAccounts is marked constexpr, the failure happens at the numAccounts definition instead of at the use of numAccounts with a template.

@HowardHinnant

HowardHinnant Mar 24, 2017

Contributor

For the curious, it is legal with the const, as long as the rhs-expression is a compile-time integral constant. But if you replace that rhs-expression with a run-time integral constant (e.g. by calling a non-constexpr function), then it fails when you try to use numAccounts in the template expression. If numAccounts is marked constexpr, the failure happens at the numAccounts definition instead of at the use of numAccounts with a template.

This comment has been minimized.

@scottschurr

scottschurr Mar 24, 2017

Contributor

Thanks for the clarification.

@scottschurr

scottschurr Mar 24, 2017

Contributor

Thanks for the clarification.

BEAST_EXPECT(typeCounts["Payment"] == 58);
BEAST_EXPECT(typeCounts["OfferCreate"] == 20);
// also, try a request with max non-admin start value

This comment has been minimized.

@scottschurr

scottschurr Mar 24, 2017

Contributor

Nice addition, testing the functioning side of the boundary.

@scottschurr

scottschurr Mar 24, 2017

Contributor

Nice addition, testing the functioning side of the boundary.

@scottschurr scottschurr self-assigned this Mar 24, 2017

@mellery451 mellery451 added the Passed label Mar 24, 2017

@nbougalis nbougalis closed this Apr 1, 2017

@mellery451 mellery451 deleted the mellery451:txhistory-test branch Apr 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment