Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Additional ShIP unit tests #9253

Merged
merged 29 commits into from
Aug 19, 2020
Merged

Additional ShIP unit tests #9253

merged 29 commits into from
Aug 19, 2020

Conversation

victorj8
Copy link
Contributor

@victorj8 victorj8 commented Jun 25, 2020

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:

  • test_deltas_account

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.

  • test_traces_present

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)

  • global_property_history

This test spots onto the global_property field of table deltas.

  • protocol_feature_history

This tests spots onto the protocol_state field of table_deltas

  • contract_history

This test spots onto contract_table, contract_row and contract_index256

  • resources_history

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

  • Documentation
  • Stability bug fix
  • Other
  • Other - special case

Consensus Changes

  • Consensus Changes

API Changes

  • API Changes

Documentation Additions

  • Documentation Additions

Copy link
Contributor

@larryk85 larryk85 left a 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.
@victorj8 victorj8 requested a review from larryk85 August 18, 2020 20:31

scoped_temp_path state_history_dir;
fc::create_directories(state_history_dir.path);
state_history_traces_log log({ .log_dir = state_history_dir.path });
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants