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

Unit test + log message related to issue #338 #341

Closed
wants to merge 8 commits into from

Conversation

pmconrad
Copy link
Contributor

No description provided.

@pmconrad
Copy link
Contributor Author

I'm seeing about 200 of these log messages during replay.

@oxarbitrage oxarbitrage self-requested a review August 23, 2017 21:06
Copy link
Member

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

make failing in the tests part:

...
[ 91%] Building CXX object tests/CMakeFiles/chain_test.dir/tests/market_tests.cpp.o
/root/bitshares/pull341/bitshares-core/tests/tests/market_tests.cpp: In member function ‘void market_tests::issue_338::test_method()’:
/root/bitshares/pull341/bitshares-core/tests/tests/market_tests.cpp:100:9: warning: variable ‘buy_low’ set but not used [-Wunused-but-set-variable]
    auto buy_low = create_sell_order(buyer, asset(90), bitusd.amount(10))->id;
         ^
/root/bitshares/pull341/bitshares-core/tests/tests/market_tests.cpp:108:9: warning: variable ‘buy_med’ set but not used [-Wunused-but-set-variable]
    auto buy_med = create_sell_order(buyer, asset(105), bitusd.amount(10))->id;
         ^
/root/bitshares/pull341/bitshares-core/tests/tests/market_tests.cpp:116:9: warning: variable ‘buy_high’ set but not used [-Wunused-but-set-variable]
    auto buy_high = create_sell_order(buyer, asset(115), bitusd.amount(10))->id;
         ^
/root/bitshares/pull341/bitshares-core/tests/tests/market_tests.cpp: In member function ‘void market_tests::taker_sells_small_lot_too_low::test_method()’:
/root/bitshares/pull341/bitshares-core/tests/tests/market_tests.cpp:328:134: error: ‘create_sell_order_with_flag’ was not declared in this scope
    limit_order_id_type first_id  = create_sell_order_with_flag( seller_account, core_asset.amount(150), test_asset.amount(100), true )->id;
                                                                                                                                      ^
/root/bitshares/pull341/bitshares-core/tests/tests/market_tests.cpp: In member function ‘void market_tests::taker_buys_small_lot_too_high::test_method()’:
/root/bitshares/pull341/bitshares-core/tests/tests/market_tests.cpp:370:133: error: ‘create_sell_order_with_flag’ was not declared in this scope
    limit_order_id_type first_id  = create_sell_order_with_flag( buyer_account, test_asset.amount(100), core_asset.amount(80), false )->id;
                                                                                                                                     ^
/root/bitshares/pull341/bitshares-core/tests/tests/market_tests.cpp: In member function ‘void market_tests::taker_sells_above_1::test_method()’:
/root/bitshares/pull341/bitshares-core/tests/tests/market_tests.cpp:412:134: error: ‘create_sell_order_with_flag’ was not declared in this scope
    limit_order_id_type first_id  = create_sell_order_with_flag( seller_account, core_asset.amount(400), test_asset.amount(100), true )->id;
                                                                                                                                      ^
/root/bitshares/pull341/bitshares-core/tests/tests/market_tests.cpp: In member function ‘void market_tests::taker_sells_below_1::test_method()’:
/root/bitshares/pull341/bitshares-core/tests/tests/market_tests.cpp:457:134: error: ‘create_sell_order_with_flag’ was not declared in this scope
    limit_order_id_type first_id  = create_sell_order_with_flag( seller_account, core_asset.amount(25), test_asset.amount(100), false )->id;
                                                                                                                                      ^
/root/bitshares/pull341/bitshares-core/tests/tests/market_tests.cpp: In member function ‘void market_tests::taker_buys_below_1::test_method()’:
/root/bitshares/pull341/bitshares-core/tests/tests/market_tests.cpp:502:133: error: ‘create_sell_order_with_flag’ was not declared in this scope
    limit_order_id_type first_id  = create_sell_order_with_flag( buyer_account, test_asset.amount(100), core_asset.amount(25), false )->id;
                                                                                                                                     ^
/root/bitshares/pull341/bitshares-core/tests/tests/market_tests.cpp: In member function ‘void market_tests::taker_buys_above_1::test_method()’:
/root/bitshares/pull341/bitshares-core/tests/tests/market_tests.cpp:547:133: error: ‘create_sell_order_with_flag’ was not declared in this scope
    limit_order_id_type first_id  = create_sell_order_with_flag( buyer_account, test_asset.amount(100), core_asset.amount(400), true )->id;
                                                                                                                                     ^
/root/bitshares/pull341/bitshares-core/tests/tests/market_tests.cpp: In member function ‘void market_tests::create_buy_uia_multiple_match_new::test_method()’:
/root/bitshares/pull341/bitshares-core/tests/tests/market_tests.cpp:592:133: error: ‘create_sell_order_with_flag’ was not declared in this scope
    limit_order_id_type first_id  = create_sell_order_with_flag( buyer_account, test_asset.amount(100), core_asset.amount(100), true )->id;
                                                                                                                                     ^
tests/CMakeFiles/chain_test.dir/build.make:77: recipe for target 'tests/CMakeFiles/chain_test.dir/tests/market_tests.cpp.o' failed
make[2]: *** [tests/CMakeFiles/chain_test.dir/tests/market_tests.cpp.o] Error 1
CMakeFiles/Makefile2:2172: recipe for target 'tests/CMakeFiles/chain_test.dir/all' failed
make[1]: *** [tests/CMakeFiles/chain_test.dir/all] Error 2
Makefile:117: recipe for target 'all' failed
make: *** [all] Error 2
root@NC-PH-1346-07:~/bitshares/pull341/bitshares-core# 

@pmconrad
Copy link
Contributor Author

Sorry about that. I must have messed up when switching branches between tests or something.

@oxarbitrage
Copy link
Member

compiles now, thanks for the changes. as @xeroc stated there are 200+ cases in the bitshares chain:

root@NC-PH-1346-07:~/bitshares/pull341/bitshares-core# cat out.txt | grep -c Multiple
222
root@NC-PH-1346-07:~/bitshares/pull341/bitshares-core#

in the market_tests there are 16 failures:

*** 16 failures detected in test suite "Master Test Suite"
root@NC-PH-1346-07:~/bitshares/pull341/bitshares-core/tests# ./chain_test -t market_tests  -l error

please confirm if the number of failures is the expected so we can merge it.

@oxarbitrage
Copy link
Member

@alexpmorris can you check with us if the 16 failing cases in the tests are the expected results. I can paste full log of the run.

@alexpmorris
Copy link

alexpmorris commented Aug 26, 2017

There are some failed test cases I saw (about 8 or so) that seem unrelated to my code, such as this:

/home/vm/bitshares-core/tests/tests/basic_tests.cpp(54): error: in "basic_tests/valid_name_test": check !is_valid_name( "a" ) has failed

Also, my code shouldn't even be active for the older tests, as the new code must be activated by setting the extension: {"isCoreBuySell"=true/false}. Otherwise, it reverts to the original behavior, which would apply to margin-call (and other) activity.

I did make a bunch of changes since what I'm seeing above though, for example create_sell_order_with_flag() has been replaced with create_sell_order_with_ext(). However, I forget to move it from optest.cpp into database_fixture.cpp (it was faster for me to test/recompile from there when I was figuring out how best to implement an extension).

I now moved it to database_fixture.cpp/hpp, and made the necessary parameter adjustments in the optest.cpp tests. All the latest changes can be found below:

https://github.com/alexpmorris/bitshares-core/commits/order-rounding-fix

alexpmorris referenced this pull request in alexpmorris/bitshares-core Sep 1, 2017
@abitmore
Copy link
Member

abitmore commented Nov 1, 2017

Since we've split #338 to multiple issues, we'd better split this PR to multiple PR's.

@abitmore abitmore added this to the Future Non-Consensus-Changing Release milestone Dec 21, 2017
This was referenced Dec 22, 2017
@pmconrad
Copy link
Contributor Author

Moved the original commits into separate PRs. Unsure about Alex's test cases.

@alexpmorris
Copy link

My test cases were meant to confirm functionality of the code to help deal with the rounding issues on the DEX, So they wouldn't necessarily apply or be useful until those changes (or some variation thereof) were implemented at some point.

@abitmore
Copy link
Member

Closing this PR so far. Will check Alex's commits later, but not here.

@abitmore abitmore closed this Jan 13, 2018
@abitmore abitmore modified the milestones: Future Non-Consensus-Changing Release, 201802 - Non-Consensus-Changing Release Jan 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants