Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Move snapshot related traits to their proper place #11012

Merged
merged 24 commits into from
Sep 10, 2019
Merged

Move snapshot related traits to their proper place #11012

merged 24 commits into from
Sep 10, 2019

Conversation

dvdplm
Copy link
Collaborator

@dvdplm dvdplm commented Aug 30, 2019 •
edited
Loading

This PR is a continuation of #11010 that puts SnapshotClient and SnapshotWriter back where they belong in the snapshot crate.
This move creates a tricky situation for tests, where the direct dependency of the tests on ethcore::Client (and other stuff from ethcore) cause the trait impls to be "lost".
As a work-around this PR moves all snapshot tests to their own crate, snapshot-tests.

Salient points:

  • moves the SnapshotClient and SnapshotWriter traits from client-traits to the snapshot crate where they belong
  • moves all tests from snapshot to snapshot-tests
  • there are a few places in the snapshot where types or methods have been made pub only to accommodate tests (e.g. here); in places where it makes sense to do so, I have used #[cfg(any(test, feature = "test-helpers"))] (e.g. here). This is unfortunate but I could not think of a better way.
  • to make cargo test --all actually work a crate has to be a workspace member or part of the dependency tree; for snapshot-tests this means it has to be listed in the [dev-dependencies] for snapshot.

Continuation of #11010

@dvdplm dvdplm mentioned this pull request Aug 30, 2019
Merged
@dvdplm dvdplm self-assigned this Aug 30, 2019
@dvdplm dvdplm added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels Aug 30, 2019
@dvdplm dvdplm added this to the 2.7 milestone Aug 30, 2019
@dvdplm
cleanup
caa2d2d
@dvdplm
Cleanup
145d77c
@dvdplm
Merge branch 'master' into dp/chore/extract-snapshot-with-traits-in-t…
c60a43f 
…he-right-place

* master:
  Extract snapshot to own crate (#11010)
  Edit publish-onchain.sh to use https (#11016)
  EIP 1108: Reduce alt_bn128 precompile gas costs (#11008)
  Fix deadlock in `network-devp2p` (#11013)
  Implement EIP-1283 reenable transition, EIP-1706 and EIP-2200 (#10191)
  EIP 1884 Re-pricing of trie-size dependent operations (#10992)
@dvdplm
fix merge issues
d6e0574
@dvdplm dvdplm marked this pull request as ready for review September 3, 2019 10:41
@dvdplm dvdplm added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Sep 3, 2019
ordian
ordian reviewed Sep 5, 2019
View reviewed changes
ethcore/snapshot/snapshot-tests/Cargo.toml Outdated Show resolved Hide resolved
ethcore/snapshot/snapshot-tests/src/helpers.rs Outdated Show resolved Hide resolved
ethcore/snapshot/snapshot-tests/src/lib.rs Outdated Show resolved Hide resolved
ethcore/snapshot/snapshot-tests/src/proof_of_authority.rs Outdated Show resolved Hide resolved
ethcore/snapshot/src/service.rs Show resolved Hide resolved
ethcore/snapshot/snapshot-tests/src/state.rs Outdated Show resolved Hide resolved
ethcore/snapshot/snapshot-tests/src/proof_of_work.rs Outdated Show resolved Hide resolved
ethcore/snapshot/snapshot-tests/src/service.rs Outdated Show resolved Hide resolved
@dvdplm
Merge branch 'dp/chore/extract-snapshot-with-traits-in-the-right-plac…
dcee086 
…e' of github.com:paritytech/parity-ethereum into dp/chore/extract-snapshot-with-traits-in-the-right-place

* 'dp/chore/extract-snapshot-with-traits-in-the-right-place' of github.com:paritytech/parity-ethereum:
  Update ethcore/snapshot/snapshot-tests/Cargo.toml
@dvdplm dvdplm added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 5, 2019
@dvdplm dvdplm added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Sep 6, 2019
@dvdplm dvdplm requested a review from ordian September 6, 2019 09:08
ordian
ordian approved these changes Sep 6, 2019
View reviewed changes
ethcore/snapshot/Cargo.toml
@@ -52,3 +52,9 @@ lazy_static = { version = "1.3" }
spec = { path = "../spec" }
tempdir = "0.3"
trie-standardmap = "0.15.0"
# Note[dvdplm]: Ensure the snapshot tests are included in the dependency tree, which in turn means that
# `cargo test --all` runs the tests.
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is unfortunate, but I didn't come up with something better

@dvdplm dvdplm added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 9, 2019
@dvdplm
Fix grumbles
78dac6a
@dvdplm
Merge branch 'master' into dp/chore/extract-snapshot-with-traits-in-t…
a21731c 
…he-right-place

* master:
  cleanup json crate (#11027)
  [spec] add istanbul test spec (#11033)
  [json-spec] make blake2 pricing spec more readable (#11034)
  Add blake2_f precompile (#11017)
  Add new line after writing block to hex file. (#10984)
  fix: remove unused error-chain (#11028)
  fix: remove needless use of itertools (#11029)
  Convert `std::test` benchmarks to use Criterion (#10999)
  Fix block detail updating (#11015)
  [trace] introduce trace failed to Ext (#11019)
  cli: update usage and version headers (#10924)
  [private-tx] remove unused rand (#11024)
@dvdplm dvdplm merged commit 48629c2 into master Sep 10, 2019
@dvdplm dvdplm deleted the dp/chore/extract-snapshot-with-traits-in-the-right-place branch September 10, 2019 20:44
dvdplm added a commit that referenced this pull request Sep 13, 2019
@dvdplm
Merge branch 'master' into dp/chore/new-ethereum-consensus-tests
bd0f53a 
* master: (70 commits)
  ethcore: remove `test-helper feat` from build (#11047)
  Include test-helpers from ethjson (#11045)
  [ethcore]: cleanup dependencies (#11043)
  add more tx tests (#11038)
  Fix parallel transactions race-condition (#10995)
  [ethcore]: make it compile without `test-helpers` feature (#11036)
  Benchmarks for block verification (#11035)
  Move snapshot related traits to their proper place (#11012)
  cleanup json crate (#11027)
  [spec] add istanbul test spec (#11033)
  [json-spec] make blake2 pricing spec more readable (#11034)
  Add blake2_f precompile (#11017)
  Add new line after writing block to hex file. (#10984)
  fix: remove unused error-chain (#11028)
  fix: remove needless use of itertools (#11029)
  Convert `std::test` benchmarks to use Criterion (#10999)
  Fix block detail updating (#11015)
  [trace] introduce trace failed to Ext (#11019)
  cli: update usage and version headers (#10924)
  [private-tx] remove unused rand (#11024)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@niklasad1 niklasad1 niklasad1 approved these changes

@ordian ordian ordian approved these changes

Assignees

@dvdplm dvdplm

Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
2.7
Development

Successfully merging this pull request may close these issues.
3 participants
@dvdplm @ordian @niklasad1