[test] Small unit test improvements, including helper to make mempool transaction#21121
Conversation
|
the failing test seems unrelated. The failure is in |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
src/util/time.h
Outdated
There was a problem hiding this comment.
Probably more of a hassle, but another way to document would be to use std::chrono types with explicit units.
There was a problem hiding this comment.
yeah, I took a quick look at changing the signature to a chrono type, but ended up taking the efficient/lazy way for now. there are a couple tweaks that would need to be made to switch it over, nothing difficult but not currently at the top of my priority list
so I thought leaving a comment was the smallest way to help :)
There was a problem hiding this comment.
in the latest push, I updated the function signature of GetMockTime, with updated call sites. I also overloaded SetMockTime to take in chronos. This does mean I introduced another function that doesn't get used until #21061, so I can remove if you'd prefer.
|
Code review ACK e7573304b7e112b6b7f49c79c25fce36a5440209 The new function is added but not used (so not tested), I would prefer if it was, but I guess it can wait until #21061. |
|
@vasild- thanks for the review! I've taken all your suggestions locally, but I'm currently getting a linker error when trying to |
|
@amitiuttarwar, I see no boost code is used in |
|
It is only allowed to use boost in the unit tests, not the fuzz tests (or other tests). setup_common is used by all test and bench libraries. |
amitiuttarwar
force-pushed
the
2021-01-unit-test-valid-tx
branch
from
e757330 to
167f442
Compare
|
ah interesting. used took all the feedback.
this means that in this commit, I introduce |
amitiuttarwar
force-pushed
the
2021-01-unit-test-valid-tx
branch
from
167f442 to
82de557
Compare
|
fixed a silent merge conflict since the function signature of ATMP has changed. updated #21061 to use e020cd6 to test the helper. |
amitiuttarwar
force-pushed
the
2021-01-unit-test-valid-tx
branch
from
82de557 to
903bbf7
Compare
|
updated to incorporate feedback from @vasild |
|
ACK 903bbf7f1068da0c27b99401483444f037a17840 |
If the miner code is faulty and does not include any transactions in a block, the code segfaults when it tries to access block transactions. Instead, add a check that safely aborts the process.
amitiuttarwar
force-pushed
the
2021-01-unit-test-valid-tx
branch
from
903bbf7 to
1363b6c
Compare
|
rebased |
|
ACK 1363b6c |
…elper to make mempool transaction 1363b6c [doc / util] Use comments to clarify time unit for int64_t type. (Amiti Uttarwar) 47a7a16 [util] Introduce a SetMockTime that takes chrono time (Amiti Uttarwar) df6a5fc [util] Change GetMockTime to return chrono type instead of int (Amiti Uttarwar) a2d908e [test] Throw error instead of segfaulting in failure scenario (Amiti Uttarwar) 9a3bbe8 [test] Introduce a unit test helper to create a valid mempool transaction. (Amiti Uttarwar) Pull request description: Some miscellaneous improvements that came up when working on bitcoin#21061 - The first commit is a helper to make valid mempool transactions & submit via ATMP. Introducing in this PR, using in bitcoin#21061. - The second commit is a small improvement in `miner_tests.cpp` that uses `BOOST_REQUIRE_EQUAL` to properly terminate the program instead of segfaulting in the failure scenario where the blocks do not include the expected number of transactions. - The third commit changes the function signature of `GetMockTime()` to return a chrono type. - The fourth & fifth commit overload `SetMockTime` to also accept chrono type, and adds documentation to indicate that the `int64_t` function signature is deprecated. ACKs for top commit: vasild: ACK 1363b6c Tree-SHA512: c72574d73668ea04ee4c33858f8de68b368780f445e05afb569aaf8564093f8112259b3afe93cf6dc2ee12a1ab5af1130ac73c16416132c1ba2851c054a67d78
| m_node.mempool->addUnchecked(entry.Fee(50000).Time(GetTime()).SpendsCoinbase(false).FromTx(tx)); | ||
|
|
||
| std::unique_ptr<CBlockTemplate> pblocktemplate = AssemblerForTest(chainparams).CreateNewBlock(scriptPubKey); | ||
| BOOST_REQUIRE_EQUAL(pblocktemplate->block.vtx.size(), 4); |
There was a problem hiding this comment.
Somehow this leads to a "comparison of integers of different signs" on bitcoinbuilds.org:
https://bitcoinbuilds.org/index.php?ansilog=982c61cf-6969-4001-bebc-dc215e5d29a4.log#l1906
Maybe only happening on older boost versions?
bedb8d8 Avoid comparision of integers with different signs (Jonas Schnelli) Pull request description: Fixes an integer comparison of different signs (which errors out on `-Werror,-Wsign-compare`). Introduced in #21121. See https://bitcoinbuilds.org/index.php?ansilog=982c61cf-6969-4001-bebc-dc215e5d29a4.log ACKs for top commit: MarcoFalke: review ACK bedb8d8 amitiuttarwar: ACK bedb8d8 vasild: ACK bedb8d8 Tree-SHA512: cb22a6239a1fc9d0be5573bf6ae4ec379eb7398c88edc8fa2ae4fd721f37f9ca3724896c1ac16de14a5286888a0b631813da32cb62d177ffbf9b2c31e716a7aa
…nt signs bedb8d8 Avoid comparision of integers with different signs (Jonas Schnelli) Pull request description: Fixes an integer comparison of different signs (which errors out on `-Werror,-Wsign-compare`). Introduced in bitcoin#21121. See https://bitcoinbuilds.org/index.php?ansilog=982c61cf-6969-4001-bebc-dc215e5d29a4.log ACKs for top commit: MarcoFalke: review ACK bedb8d8 amitiuttarwar: ACK bedb8d8 vasild: ACK bedb8d8 Tree-SHA512: cb22a6239a1fc9d0be5573bf6ae4ec379eb7398c88edc8fa2ae4fd721f37f9ca3724896c1ac16de14a5286888a0b631813da32cb62d177ffbf9b2c31e716a7aa
6 participants
Some miscellaneous improvements that came up when working on #21061
miner_tests.cppthat usesBOOST_REQUIRE_EQUALto properly terminate the program instead of segfaulting in the failure scenario where the blocks do not include the expected number of transactions.GetMockTime()to return a chrono type.SetMockTimeto also accept chrono type, and adds documentation to indicate that theint64_tfunction signature is deprecated.