Updater unit tests by lght · Pull Request #6913 · openethereum/parity-ethereum

lght · 2017-10-27T17:19:21Z

Updater Unit Tests

The main point of this PR is to provide basic unit tests, covering all members of the Updater.

Through the process of writing tests, TestBlockChainClient is also undergoing some pretty heavy refactoring. The test client will now use an in-memory blockchain instead of the current HashMap.

The Operations contract is the central feature of Updater. Contracts are executed locally using Parity's EVM interpreter. I still need to write unit tests exercising the smart contract functionality.

Integration tests will target the Kovan test network. One cool possibility would be to make the tests generic across all available testnets. Pluggable consensus and spec configuration should make this pretty straightforward. Integration tests will likely go in their own PR.

Victories

All tests pass!

Shortcomings

Current tests are stupid simple, and test the setup of Updater more than anything else
Complex tests for the Operations contract are needed

TODO

Unit tests are still needed for:

deploying Operations contract to in-memory blockchain
pushing/executing transactions for relevant parts of the Operations contract

TestBlockChainClient needs to be refactored to use its in-memory blockchain for all block operations, instead of relying on the current HashMap-based mock blockchain.

There are a couple other things I want to do, but fall outside the scope of this pull request.

Write integration tests targeting Kovan (eventually all testnets)

Create a standalone test crate:

There are a number of test helpers useful across crates
Rust doesn't enable sharing structs directly from another crate's tests module
devtools started (in-part?) to address this issue
- fairly bare-bones, needs feature enhancement, additional structs and restructuring
- may be able to lower (re)compilation overhead by messing w/ file structure
The current method for using test helpers in other crates is to:
- split them into their own module
- export with pub use/mod in the lib.rs of the crate
Splitting off useful test helpers may turn out to be the best way to go
- less fighting with Rust build tools
- test docs could include a readme for how test helpers are structured
- the current method is definitely more modular
- naming and structuring idiosyncracies could become an issue
A standalone test crate would be a powerful tool unto itself
- similar to ethrun using parity-ethabi, a fully-featured devtools could be utilized by 3rd-party projects

A hybrid of the current approach and a standalone crate may be possible. I'm thinking this might look like each Parity crate having its own publicly accessible test helper crate. To save on compilation overhead, the crates could be hidden behind feature flags. Then a separate, root-level crate (devtools?) could import all the public test crates from each Parity component.

Not sure if this hybrid approach would actually work, or is a total pipedream. Thoughts?

This change is

parity-cla-bot · 2017-10-27T17:19:23Z

It looks like @lght hasn'signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, plesae reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

NikVolf · 2017-10-27T17:24:56Z

why commits from @arkpar are included in this pr?
rebase to master probably?

lght · 2017-10-27T17:25:02Z

[clabot:check]

parity-cla-bot · 2017-10-27T17:25:04Z

It looks like @lght signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

lght · 2017-10-27T17:26:34Z

@NikVolf I know, I tried to roll back to master before that contribution. Still not sure how the two commits got rolled up together.

Any recs to detangle that commit?

NikVolf · 2017-10-27T17:31:59Z

just merge latest master, reset soft to it, then add, commit and force push
something like this, just make sure you backup the branch though :)

git fetch
git merge origin/master
git reset --soft origin/master
git commit -a -m "Updater unit tests"
git push origin HEAD --force

NikVolf · 2017-10-27T17:33:35Z

oh a since it's a fork, make sure your master is the same as paritytech::master

Signed-off-by: lght <cole@parity.io>

lght · 2017-10-27T17:49:27Z

woot! that worked, thanks so much :) 👍

Signed-off-by: lght <cole@parity.io>

rphmeier · 2017-10-27T21:17:51Z

 	}
 }

+pub struct NoopDBRestore;


add a doc comment

just saw this. added doc comments to that struct and the following trait.

was planning to move the split-out test classes shared across crates to a central parity-test crate. think this would be a worthwhile change, but wanted to consult the team before starting in on the work. what are your thoughts on taking this approach?

Signed-off-by: lght <cole@parity.io>

First steps to implementing calling into the local Operations contract from TestBlockChainClient Signed-off-by: lght <cole@parity.io>

Signed-off-by: lght <cole@parity.io> [fix] UpdaterState set in test constructor, tests::info passes Signed-off-by: lght <cole@parity.io> [fix] All unit tests passing, using kvdb_memory-backed blockchain Signed-off-by: lght <cole@parity.io> [minor fix] corrected path to test helper Signed-off-by: lght <cole@parity.io> [minor fixes] dependency refactors and new Spec::new_test_kovan() Signed-off-by: lght <cole@parity.io> [minor fix] ethcore::snapshot::account test dependency fix Signed-off-by: lght <cole@parity.io> [fix] Dependency issue finally fixed Signed-off-by: lght <cole@parity.io>

Signed-off-by: lght <cole@parity.io>

Operations.sol and derived files under updater/res were added accidentally, and only used for local analysis purposes. Signed-off-by: lght <cole@parity.io>

5chdn · 2017-11-28T13:40:24Z

9 tests failed.

lght · 2017-11-28T21:39:49Z

Yeah, I noticed that too. 👎

Those are all tests for rpc, which are not part of this PR. Think those errors came over from a merge with master. ~~~If those tests are fixed in main, I can pull down the changes and merge with this PR.~~~

Pulled down the latest master, and this problem still exists.

From latest CI run:

failures:
    v1::tests::mocked::eth::rpc_eth_call
    v1::tests::mocked::eth::rpc_eth_call_default_block
    v1::tests::mocked::eth::rpc_eth_call_latest
    v1::tests::mocked::parity::rpc_parity_call
    v1::tests::mocked::traces::rpc_trace_call
    v1::tests::mocked::traces::rpc_trace_call_state_pruned
    v1::tests::mocked::traces::rpc_trace_multi_call
    v1::tests::mocked::traces::rpc_trace_raw_transaction
    v1::tests::mocked::traces::rpc_trace_raw_transaction_state_pruned

test result: FAILED. 264 passed; 9 failed; 4 ignored; 0 measured; 0 filtered out

error: test failed, to rerun pass '--lib'
ERROR: Job failed: exit code 1

tomusdrw

Grumbles

tomusdrw · 2017-11-30T10:43:50Z

+    // against a real testnet.
+
+    //#[test]
+    //fn operations_call_clients_required() {


Commented code?

tomusdrw · 2017-11-30T10:44:09Z

-				return;
-			}
-		}
+        if self.operations.lock().is_none() {


Indentation issues.

tomusdrw · 2017-11-30T10:47:54Z

+	manifest: Option<ManifestData>,
+	chunks: HashMap<H256, Bytes>,
+
+	restoration_manifest: Mutex<Option<ManifestData>>,


Might be simple to have a:

struct Wrapped(Mutex<TestSnapshotService>); impl SnapshotService for Wrapped { }

instead of maintaining all those Mutexes/locks here.

That would be simpler. ~~~Since this PR is already fairly large, can we save that change for another PR to refactor TestSnapshotSevice?~~~ Going to put my focus into refactoring TestSnapshotService to merge this PR as quickly as possible since it is holding #6896.

There are a number of refactors that will need to happen after #6896 is merged. I would prefer to save further refactors for TestSnapshotService until after #6896 and #7038 are merged.

Update: Problems with jsonrpc tests brought up another issue with TestSnapshotService. I'll try to resolve the issues for the RPC API, and see how much refactoring will be necessary to replace all instances of TestSnapshotService with Wrapped(Mutex<TestSnapshotService>).

tomusdrw · 2017-11-30T10:48:10Z

 		load_bundled!("ethereum/olympic")
 	}
+
+    /// Create a new spec for Operations contract


Indentation.

tomusdrw · 2017-11-30T10:48:15Z

 	}

+    /// Create a new Spec with Kovan testnet using the AuthorityRound consensuse engine (not requiring work).
+    pub fn new_test_kovan() -> Self {


Indentation.

tomusdrw · 2017-11-30T10:55:23Z

+
+    /// Convenience method to retrieve current state from a given state_db
+    pub fn get_in_memory_state(&self) -> Result<State<StateDB>, EvmTestError> {
+        {


Additional block unnecessary.

tomusdrw · 2017-11-30T10:55:41Z

+
+    /// Convenience method to retrieve state from a fresh state_db
+    pub fn get_new_state(&self) -> Result<State<StateDB>, EvmTestError> {
+        let (_, state_db) = get_in_memory_db(&self.spec).unwrap();


why unwrap and not ? seems that the error type is the same.

tomusdrw · 2017-11-30T10:56:56Z

+	}
+
+    /// Convenience method to retrieve current state from a given state_db
+    pub fn get_in_memory_state(&self) -> Result<State<StateDB>, EvmTestError> {


Avoid get in function names use in_memory_state is it's a getter or new_/create_ if you are creating something.

tomusdrw · 2017-11-30T10:59:46Z

  script:
    - set RUST_BACKTRACE=1
-    - echo cargo test --features json-tests -p rlp -p ethash -p ethcore -p ethcore-bigint -p parity-dapps -p parity-rpc -p ethcore-util -p ethcore-network -p ethcore-io -p ethkey -p ethstore -p ethsync -p ethcore-ipc -p ethcore-ipc-tests -p ethcore-ipc-nano -p parity-rpc-client -p parity %CARGOFLAGS% --verbose --release
+    - echo cargo test --features json-tests -p rlp -p ethash -p ethcore -p ethcore-bigint -p parity-dapps -p parity-rpc -p ethcore-util -p ethcore-network -p ethcore-io -p ethkey -p ethstore -p ethsync -p ethcore-ipc -p ethcore-ipc-tests -p ethcore-ipc-nano -p parity-rpc-client -p parity -p parity-updater %CARGOFLAGS% --verbose --release


Should go to workspace members, Why does .gitlab-ci-.yml list packages names here? CC @paritytech/ui-devs
Should only run cargo test --all

I'll change it to cargo test --all, and revert to original if there are merge issues.

tomusdrw · 2017-11-30T11:00:48Z

+	// Write DB
+	{
+		let mut batch = kvdb::DBTransaction::new();
+		state_db.journal_under(&mut batch, 0, &genesis.hash())?;


state_db is empty at this point, no? I don't think there is a need to write anything to the DB.

Unsure, I took this from the existing code, and modified it to use the new state_db setup.

I don't think there is a need to write anything to the DB.

Why wouldn't we need to add the genesis hash to the DB when it is created?

tomusdrw · 2017-12-21T08:58:32Z

@lght any progress on this one?

lght · 2018-01-15T18:16:52Z


Will also write up the auto-updater Truffle tests, and remove the commented tests from this PR. Will separate the Truffle/Operations.sol-specific tests into their own PR.

Formatting changes for openethereum#6913

Part of an attempt to get updater unit tests running on automated builds for openethereum#6913.

Removed commented tests from `updater/src/updater.rs`, and will migrate them to a separate PR dedicated to unit tests for Operations.sol. Changed `test_client::get_in_memory_db` to `test_client::create_in_memory_db` to be more semantically correction, and follow better naming convention.

lght · 2018-01-15T21:35:03Z

RPC tests are causing this PR's builds to fail. Changes to RPC originated from a merge with past master. Looks like the tests use a snapshot_service() that returns a wrapped TestSnapshotService. Debugging the failures now.

Part of openethereum#6913, JSON RPC tests from a merge with previous master were causing CI builds to fail.

More formatting fixes for openethereum#6913

debris · 2018-02-06T10:59:54Z

please reopen if/when the pr is ready

lght added P5-sometimesoon 🌲 Issue is worth doing soon. A0-pleasereview 🤓 Pull request needs code review. labels Oct 27, 2017

NikVolf added M4-core ⛓ Core client code / Rust. and removed P5-sometimesoon 🌲 Issue is worth doing soon. labels Oct 27, 2017

Updater unit tests

0083982

Signed-off-by: lght <cole@parity.io>

lght force-pushed the updater-unit-tests branch from 95a19f1 to 0083982 Compare October 27, 2017 17:48

lght added 3 commits October 27, 2017 12:23

Updater tests: minor fixes

6c57666

Signed-off-by: lght <cole@parity.io>

build fix: incorrectly referenced test::helpers

caee363

Signed-off-by: lght <cole@parity.io>

CI: added updater tests to rust-stable tests

d0516b0

Signed-off-by: lght <cole@parity.io>

rphmeier reviewed Oct 27, 2017

View reviewed changes

lght added 2 commits October 27, 2017 18:01

[CI] minor fix: updater package name is parity-updater

e45cfb5

Signed-off-by: lght <cole@parity.io>

[doc] comments describing NoopDBRestore stub struct

20d28c1

Signed-off-by: lght <cole@parity.io>

5chdn added this to the 1.9 milestone Oct 29, 2017

axelchalon mentioned this pull request Oct 30, 2017

Refactor updater to use ethabi-derive #6896

Closed

lght added 2 commits October 31, 2017 15:28

[refactor] Implement calling Operations contract

2013584

First steps to implementing calling into the local Operations contract from TestBlockChainClient Signed-off-by: lght <cole@parity.io>

lght force-pushed the updater-unit-tests branch from 967399c to eb9da9b Compare November 2, 2017 02:39

lght removed the A0-pleasereview 🤓 Pull request needs code review. label Nov 2, 2017

5chdn added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Nov 2, 2017

lght added 2 commits November 2, 2017 18:57

[refactor] TestBlockChainClient w/ in-memory kvdb

03d809b

Signed-off-by: lght <cole@parity.io>

Merge branch 'master' into updater-unit-tests

b55d5c5

lght force-pushed the updater-unit-tests branch from b2ff878 to b55d5c5 Compare November 3, 2017 01:52

lght added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Nov 9, 2017

Remove Updater Resources

96e2ea7

Operations.sol and derived files under updater/res were added accidentally, and only used for local analysis purposes. Signed-off-by: lght <cole@parity.io>

lght added 2 commits November 28, 2017 14:58

Merge branch 'master' into updater-unit-tests

15fde3f

Fixed import in test_sync

62003c3

tomusdrw reviewed Nov 30, 2017

View reviewed changes

tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 30, 2017

5chdn added the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Dec 30, 2017

5chdn modified the milestones: 1.9, 1.10 Dec 30, 2017

5chdn removed the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Jan 9, 2018

lght added a commit to lght/parity that referenced this pull request Jan 15, 2018

Fix indentation

87be7e5

Formatting changes for openethereum#6913

Indentation fixes

842ef40

Formatting changes for openethereum#6913

lght force-pushed the updater-unit-tests branch from 87be7e5 to 842ef40 Compare January 15, 2018 20:04

lght added 2 commits January 15, 2018 20:09

Change gitlab windows build to run all tests

658c69d

Part of an attempt to get updater unit tests running on automated builds for openethereum#6913.

lght added 2 commits January 15, 2018 21:37

Remove json tests from Windows CI build

9acb486

Part of openethereum#6913, JSON RPC tests from a merge with previous master were causing CI builds to fail.

Indentation fixes untrusted_rlp

98f2a87

More formatting fixes for openethereum#6913

5chdn modified the milestones: 1.10, 1.11 Jan 23, 2018

debris closed this Feb 6, 2018

Conversation

lght commented Oct 27, 2017 • edited by 5chdn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Updater Unit Tests

Victories

Shortcomings

TODO

Uh oh!

parity-cla-bot commented Oct 27, 2017

Uh oh!

NikVolf commented Oct 27, 2017

Uh oh!

lght commented Oct 27, 2017

Uh oh!

parity-cla-bot commented Oct 27, 2017

Uh oh!

lght commented Oct 27, 2017

Uh oh!

NikVolf commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NikVolf commented Oct 27, 2017

Uh oh!

lght commented Oct 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

5chdn commented Nov 28, 2017

Uh oh!

lght commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lght Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomusdrw commented Dec 21, 2017

Uh oh!

lght commented Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lght commented Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

debris commented Feb 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

lght commented Oct 27, 2017 •

edited by 5chdn

Loading

NikVolf commented Oct 27, 2017 •

edited

Loading

lght commented Nov 28, 2017 •

edited

Loading

lght Jan 15, 2018 •

edited

Loading

lght commented Jan 15, 2018 •

edited

Loading

lght commented Jan 15, 2018 •

edited

Loading