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

Update JSON tests to d4f86ecf4aa7c #11054

Merged
merged 30 commits into from
Sep 25, 2019
Merged

Update JSON tests to d4f86ecf4aa7c #11054

merged 30 commits into from
Sep 25, 2019

Conversation

dvdplm
Copy link
Collaborator

@dvdplm dvdplm commented Sep 13, 2019 •
edited by ordian
Loading

This PR started out as an attempt to finish #10923 but ended up being something else.

Questions for reviewers:

  • having two different mechanisms for skipping tests is confusing: what's a better way? Skipping whole test files is easy but coarse; skipping test-by-test is very cumbersome but precise. Do we need both?
  • currently one test-skipping mechanism is hidden behind a feature (ci-skip-tests). Is this really what we want?
  • what else can we do to make maintenance of json tests less painful than it currently is?

Open issues for fixing the failing tests:

Previous description, now outdated:

This PR reverts the controversial changes of the previous PR and skips the failing tests.

Maybe I misunderstand the suggested workaround of putting the fix under #[cfg(test)] but it seems odd to run different code in production than we run in tests.
Instead here I suggest we skip the failing tests with the argument that we do not wish to fix this issue (at least not at this time) because it does not affect us. If I am wrong, and I likely am, I look
forward to hearing why and what a better approach to updating the state tests is.

We need to update the state tests again very soon for Istanbul so there's no avoiding this discussion.

Branched off #10923

ref #10908

debris and others added 3 commits July 28, 2019 18:10
@debris
new ethereum consensus tests, #10908
b745fa3
@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)
  ...
@dvdplm
Update JSON tests to 725dbc73a
9339d0d 
This PR reverts the controversial changes of the previous PR and skips the failing tests.

Maybe I misunderstand the suggested workaround of putting the fix under `#[cfg(test)]` but it seems odd to run different code in production than we run in tests. Instead here I suggest we skip the failing tests with the argument that we do not wish to fix this issue (at least not at this time) because it does not affect us. If I am wrong, and I likely am, I look forward to hearing why and what a better approach to updating the state tests is.

Branched off #10923

ref #10908
@dvdplm dvdplm requested review from sorpaas, cheme and debris September 13, 2019 19:21
@dvdplm dvdplm self-assigned this Sep 13, 2019
@dvdplm dvdplm added A0-pleasereview 🤓 Pull request needs code review. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 F4-tests 💻 Tests need fixing, improving or augmenting. P2-asap 🌊 No need to stop dead in your tracks, however issue should be addressed as soon as possible. labels Sep 13, 2019
@dvdplm dvdplm requested review from niklasad1 and ordian September 13, 2019 19:25
jam10o-new
jam10o-new previously approved these changes Sep 14, 2019
View reviewed changes
@niklasad1
Copy link
Collaborator

niklasad1 commented Sep 14, 2019 •
edited
Loading

@dvdplm

I got a bit confused because of title update JSON tests to 725dbc73a

725dbc73a is used already on master and is really old?

Is that correct?

In general, I agree with the change in skipping the tests rather than changing production code but I'm that not familiar with these tests TBH.

@dvdplm
Copy link
Collaborator Author

dvdplm commented Sep 16, 2019

725dbc73a is used already on master and is really old?

@niklasad1 Yes it's the same as master. I must have done something wrong when branching off #10923

Good catch!

