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

Refactor cli_test #1192 #1243

Merged
merged 11 commits into from Aug 29, 2018

Conversation

Projects
5 participants
@cogutvalera
Member

cogutvalera commented Aug 9, 2018

PR for #1192

@ryanRfox ryanRfox added this to In Testing in Community Claims Aug 9, 2018

@ryanRfox

This comment has been minimized.

Member

ryanRfox commented Aug 9, 2018

This is ready for testing. Valera mentioned his original estimate of 4 hours was too few. Now estimating 10 hours. Core Team: please assist me with revising the estimation based on the original Issue, comments, development and resulting PR.

@ryanRfox ryanRfox added this to In progress in Feature release (201810) via automation Aug 9, 2018

@ryanRfox ryanRfox moved this from In progress to To do in Feature release (201810) Aug 9, 2018

@cogutvalera

This comment has been minimized.

Member

cogutvalera commented Aug 9, 2018

I've spent even more than 10 hours, so guys specify your estimation please and I will agree with it, because I'm still new dev in BitShares Core and my estimation maybe wrong for you ;-) you know better than me how many hours it would take for you ...

using namespace graphene::chain;
using namespace graphene::app;
//using namespace graphene::chain;
//using namespace graphene::app;

This comment has been minimized.

@oxarbitrage

oxarbitrage Aug 10, 2018

Member

pls remove comments.

This comment has been minimized.

@cogutvalera

cogutvalera Aug 10, 2018

Member

sure ! removed ! thanks !

Feature release (201810) automation moved this from To do to In Testing Aug 10, 2018

@oxarbitrage

looks good to me, tested and works as expected. well done.

Feature release (201810) automation moved this from In Testing to In progress Aug 10, 2018

@cogutvalera

This comment has been minimized.

Member

cogutvalera commented Aug 10, 2018

Thanks !

Feature release (201810) automation moved this from In progress to In Testing Aug 10, 2018

@cogutvalera

This comment has been minimized.

Member

cogutvalera commented Aug 14, 2018

can we merge this PR and close related issue ? or this is not still ready and need some changes and improvements ? if YES, then which changes and improvements in your opinion ?

Thanks !

@abitmore

This comment has been minimized.

Member

abitmore commented Aug 14, 2018

