Fix tests #1126

nathanielhourt · 2018-07-07T08:09:30Z

Fix failing tests
Move obnoxiously slow test to separate test driver

I'm running Arch with clang 6.0.0, boost 1.67.0.

Two tests were failing due to a divide by zero bug. Add a check to bound out the divide by zero.

The main test driver should run fairly quickly; extremely slow tests should be moved to a separate driver so developer time isn't wasted waiting for many runs of a slow test. Really more tests should be moved over, but I'm too lazy right now.

abitmore

The test case was not broken, so this PR is more an optimization than a fix.

There were efforts to include all consensus related tests into chain_test. Moving tests outside of chain_test will likely make them be ignored and may cause unexpected consequences.

abitmore · 2018-07-07T11:30:08Z

libraries/chain/market_evaluator.cpp

+      auto new_debt = *old_debt + o.delta_debt.amount;
+
+      // Forbid zero collateral with nonzero debt (avoids divide by zero when calculating call price below)
+      FC_ASSERT(!(new_collateral == 0 && new_debt != 0), "Cannot have zero collateral and nonzero debt");


"zero collateral with nonzero debt" is covered by this test case:

bitshares-core/tests/tests/operation_tests.cpp

Lines 146 to 147 in c6b28b2

BOOST_TEST_MESSAGE( "attempting to claim all collateral without paying off debt" );

GRAPHENE_REQUIRE_THROW( cover( dan, bitusd.amount(0), asset(20000)), fc::exception );

price::call_price() will generate a division-by-zero exception, then the exception will be captured and converted to fc::exception, then be caught by callers. So I think this change is more an optimization rather than a fix. No?

Incorrect. The test fails, because boost captures the division by zero exception, as it occurs inside the modify functor (line 257), triggering the object to be deleted. As per boost docs, this is expected behavior:

https://www.boost.org/doc/libs/1_67_0/libs/multi_index/doc/reference/ord_indices.html#modify
Exception safety: Basic. If an exception is thrown by some user-provided operation (including mod), then the element pointed to by position is erased.

abitmore · 2018-07-07T12:32:32Z

tests/tests/operation_tests.cpp

@@ -145,6 +145,7 @@ BOOST_AUTO_TEST_CASE( call_order_update_test )
      GRAPHENE_REQUIRE_THROW( borrow( dan, bitusd.amount(80000), asset(0)), fc::exception );
      BOOST_TEST_MESSAGE( "attempting to claim all collateral without paying off debt" );
      GRAPHENE_REQUIRE_THROW( cover( dan, bitusd.amount(0), asset(20000)), fc::exception );
+      GRAPHENE_REQUIRE_THROW( cover( dan, bitusd.amount(0), asset(20000-1)), fc::exception );


What is this new test for? Actually it will throw an exception due to triggering a blackswan which is not allowed when updating a call position. It worth testing, but better add a message above it.

The underlying implementation now explicitly checks for zero collateral. This test was added to probe the edge case that collateral is not zero, but still too low to sustain the debt. I feel it is sufficiently described by the message, "attempting to claim all collateral without paying off debt"; even though it leaves one unit of collateral unclaimed, the spirit is the same.

0 and 1 are absolutely different.

abitmore · 2018-07-07T12:55:27Z

@nathanhourt would you please clarify what environment you're using which detected the failing tests? E.G.

OS version
Compiler and version
Boost version

Thanks.

nathanielhourt · 2018-07-07T15:09:00Z

I'm running Arch with clang 6.0.0, boost 1.67.0.

nathan@spiral ~/dev/bitshares-core/build/debug/tests (git)-[remotes/origin/develop] % ./chain_test -t operation_tests/call_order_update_test                                                                                                                                                                                                                                                                                                :(
Random number generator seeded to 1530975831
GRAPHENE_TESTING_GENESIS_TIMESTAMP is 1431700000
Running 1 test case...
231200ms th_a       db_management.cpp:153         open                 ] Wiping object_database due to missing or wrong version
231200ms th_a       object_database.cpp:93        wipe                 ] Wiping object database...
231200ms th_a       object_database.cpp:95        wipe                 ] Done wiping object databse.
231200ms th_a       object_database.cpp:106       open                 ] Opening object database from /tmp/graphene-tmp/17d1-b3dc-ad22-3e1d ...
231200ms th_a       object_database.cpp:111       open                 ] Done opening object database.
231248ms th_a       db_update.cpp:230             check_for_blackswan  ] *call_itr: {"id":"1.8.1","borrower":"1.2.16","collateral":20000,"debt":85000,"call_price":{"base":{"amount":16,"asset_id":"1.3.0"},"quote":{"amount":119,"asset_id":"1.3.2"}}} 
231248ms th_a       db_update.cpp:240             check_for_blackswan  ] Black Swan detected on asset USDBIT (1.3.2) at block 1: 
   Least collateralized call: 0.23529411764705882  4.25000000000000000
   Settle Price:              1.00000000000000000  1.00000000000000000
   Max:                       1.00000000000000000  1.00000000000000000

231249ms th_a       db_update.cpp:241             check_for_blackswan  ] enable_black_swan: false 
231249ms th_a       undo_database.hpp:68          ~session             ] 10 assert_exception: Assert Exception
maybe_found != nullptr: Unable to find Object 1.8.1
    {"id":"1.8.1"}
    th_a  index.hpp:111 get

    {}
    th_a  undo_database.cpp:123 undo
terminate called after throwing an instance of 'fc::assert_exception'
unknown location(0): fatal error: in "operation_tests/call_order_update_test": signal: SIGABRT (application abort requested)
../../tests/tests/operation_tests.cpp(147): last checkpoint

*** 1 failure is detected in the test module "Master Test Suite"
201 nathan@spiral ~/dev/bitshares-core/build/debug/tests (git)-[remotes/origin/develop] % ./chain_test -t operation_tests/old_call_order_update_test_after_hardfork_583                                                                                                                                                                                                                                                                         :(
Random number generator seeded to 1530975832
GRAPHENE_TESTING_GENESIS_TIMESTAMP is 1431700000
Running 1 test case...
232891ms th_a       db_management.cpp:153         open                 ] Wiping object_database due to missing or wrong version
232891ms th_a       object_database.cpp:93        wipe                 ] Wiping object database...
232891ms th_a       object_database.cpp:95        wipe                 ] Done wiping object databse.
232891ms th_a       object_database.cpp:106       open                 ] Opening object database from /tmp/graphene-tmp/7f57-bd5b-56d8-3f12 ...
232891ms th_a       object_database.cpp:111       open                 ] Done opening object database.
232935ms th_a       db_maint.cpp:897              process_hf_868_890   ] Processing hard fork core-868-890 at block 3
232935ms th_a       db_maint.cpp:981              process_hf_868_890   ] Done processing hard fork core-868-890 at block 3
232935ms th_a       db_maint.cpp:805              update_and_match_cal ] Updating all call orders for hardfork core-343 at block 3
232935ms th_a       db_maint.cpp:833              update_and_match_cal ] Done updating all call orders for hardfork core-343 at block 3
232938ms th_a       db_update.cpp:230             check_for_blackswan  ] *call_itr: {"id":"1.8.1","borrower":"1.2.16","collateral":20000,"debt":85000,"call_price":{"base":{"amount":16,"asset_id":"1.3.0"},"quote":{"amount":119,"asset_id":"1.3.2"}}} 
232938ms th_a       db_update.cpp:240             check_for_blackswan  ] Black Swan detected on asset USDBIT (1.3.2) at block 4: 
   Least collateralized call: 0.23529411764705882  4.25000000000000000
   Settle Price:              1.00000000000000000  1.00000000000000000
   Max:                       1.50000000000000000  0.66666666666666663

232938ms th_a       db_update.cpp:241             check_for_blackswan  ] enable_black_swan: false 
232939ms th_a       undo_database.hpp:68          ~session             ] 10 assert_exception: Assert Exception
maybe_found != nullptr: Unable to find Object 1.8.1
    {"id":"1.8.1"}
    th_a  index.hpp:111 get

    {}
    th_a  undo_database.cpp:123 undo
terminate called after throwing an instance of 'fc::assert_exception'
unknown location(0): fatal error: in "operation_tests/old_call_order_update_test_after_hardfork_583": signal: SIGABRT (application abort requested)
../../tests/tests/operation_tests.cpp(249): last checkpoint

*** 1 failure is detected in the test module "Master Test Suite"

nathanielhourt · 2018-07-07T15:17:23Z

Oops, didn't mean to close this...

I'm not sure what ci/dockercloud is angry about, as I am apparently forbidden to view the logs.

abitmore · 2018-07-07T19:17:46Z

Thanks. Boost 1.66+ is known to have issues with current object database and/or undo database implementation. I believe not only these two test cases are affected. More discussions:

abitmore · 2018-07-07T19:31:57Z

Update: created a new project to track all boost 1.66+ related issues.

nathanielhourt · 2018-07-08T01:43:02Z

Oh, OK, so it looks like I fixed #852 then?

…odify() Current boost will delete an object from a multi_index_container if when modifying that object, the functor throws an exception. This breaks the undo infrastructure when it tries to undo the failed change, but the object is missing. To prevent this, we catch the exception before it reaches boost.

nathanielhourt · 2018-07-09T01:37:35Z

Added commit to ensure we don't trip if some other modify functor ever throws in the future

abitmore

Good job.

Does this PR fix #694?

abitmore · 2018-07-09T11:03:40Z

libraries/db/include/graphene/db/generic_index.hpp

+                                          }
+                                       }
+                      );
+            FC_ASSERT(!exception, "Aborting for exception while modifying object");


Best if can re-throw the real exception rather than covering it with a new message and logging it only, so a client program will know what happened.

That is true. I went with the lazy option to start, because copying exceptions can be funny, but I'll play with it a bit more

…older

nathanielhourt · 2018-07-09T16:43:37Z

OK, now it throws the original exception rather than a placeholder.

As to #694, that looks like a non-issue to me. The original report seems to be that someone was modifying one entry's key to collide with another one? I agree with the discussion on #694, that doesn't make sense to do, and it's not clear what the expected, non-error behavior would be if such a thing were attempted.

abitmore · 2018-07-09T20:18:46Z

@nathanhourt the Travis building log is public but DockerCloud is not.

/home/travis/build/bitshares/bitshares-core/tests/tests/database_tests.cpp:140:1:   required from here
/usr/include/boost/test/test_tools.hpp:326:14: error: ambiguous overload for ‘operator<<’ (operand types are ‘std::ostream {aka std::basic_ostream<char>}’ and ‘std::nullptr_t’)
         ostr << t; // by default print the value
              ^

In other words, hopefully this makes travis work

nathanielhourt · 2018-07-10T16:22:39Z

Travis, you dunce... OK, try this...

nathanielhourt · 2018-07-12T15:01:55Z

So I can't fix the docker test without some idea as to where it's breaking... Is there something else I can do here?

abitmore · 2018-07-12T20:57:39Z

@nathanhourt thanks for your contribution.

If you're asking me, I'd like to see

the slow_tests change to be moved to another PR.

Other changes in this PR are mostly fine for me, except some minor issues:

Fix tests #1126 (comment)
perhaps add a same line in the test case old_call_order_update_test_after_hardfork_583
Fix tests #1126 (comment)

By the way, most core dev team members are on vacation now, I think they'll comment when came back.

nathanielhourt added 2 commits July 7, 2018 02:51

Fix tests

7f17d17

Two tests were failing due to a divide by zero bug. Add a check to bound out the divide by zero.

Segregate obnoxiously slow test

00c9188

The main test driver should run fairly quickly; extremely slow tests should be moved to a separate driver so developer time isn't wasted waiting for many runs of a slow test. Really more tests should be moved over, but I'm too lazy right now.

abitmore reviewed Jul 7, 2018

View reviewed changes

nathanielhourt closed this Jul 7, 2018

nathanielhourt reopened this Jul 7, 2018

nathanielhourt mentioned this pull request Jul 8, 2018

Boost 1.66 incompatible with Bitshares Undo process #852

Closed

nathanielhourt force-pushed the nathan-testfix branch from 501646c to 7dd4ce0 Compare July 9, 2018 01:36

abitmore reviewed Jul 9, 2018

View reviewed changes

When a modify() throws, rethrow original exception rather than placeh…

025bd61

…older

Give rusty old-fashioned boost a crutch

1561981

In other words, hopefully this makes travis work

abitmore self-assigned this Jul 21, 2018

abitmore changed the base branch from develop to 852-boost166 July 21, 2018 17:27

abitmore approved these changes Jul 21, 2018

View reviewed changes

abitmore merged commit 444a21b into bitshares:852-boost166 Jul 21, 2018

abitmore mentioned this pull request Jul 21, 2018

Segregate obnoxiously slow test #1160

Closed

abitmore mentioned this pull request Jul 21, 2018

Fix boost 1.66 compatibility #1161

Merged

nathanielhourt deleted the nathan-testfix branch July 30, 2018 21:32

abitmore mentioned this pull request Aug 4, 2018

Dummy issue for release note for (test-)2.0.1808xx #1234

Closed

pmconrad mentioned this pull request Aug 27, 2018

Merge improvements from BitShares soundac/SounDAC-Source#26

Open

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix tests #1126

Fix tests #1126

nathanielhourt commented Jul 7, 2018 •

edited by abitmore

Loading

abitmore left a comment

abitmore Jul 7, 2018

nathanielhourt Jul 7, 2018

abitmore Jul 7, 2018

nathanielhourt Jul 7, 2018

abitmore Jul 7, 2018

abitmore commented Jul 7, 2018

nathanielhourt commented Jul 7, 2018

nathanielhourt commented Jul 7, 2018

abitmore commented Jul 7, 2018

abitmore commented Jul 7, 2018

nathanielhourt commented Jul 8, 2018

nathanielhourt commented Jul 9, 2018

abitmore left a comment

abitmore Jul 9, 2018

nathanielhourt Jul 9, 2018

nathanielhourt commented Jul 9, 2018

abitmore commented Jul 9, 2018

nathanielhourt commented Jul 10, 2018

nathanielhourt commented Jul 12, 2018

abitmore commented Jul 12, 2018

	BOOST_TEST_MESSAGE( "attempting to claim all collateral without paying off debt" );
	GRAPHENE_REQUIRE_THROW( cover( dan, bitusd.amount(0), asset(20000)), fc::exception );

Fix tests #1126

Fix tests #1126

Conversation

nathanielhourt commented Jul 7, 2018 • edited by abitmore Loading

abitmore left a comment

Choose a reason for hiding this comment

abitmore Jul 7, 2018

Choose a reason for hiding this comment

nathanielhourt Jul 7, 2018

Choose a reason for hiding this comment

abitmore Jul 7, 2018

Choose a reason for hiding this comment

nathanielhourt Jul 7, 2018

Choose a reason for hiding this comment

abitmore Jul 7, 2018

Choose a reason for hiding this comment

abitmore commented Jul 7, 2018

nathanielhourt commented Jul 7, 2018

nathanielhourt commented Jul 7, 2018

abitmore commented Jul 7, 2018

abitmore commented Jul 7, 2018

nathanielhourt commented Jul 8, 2018

nathanielhourt commented Jul 9, 2018

abitmore left a comment

Choose a reason for hiding this comment

abitmore Jul 9, 2018

Choose a reason for hiding this comment

nathanielhourt Jul 9, 2018

Choose a reason for hiding this comment

nathanielhourt commented Jul 9, 2018

abitmore commented Jul 9, 2018

nathanielhourt commented Jul 10, 2018

nathanielhourt commented Jul 12, 2018

abitmore commented Jul 12, 2018

nathanielhourt commented Jul 7, 2018 •

edited by abitmore

Loading