@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 16, 2019
@dvdplm dvdplm changed the title Update JSON tests to 725dbc73a Update JSON tests to 1dc9d20e97165708f7db0bbf2d1a87a6b4285827 Sep 18, 2019
@dvdplm dvdplm changed the title Update JSON tests to 1dc9d20e97165708f7db0bbf2d1a87a6b4285827 Update JSON tests to 1dc9d20e9716 Sep 18, 2019
@dvdplm
Handle missing r, s, v params in json tests
1c68b36 
Light cleanup of json test runner
@dvdplm
Merge branch 'master' into dp/chore/new-ethereum-consensus-tests
1ab7ec6 
* master:
  [ethcore]: move client test types to test-helpers (#11062)
  [sync]: remove unused dependencies or make dev (#11061)
  [ethcore]: reduce re-exports (#11059)
  [evmbin] fix time formatting (#11060)
  Update hardcoded headers (foundation, classic, kovan,  xdai, ewc, ...) (#11053)
  cargo update -p eth-secp256k1 (#11052)
@dvdplm
Include the path to the test file
63bb992
@dvdplm
Handle new postState format: string or map
6986fe8 
Sort out tests
Missing docs
niklasad1
niklasad1 reviewed Sep 19, 2019
View reviewed changes
json/src/spec/state.rs Outdated Show resolved Hide resolved
@dvdplm dvdplm mentioned this pull request Sep 20, 2019
Closed
@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 20, 2019
@ordian @dvdplm
[json-tests] populate state from genesis pod state (#11083)
8d80635 
* [json-tests] populate state from genesis pod state

* [json-tests] #11075 is resolved as well

* [json-tests] #11076 hopefully too

* [json-tests] #11077 🎉

* [json-tests] fix trailing comma
@ordian ordian added this to the 2.7 milestone Sep 23, 2019
ordian
ordian reviewed Sep 23, 2019
View reviewed changes
Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Let's update to beta when it's ready ethereum/tests#640 and merge this PR.

ethcore/src/json_tests/chain.rs Outdated Show resolved Hide resolved
ethcore/src/json_tests/chain.rs Outdated Show resolved Hide resolved
ethcore/src/json_tests/chain.rs Outdated Show resolved Hide resolved
ethcore/src/json_tests/chain.rs Outdated Show resolved Hide resolved
ethcore/src/json_tests/chain.rs Outdated Show resolved Hide resolved
dvdplm and others added 5 commits September 23, 2019 19:57
@dvdplm @ordian
Update ethcore/src/json_tests/chain.rs
70c45b1 
Co-Authored-By: Andronik Ordian <[email protected]>
@dvdplm
Add issue numbers to TODOs
025e323
@dvdplm
Merge branch 'dp/chore/new-ethereum-consensus-tests' of github.com:pa…
56a1069 
…ritytech/parity-ethereum into dp/chore/new-ethereum-consensus-tests

* 'dp/chore/new-ethereum-consensus-tests' of github.com:paritytech/parity-ethereum:
  Update ethcore/src/json_tests/chain.rs
  [json-tests] populate state from genesis pod state (#11083)
@dvdplm
Apply @ordians fix for wrong state_root
fa9f02a
@dvdplm
Warn on invalid RLP
8716813
@dvdplm
Copy link
Collaborator Author

dvdplm commented Sep 23, 2019

Let's update to beta when it's ready ethereum/tests#640 and merge this PR.

Hmm. Not sure I want to wait for that tbh. Who knows what else awaits when we update.

FWIW I updated and ran the tests from ethereum/tests@86b8a27 and we pass except for the Istanbul version of the same handful of tests that we skip in this PR.

@dvdplm
Copy link
Collaborator Author

dvdplm commented Sep 23, 2019 

* currently one test-skipping mechanism is hidden behind a feature (`ci-skip-tests`). Is this really what we want?

I think we should remove the feature. The purpose of the feature is to let CI run green while still being annoying to devs.
With the updated tests however we're in a situation where we either change our code like @debris suggestion in #10923 or we keep skipping those revert tests indefinitely. In that scenario, ci-skip-tests doesn't make much sense.

ordian
ordian approved these changes Sep 24, 2019
View reviewed changes
ethcore/spec/src/spec.rs
call_type: CallType::None,
params_type: ParamsType::Embedded,
if constructors.is_empty() {
state.populate_from(genesis_state.clone());
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if it should be called only in case of empty constructors

@dvdplm dvdplm requested a review from niklasad1 September 24, 2019 13:05
@dvdplm dvdplm merged commit d9201aa into master Sep 25, 2019
@dvdplm dvdplm deleted the dp/chore/new-ethereum-consensus-tests branch September 25, 2019 08:02
dvdplm added a commit that referenced this pull request Sep 25, 2019
@dvdplm
Update JSON tests to d4f86ecf4aa7c (#11054)
bd0b989 
* new ethereum consensus tests, #10908

* Update JSON tests to 725dbc73a

This PR reverts the controversial changes of the previous PR and skips the failing tests.

Maybe I misunderstand the suggested workaround of putting the fix under `#[cfg(test)]` but it seems odd to run different code in production than we run in tests. Instead here I suggest we skip the failing tests with the argument that we do not wish to fix this issue (at least not at this time) because it does not affect us. If I am wrong, and I likely am, I look forward to hearing why and what a better approach to updating the state tests is.

Branched off #10923

ref #10908

* Update json test commit to 1dc9d20e97165708f7db0bbf2d1a87a6b4285827

* Fail with error message

* Handle missing r, s, v params in json tests
Light cleanup of json test runner

* Include the path to the test file

* Handle new `postState` format: string or map
Sort out tests
Missing docs

* WIP

* Include test-helpers from ethjson

* Sort out new paths

* Remove dead code

* Fix warnings stemming from code called only from macros
Skip failing tests in stRevert/ and stTransactionTest/ (too course a filter!)
Docs and light touch refactorings for readability

* Skip all failing tests

* Document the single-test-skipping madness

* Update tests to latest commit on the `develop` branch

* Rename test skipping types to reflect actual purpose

* Switch to skipping individual tests in currents.json
Add some logging to help debug skipping

* Fix rpc test by curve fitting to new json test source file

* Add refs to all issues for fixing failing&skipped json tests

* Sort out the need for Clone for tests

* [json-tests] populate state from genesis pod state (#11083)

* [json-tests] populate state from genesis pod state

* [json-tests] #11075 is resolved as well

* [json-tests] #11076 hopefully too

* [json-tests] #11077 🎉

* [json-tests] fix trailing comma

* Update ethcore/src/json_tests/chain.rs

Co-Authored-By: Andronik Ordian <[email protected]>

* Add issue numbers to TODOs

* Apply @ordians fix for wrong state_root

* Warn on invalid RLP

* Remove the `ci-skip-tests` feature
@s3krit s3krit mentioned this pull request Sep 25, 2019
Merged
s3krit added a commit that referenced this pull request Sep 26, 2019
@s3krit
v2.6.4-beta (#11090)
7ae5d8e 
* ethcore/res: activate Istanbul on Ropsten, Görli, Rinkeby, Kovan (#11068)

* ethcore/res: activate Istanbul on Ropsten block 6485846

* ethcore/res: activate Istanbul on Goerli block 1561651

* ethcore/res: use hex values for Istanbul specs

* ethcore/res: fix trailing comma

* ethcore/res: be pedantic about EIP-1283 in Petersburg and Istanbul test specs

* ethcore/res: activate Istanbul on Rinkeby block 5435345

* ethcore/res: activate Istanbul on Kovan block 14111141

* ethcore/res: fix kovan istanbul number to 0xd751a5

* cleanup json crate (#11027)

* [json]: cleanup

write something here....

* nit: commit new/moved files

* nit: remove needless features

* nits

* fix(grumbles): use explicit import `DifficultyTest`

* fix(grumbles): remove needless type hints

* fix(grumble): docs `from -> used by`

Co-Authored-By: David <[email protected]>

* fix(grumbles): use explicit `imports`

* fix(grumble): merge `tx` and `tx_with_signing_info`

* fix(grumbles): resolve introduced `TODO's`

* [json-spec] make blake2 pricing spec more readable (#11034)

* [json-spec] make blake2 pricing spec more readable

* [ethcore] fix compilation

* Update JSON tests to d4f86ecf4aa7c (#11054)

* new ethereum consensus tests, #10908

* Update JSON tests to 725dbc73a

This PR reverts the controversial changes of the previous PR and skips the failing tests.

Maybe I misunderstand the suggested workaround of putting the fix under `#[cfg(test)]` but it seems odd to run different code in production than we run in tests. Instead here I suggest we skip the failing tests with the argument that we do not wish to fix this issue (at least not at this time) because it does not affect us. If I am wrong, and I likely am, I look forward to hearing why and what a better approach to updating the state tests is.

Branched off #10923

ref #10908

* Update json test commit to 1dc9d20e97165708f7db0bbf2d1a87a6b4285827

* Fail with error message

* Handle missing r, s, v params in json tests
Light cleanup of json test runner

* Include the path to the test file

* Handle new `postState` format: string or map
Sort out tests
Missing docs

* WIP

* Include test-helpers from ethjson

* Sort out new paths

* Remove dead code

* Fix warnings stemming from code called only from macros
Skip failing tests in stRevert/ and stTransactionTest/ (too course a filter!)
Docs and light touch refactorings for readability

* Skip all failing tests

* Document the single-test-skipping madness

* Update tests to latest commit on the `develop` branch

* Rename test skipping types to reflect actual purpose

* Switch to skipping individual tests in currents.json
Add some logging to help debug skipping

* Fix rpc test by curve fitting to new json test source file

* Add refs to all issues for fixing failing&skipped json tests

* Sort out the need for Clone for tests

* [json-tests] populate state from genesis pod state (#11083)

* [json-tests] populate state from genesis pod state

* [json-tests] #11075 is resolved as well

* [json-tests] #11076 hopefully too

* [json-tests] #11077 🎉

* [json-tests] fix trailing comma

* Update ethcore/src/json_tests/chain.rs

Co-Authored-By: Andronik Ordian <[email protected]>

* Add issue numbers to TODOs

* Apply @ordians fix for wrong state_root

* Warn on invalid RLP

* Remove the `ci-skip-tests` feature
@soc1c soc1c mentioned this pull request Nov 6, 2019
Merged
36 tasks
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

@jam10o-new jam10o-new jam10o-new left review comments

@sorpaas sorpaas Awaiting requested review from sorpaas

@cheme cheme Awaiting requested review from cheme

@debris debris Awaiting requested review from debris

Assignees

@dvdplm dvdplm

Labels
A0-pleasereview 🤓 Pull request needs code review. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. F4-tests 💻 Tests need fixing, improving or augmenting. P2-asap 🌊 No need to stop dead in your tracks, however issue should be addressed as soon as possible.
Projects
None yet
Milestone
2.7
Development

Successfully merging this pull request may close these issues.
5 participants
@dvdplm @niklasad1 @ordian @jam10o-new @debris