-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For test_deltas_account
, I would suggest splitting this up into smaller tests. The test as it is, is quite large and is covering many invariants. Each one of these tests should have one specific thing that they are concerned with and we should exhaustively test that. I understand the wanting to check all account stuff in one test. But, splitting it up will make it easier to audit and with simple descriptions as to what the invariants of the test are (this one is purely a suggestion).
There is also a fair amount of code duplication and could be factored out as most of these tests share the same form.
On line 350 (the WA test), we should explicitly call out this test to ensure this acts as a regression test (it highlights a bug). We should also test the same pattern for R1 and K1 keys to ensure they serialize and deserialize correctly through the delta.
chain.produce_blocks()
and chain.produce_blocks(1)
can be replaced by chain.produce_block()
.
Line 439 this should be BOOST_REQUIRE_EQUAL( traces.size(), 1 );
Around line 569, you are testing the indices from the action call. You need to check both indices based on the test smart contract.
- Split up the test_deltas_account into smaller tests. - Refactored some common code regarding the search for the table_deltas and deserialization of data. - Testing correct deserialization of R1 and K1 public keys - Replaced chain.produce_blocks() and chain.produce_blocks(1) with chain.produce_block() - Added test for BOOST_REQUIRE_EQUAL( traces.size(), 1 ); - Both indices for contract_index256 now being tested.
unittests/state_history_tests.cpp
Outdated
|
||
scoped_temp_path state_history_dir; | ||
fc::create_directories(state_history_dir.path); | ||
state_history_traces_log log({ .log_dir = state_history_dir.path }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
designated initializers are a C++20 feature. I will preemptively agree that most compilers at this point support it, but it is still considered an extension until C++20.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Change Description
Additional SHiP Plugin unit tests
Here are some unit tests for SHiP plugin, most of them related to spotting onto the correct serialization and deserialization of table deltas, the description of the tests is here:
This test tracks state deltas involving creating a new object in the DB, spotting onto fields such as account, account_metadata, permission and permission_link fields, also correct serialization of a WA key is tested (see issue #9087), the permission is deleted and the deltas are also verified.
This test creates an account and then it verifies that the traces include the expected transaction. Also a test for onblock is done here (see PR GH-9159)
This test spots onto the global_property field of table deltas.
This tests spots onto the protocol_state field of table_deltas
This test spots onto contract_table, contract_row and contract_index256
This test spots onto resource_limits and resource_usage fields. NOTE: the code to spot on is currently commented due to the serialization code hitting an assert, this is going to be worked on as part of another issue.
Change Type
Select ONE
Consensus Changes
API Changes
Documentation Additions