Skip to content
This repository has been archived by the owner on Jan 19, 2021. It is now read-only.

Drop ethereumjs-testing dep and fix bug in branch value update #69

Merged
merged 5 commits into from
Jan 10, 2019

Conversation

s1na
Copy link
Contributor

@s1na s1na commented Jan 10, 2019

This PR upgrades drops ethereumjs-testing dependency, adds the test fixture files directly to the repository and fixes #54 which caused a new test case to fail.

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
@holgerd77
Copy link
Member

Will do an ethereumjs-testing update in a minute 😄 together with a v1.2.6 release. If you want you can directly upgrade to this. Shouldn't have any effect though since trie tests very likely haven't been touched, so would be only for the "optics". 😛

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
@coveralls
Copy link

coveralls commented Jan 10, 2019

Coverage Status

Coverage increased (+0.1%) to 94.444% when pulling 8d658c4 on chore/upgrade-ethereumjs-testing into 5b227a1 on master.

@s1na
Copy link
Contributor Author

s1na commented Jan 10, 2019

@holgerd77 Sure, we might as well upgrade to the latest version :)

Browsers tests are now failing. It seems ethereumjs-testing depends on asyncawait which depends on node-fibers which doesn't work in browsers! Searching more to see if there's a workaround.

@holgerd77
Copy link
Member

Just did the v1.2.6 release.

@s1na
Copy link
Contributor Author

s1na commented Jan 10, 2019

@holgerd77 Even without the asyncawait issue karma isn't able to find the json test files from ethereumjs-testing. Do you have any ideas? Do other projects run ethereumjs-testing tests also in browser?

@holgerd77
Copy link
Member

Hmm, not sure. @danjm is also working on a tests update ethereumjs/ethereumjs-tx#131 on the tx library and this library also has a browser test configuration and script. However the test run is deactivated in .travis.yml (which also shouldn't be the case, don't know the reasoning ad hoc).

Maybe the PR from Dan helps nevertheless, eventually you can check it out and see if the browser tests pass manually? Otherwise you can figure out together?

@holgerd77
Copy link
Member

One other thing for consideration: these trie tests here are also pretty static, there was just one test case added in the last four years or so. Maybe we should also follow the path here to just remove this dependency all together like being done in ethereumjs/ethereumjs-block#61 for the block library (together with some copying and wrapping - see block PR-related issue - on the test files).

The tests dependency is a super-heavy several hundred MBs dependency which takes ages on installation, think this would very much make sense here too, think trie tests are just some KB compared.

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
@s1na s1na changed the title Upgrade ethereumjs-testing and fix bug in branch value update Drop ethereumjs-testing dep and fix bug in branch value update Jan 10, 2019
@s1na
Copy link
Contributor Author

s1na commented Jan 10, 2019

@holgerd77 Took you on your offer 😄 Added the json test files directly to the repository and dropped ethereumjs-testing dependency. Tests are passing now.

@holgerd77
Copy link
Member

Ah, but could you add the wrapping of the file structures with some meta fields for file information as well, see ethereumjs/ethereumjs-block#55 for reference.

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
@s1na
Copy link
Contributor Author

s1na commented Jan 10, 2019

Ah, I had missed that. Added the metadata.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Super-cool, great, thanks! 😄

@holgerd77 holgerd77 merged commit 37272b7 into master Jan 10, 2019
@holgerd77 holgerd77 deleted the chore/upgrade-ethereumjs-testing branch January 10, 2019 14:14
@ryanio ryanio mentioned this pull request Apr 17, 2020
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.

Library fails branch-value-update test
3 participants