I'd say split it into smaller files.

}
app1->shutdown();
try
{

This comment has been minimized.

@abitmore

abitmore Aug 14, 2018

Member

Why changed the indentation to 4 white spaces (both above and below)?

This comment has been minimized.

@cogutvalera

cogutvalera Aug 14, 2018

Member

where can I see BitShares Core coding convention ? I prefer 4 white spaces, what do you prefer ?

This comment has been minimized.

@oxarbitrage

oxarbitrage Aug 14, 2018

Member

3 spaces. sorry @abitmore i didnt saw this one.

This comment has been minimized.

@cogutvalera

cogutvalera Aug 14, 2018

Member

indentation everywhere in BitShares Core code base is prefer to 3 spaces ?

This comment has been minimized.

@oxarbitrage

oxarbitrage Aug 14, 2018

Member

yes. 3 spaces everywhere(except p2p code that uses 2 but we don't want to change that now)

This comment has been minimized.

@cogutvalera

cogutvalera Aug 14, 2018

Member

ok sure. Thank you very much ! Does BitShares Core have code convention ?

This comment has been minimized.

@abitmore

abitmore Aug 23, 2018

Member

Can you please change the indentation back to 3 white spaces? Thanks.

We don't have code conventions so far.

account_object nathan_acct_after_upgrade;
graphene::wallet::brain_key_info bki;
signed_transaction create_acct_tx;
signed_transaction transfer_tx;

This comment has been minimized.

@abitmore

abitmore Aug 14, 2018

Member

Should not define so many different transaction objects in cli_fixture; nathan_acct_before_upgrade and nathan_acct_after_upgrade should not be here as well; they should be declared in individual test cases.

BOOST_TEST_MESSAGE("Importing nathan's balance");
import_txs = con->wallet_api_ptr->import_balance("nathan", nathan_keys, true);
nathan_acct_before_upgrade = con->wallet_api_ptr->get_account("nathan");

This comment has been minimized.

@abitmore

abitmore Aug 14, 2018

Member

The nathan account upgrading should be in a "basic" test case and be invoked by needed test cases, but not in fixture.

// create a new account
bki = con->wallet_api_ptr->suggest_brain_key();
BOOST_CHECK(!bki.brain_priv_key.empty());
create_acct_tx = con->wallet_api_ptr->create_account_with_brain_key(bki.brain_priv_key, "jmjatlanta", "nathan", "nathan", true);

This comment has been minimized.

@abitmore

abitmore Aug 14, 2018

Member

The jmj account creation should be in a test case as well.

// attempt to give jmjatlanta some bitsahres
BOOST_TEST_MESSAGE("Transferring bitshares from Nathan to jmjatlanta");
transfer_tx = con->wallet_api_ptr->transfer("nathan", "jmjatlanta", "10000", "1.3.0", "Here are some CORE token for your new account", true);

This comment has been minimized.

@abitmore

abitmore Aug 14, 2018

Member

The transfer should not be in cli_fixture.

Feature release (201810) automation moved this from In Testing to In progress Aug 14, 2018

@cogutvalera

This comment has been minimized.

Member

cogutvalera commented Aug 18, 2018

made improvements as @abitmore mentioned, do we need here some more changes/improvements or not ?

}
app1->shutdown();
try
{

This comment has been minimized.

@abitmore

abitmore Aug 23, 2018

Member

Can you please change the indentation back to 3 white spaces? Thanks.

We don't have code conventions so far.

int server_port_number {0};
fc::temp_directory app_dir = graphene::utilities::temp_directory_path();
std::shared_ptr<graphene::app::application> app1 = start_application(app_dir, server_port_number);
client_connection* con = new client_connection(app1, app_dir, server_port_number);

This comment has been minimized.

@abitmore

abitmore Aug 23, 2018

Member

Initializing these variables here is a bad idea. They're heavy. Better move them to a constructor. Then some member variables can be changed to local variable.

By the way, why need to use a pointer?

This comment has been minimized.

@cogutvalera

cogutvalera Aug 24, 2018

Member
  1. fixed indentation from 4 white spaces to 3 (commit is ready)
  2. initializing variables in constructor has different compile errors, need to fight all them, trying to understand why they happened, still fighting ...
  3. I've used pointer because without pointer I had compile errors, still trying to understand why and still fighting with them ...

This comment has been minimized.

@cogutvalera

cogutvalera Aug 26, 2018

Member
unknown location(0): fatal error: in "cli_connect": memory access violation at address: 0x00000064: no mapping at fault address
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/tests/cli/main.cpp(260): last checkpoint: "cli_connect" fixture ctor

*** 1 failure is detected in the test module "Test Application"

such error I see when trying to call next method:

app_dir = graphene::utilities::temp_directory_path();

It means next:

BOOST_FIXTURE_TEST_CASE( cli_connect, cli_fixture ) - this Boost fixture cannot call cli_fixture constructor with graphene::utilities::temp_directory_path() method inside, maybe I'm doing something wrong ? Still trying to understand the reason and to code as we desire !

This comment has been minimized.

@cogutvalera

cogutvalera Aug 27, 2018

Member

fixed by direct initialization instead of using assignment operators inside constructor body

// upgrade nathan
BOOST_TEST_MESSAGE("Upgrading Nathan to LTM");
upgrade_tx = con->wallet_api_ptr->upgrade_account("nathan", true);
nathan_acct_after_upgrade = con->wallet_api_ptr->get_account("nathan");

This comment has been minimized.

@abitmore

abitmore Aug 23, 2018

Member

The upgrade of nathan account can be abstracted to a single test case, then be invoked by other test cases. See this for example:

INVOKE(create_account_test);

This comment has been minimized.

@cogutvalera

cogutvalera Aug 24, 2018

Member

will do it soon

This comment has been minimized.

@cogutvalera

cogutvalera Aug 24, 2018

Member

INVOKE doesn't work well enough with test fixtures, also need to fight here with errors

cogutvalera added some commits Aug 24, 2018

using direct initialization, which is more efficient than using assig…
…nment operators inside the constructor body
@cogutvalera

This comment has been minimized.

Member

cogutvalera commented Aug 27, 2018

guys, when you will have enough time check please my new changes, I've committed all required changes that were asked by @abitmore , hope all was done correctly enough, I've compiled and tested and it seems work absolutely as intended.

Thanks a lot !

@abitmore

Looks much better now. Thanks!

// create a new account
graphene::wallet::brain_key_info bki = con.wallet_api_ptr->suggest_brain_key();
BOOST_CHECK(!bki.brain_priv_key.empty());
signed_transaction create_acct_tx = con.wallet_api_ptr->create_account_with_brain_key(
bki.brain_priv_key, "jmjatlanta", "nathan", "nathan", true);
signed_transaction create_acct_tx = con.wallet_api_ptr->create_account_with_brain_key(bki.brain_priv_key, "jmjatlanta", "nathan", "nathan", true);

This comment has been minimized.

@abitmore

abitmore Aug 27, 2018

Member

Apparently your screen is wider than mine. I like the wrapped format, at max around 130 characters a line.

This comment has been minimized.

@cogutvalera

cogutvalera Aug 27, 2018

Member

ok sure ! I will format the whole file right now as required !

nathan_acct_after_upgrade = con.wallet_api_ptr->get_account("nathan");
// verify that the upgrade was successful
BOOST_CHECK_PREDICATE( std::not_equal_to<uint32_t>(), (nathan_acct_before_upgrade.membership_expiration_date.sec_since_epoch())(nathan_acct_after_upgrade.membership_expiration_date.sec_since_epoch()) );

This comment has been minimized.

@abitmore

abitmore Aug 27, 2018

Member

This line is too long to fit in my screen as well.

By the way there were some long lines that didn't change, so I'm unable to comment on them. Please help wrap them as well.

This comment has been minimized.

@cogutvalera

cogutvalera Aug 27, 2018

Member

ok sure !

std::vector<std::string> nathan_keys;
cli_fixture() :
server_port_number(0),

This comment has been minimized.

@abitmore

abitmore Aug 27, 2018

Member

I think we can use some indentations here.

This comment has been minimized.

@cogutvalera

cogutvalera Aug 27, 2018

Member

ok sure !

Feature release (201810) automation moved this from In progress to In Testing Aug 27, 2018

@abitmore

Looks good to me so far. Waiting for CI reports.

@cogutvalera

This comment has been minimized.

Member

cogutvalera commented Aug 27, 2018

Thank you !

@abitmore

This comment has been minimized.

Member

abitmore commented Aug 27, 2018

@oxarbitrage

This comment has been minimized.

Member

oxarbitrage commented Aug 27, 2018

i think it is looking great but i will suggest the following if possible:

the "create a new account", "save the private key for this new account in the wallet file" and "attempt to give jmjatlanta some bitsahres" are pretty common actions. maybe create test createaccount and add the 3 things there. then we can invoke this new test after upgrade_nathan_account.

for the new test to make sense you can add some checks on it like if the account was created and if the transfer went ok.

@cogutvalera

This comment has been minimized.

Member

cogutvalera commented Aug 28, 2018

@oxarbitrage thank you very much ! Your idea is great and makes sense ! I think I need to code it too. Is anybody disagree ?

Feature release (201810) automation moved this from In Testing to In progress Aug 28, 2018

// attempt to give jmjatlanta some bitsahres
BOOST_TEST_MESSAGE("Transferring bitshares from Nathan to jmjatlanta");
for(int i = 1; i <= 200; i++)
for(int i = 1; i <= 199; i++)

This comment has been minimized.

@abitmore

abitmore Aug 28, 2018

Member

Why this change? Something was broken?

This comment has been minimized.

@cogutvalera

cogutvalera Aug 28, 2018

Member

nothing was broken, I've changed this because we have +1 transaction inside INVOKE create_new_account, and I've decided to decrease here history by -1 transaction. Should I change another way ? I can change BOOST_CHECK history transfers to 202 as an example here

BOOST_CHECK_EQUAL(201, history.size() );

what are your thoughts ? Thanks !

This comment has been minimized.

@abitmore

abitmore Aug 28, 2018

Member

I've changed this because we have +1 transaction inside INVOKE create_new_account, and I've decided to decrease here history by -1 transaction.

Makes sense. Thanks.

@pmconrad

This comment has been minimized.

pmconrad commented Aug 28, 2018

Looks good, but haven't checked in depth.

@oxarbitrage

looks good to me too, should be easier to add more tests here with the new framework. thanks.

Feature release (201810) automation moved this from In progress to In Testing Aug 28, 2018

@cogutvalera

This comment has been minimized.

Member

cogutvalera commented Aug 28, 2018

Thank you guys !

@abitmore abitmore merged commit 0ad21dc into bitshares:develop Aug 29, 2018

2 checks passed

ci/dockercloud Your tests passed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Feature release (201810) automation moved this from In Testing to Done Aug 29, 2018

@cogutvalera

This comment has been minimized.

Member

cogutvalera commented Aug 29, 2018

Thanks for merge !

@ryanRfox ryanRfox moved this from In Testing to Ready for Integration in Community Claims Sep 4, 2018

@ryanRfox ryanRfox removed this from Ready for Integration in Community Claims Sep 10, 2018

@oxarbitrage oxarbitrage referenced this pull request Sep 28, 2018

Closed

Refactor cli_test #1192

2 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment