Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Finish / fix price_to_string_test #1186

Closed
abitmore opened this issue Jul 26, 2018 · 4 comments
Closed

Finish / fix price_to_string_test #1186

abitmore opened this issue Jul 26, 2018 · 4 comments
Labels
4a Low Priority Priority indicating minimal impact to system/user -OR- an inexpensive workaround exists 9c Large Effort estimation indicating TBD low priority testing

Comments

@abitmore
Copy link
Member

Code:

Travis-CI reports:

3568856ms th_a app_util_tests.cpp:217 test_method ] i: 0 j: 0 p[i][j]: {"base":{"amount":-1,"asset_id":"1.3.0"},"quote":{"amount":-1,"asset_id":"1.3.0"}}

3568856ms th_a app_util_tests.cpp:239 test_method ] [ERROR: Got exception while trying to dump ( (price_to_string(p[i][j],0,0))(price_to_string(p[i][j],0,1))(price_to_string(p[i][j],0,2))(price_to_string(p[i][j],0,8))(price_to_string(p[i][j],0,19))(price_to_string(p[i][j],1,0))(price_to_string(p[i][j],1,15))(price_to_string(p[i][j],2,6))(price_to_string(p[i][j],2,10))(price_to_string(p[i][j],5,0))(price_to_string(p[i][j],9,1))(price_to_string(p[i][j],9,9))(price_to_string(p[i][j],9,19))(price_to_string(p[i][j],18,10))(price_to_string(p[i][j],18,13))(price_to_string(p[i][j],18,19))(price_to_string(p[i][j],19,0))(price_to_string(p[i][j],19,7))(price_to_string(p[i][j],19,19))(price_diff_percent_string(p[i][j],p[j][i])) )]

@abitmore abitmore added testing low priority 4a Low Priority Priority indicating minimal impact to system/user -OR- an inexpensive workaround exists 9c Large Effort estimation indicating TBD labels Jul 26, 2018
@jmjatlanta
Copy link
Contributor

I realize this is low priority, but please include a hint as to what is left to be done. From my testing, the catch is never called, although the log shows some errors. Are you suggesting that we need to get rid of the idump and verify that an exception is thrown at the right time?

@jmjatlanta
Copy link
Contributor

jmjatlanta commented Jan 8, 2019

What should happen if the _price.base.amount is 0? price_to_string is currently returning the string "0", paying no attention to precision. But yet the next line asserts that _price.base.amount >= 0, which the = will never be true, as the return "0" above would have been hit. Should the FC_ASSERT be changed to _price.base.amount > 0? Or should the call ASSERT due to the _price.base.amount being 0?

string price_to_string( const price& _price, const uint8_t base_precision, const uint8_t quote_precision )
{ try {
if( _price.base.amount == 0 )
return "0";
FC_ASSERT( _price.base.amount >= 0 );
FC_ASSERT( _price.quote.amount >= 0 );
FC_ASSERT( base_precision <= 19 );
FC_ASSERT( quote_precision <= 19 );

Also, should the rules be similar for price_diff_percent_string that is also being tested?

@pmconrad
Copy link
Contributor

pmconrad commented Jan 9, 2019

IMO the test needs special handling for i == 1 (result always equal to "0") and i == 0 || j == 0 (always throws).
The idump can be removed, unless you want to manually inspect the results in each test run. Better choose some reasonable sample for i and j and test these explicitly.

@jmjatlanta
Copy link
Contributor

Fixed by PR #1518

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
4a Low Priority Priority indicating minimal impact to system/user -OR- an inexpensive workaround exists 9c Large Effort estimation indicating TBD low priority testing
Projects
None yet
Development

No branches or pull requests

3 participants