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

Expand unit test coverage #453

Closed
wants to merge 56 commits into from

Conversation

jmjatlanta
Copy link

@jmjatlanta jmjatlanta commented Jun 7, 2021

While not strictly "unit testing", more like mini system/integration testing, developers sometimes need to test their code alongside portions of the existing system. In the case that brought about this change, a new CryptoCondition needed to be tested in the context of the whole create transaction/spend transaction process.

testutils already existed to do some of this, but it did not seem to be used often. Existing functions were collected and adjusted to work inside a class. Now simple chain transactions can be tested by writing a gtest:

TestChain chain; // start a new chain

// Basic user wallets
auto notary = chain.AddWallet(chain.getNotaryKey());
auto alice = chain.AddWallet();
auto bob = chain.AddWallet();

CBlock lastBlock = chain.generateBlock(); // Genesis block

// turn on CryptoConditions
ASSETCHAINS_CC = 1;

{
    // create a transaction that gives some coin to Alice
    CValidationState returnState = notary->Transfer(alice, 100000);
    // verify everything worked
    EXPECT_TRUE( returnState.IsValid() );
    // mine a block
    lastBlock = chain.generateBlock(); // vtx[0] is unrelated, vtx[1] is ours
}
{
    // notary gives some coin to Bob, with the stipulation that he pay some kind of miner fee
    auto notaryPrevOut = notary->GetAvailable(200000);
    CMutableTransaction tx;
    CTxIn notaryIn;
    notaryIn.prevout.hash = notaryPrevOut.first.GetHash();
    notaryIn.prevout.n = notaryPrevOut.second;
    tx.vin.push_back(notaryIn);
    // give some coin to bob, but he must pay something to the miners
    CC* bobMustPayFee = MakeCCcond1(EVAL_INCLMINERFEE, bob->GetPubKey());
    CTxOut bobOut;
    bobOut.scriptPubKey = CCPubKey(bobMustPayFee);
    bobOut.nValue = 200000;
    tx.vout.push_back(bobOut);
    // give the rest back to the notary
    CTxOut notaryOut;
    notaryOut.scriptPubKey = GetScriptForDestination(notary->GetPubKey());
    notaryOut.nValue = notaryPrevOut.first.vout[1].nValue - 200000;
    tx.vout.push_back(notaryOut);
    // sign it
    uint256 hash = SignatureHash(notaryPrevOut.first.vout[1].scriptPubKey, tx, 0, SIGHASH_ALL, 0, 0);
    tx.vin[0].scriptSig << notary->Sign(hash, SIGHASH_ALL);
    // send it to the mempool
    CTransaction fundBob(tx);
    CValidationState returnState = chain.acceptTx(fundBob);
    // verify everything worked
    EXPECT_TRUE( returnState.IsValid() );
    // mine a block
    CBlock currentBlock = chain.generateBlock();
    // the wallet doesn't understand these transactions, so add this vout to Bob's wallet manually
    bob->AddOut(currentBlock.vtx[1], 0);
    cc_free(bobMustPayFee);
}

@jmjatlanta jmjatlanta marked this pull request as ready for review June 8, 2021 14:24
@jmjatlanta jmjatlanta changed the title Wrap test chain funcs into class [WIP] Wrap test chain funcs into class Oct 28, 2021
@jmjatlanta jmjatlanta changed the title [WIP] Wrap test chain funcs into class [WIP] Expand unit test coverage Oct 28, 2021
@jmjatlanta
Copy link
Author

In the commits above I have migrated various zcash and bitcoin tests to include them in the run of komodo-tests

@dimxy
Copy link
Collaborator

dimxy commented Jun 15, 2022

this should be rebased to include the S6 updates

#define _COINBASE_MATURITY 100
int COINBASE_MATURITY = _COINBASE_MATURITY;//100;
unsigned int WITNESS_CACHE_SIZE = _COINBASE_MATURITY+10;
unsigned int WITNESS_CACHE_SIZE = 100+10; // coinbase maturity plus 10
Copy link
Collaborator

@dimxy dimxy Jun 15, 2022

Choose a reason for hiding this comment

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

could it be a named constant instead of '100'?

Copy link
Author

Choose a reason for hiding this comment

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

I would like to replace it with something other than a global. It is only used in wallet functions, so I put it there. See 8db798b

@@ -78,7 +77,7 @@ static const bool DEFAULT_ALERTS = true;
/** Minimum alert priority for enabling safe mode. */
static const int ALERT_PRIORITY_SAFE_MODE = 4000;
/** Maximum reorg length we will accept before we shut down and alert the user. */
static unsigned int MAX_REORG_LENGTH = _COINBASE_MATURITY - 1;
static unsigned int MAX_REORG_LENGTH = 100 - 1; // based on COINBASE_MATURITY
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we could use a named constant here (isn't it originalCoinbaseMaturity)?

Copy link
Author

Choose a reason for hiding this comment

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

Removed MAX_REORG_LENGTH, see comment above


# tool for generating our public parameters
komodo_gtest_SOURCES = \
zcash_gtest_SOURCES = \
Copy link
Collaborator

Choose a reason for hiding this comment

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

aha, renamed to zcash_gtest as in fact it tests just zcash features

@tonymorony
Copy link

these changes implemented in combined PR: #559

@tonymorony tonymorony closed this Sep 5, 2022
who-biz pushed a commit to who-biz/komodo that referenced this pull request Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants