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

util cleanup #1474

Merged
merged 10 commits into from Jul 5, 2016
Merged

util cleanup #1474

merged 10 commits into from Jul 5, 2016

Conversation

debris
Copy link
Collaborator

@debris debris commented Jun 28, 2016

  • removed unused util/squeeze.rs
  • removed unused util/json_aid.rs
  • removed util/json-tests module. It was originally created by me as a replacement for ethereum / tests.
    • there is just one test worth keeping there. it's util/json-tests/json/trie/branching.json
  • few simplifications in triehash.rs iterators (7 lines added)

@debris debris added the A0-pleasereview 🤓 Pull request needs code review. label Jun 28, 2016
@gavofyork
Copy link
Contributor

are these not the standard json tests that all trie implementations check against?

@debris
Copy link
Collaborator Author

debris commented Jun 28, 2016

are these not the standard json tests that all trie implementations check against?

They are. I forgot about that. We can safely remove everything then :)

@gavofyork
Copy link
Contributor

only if they're still checked from the standard tests. and i don't think they are.

@gavofyork gavofyork added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 28, 2016
@debris
Copy link
Collaborator Author

debris commented Jun 29, 2016

I've added util/json-tests/json/trie/branching.json test to triedbmut.rs in 321b16c. I think it's sufficient.

@debris debris added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jun 29, 2016
@gavofyork
Copy link
Contributor

i'd prefer to keep the standard ethereum trie tests for completeness.

they are part of the cross-client consensus tests suite and i see no reason to remove.

@debris
Copy link
Collaborator Author

debris commented Jun 29, 2016

Removed files are not a part of cross-client consensus tests. We never used cross-client consensus tests for trie, cause these tests are placed in 6 json files written in 3 different formats. Instead of using them directly I unified their format and placed them in util/json-tests/json/trie directory (it was back in December last year). This pr is moving these tests to the code.

We could add a module for parsing and running cross-client consensus tests, but imo it should not be a part of this pr

@gavofyork
Copy link
Contributor

This pr is moving these tests to the code.

only additional tests i see are the long random strings. where have those consensus-derived tests been moved to?

@debris
Copy link
Collaborator Author

debris commented Jul 4, 2016

All trie tests from ethereum/tests looks quite random. Anyway I've added a module, which is running all of them. link

@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 5, 2016
@gavofyork gavofyork merged commit 62b9c1b into master Jul 5, 2016
@gavofyork gavofyork deleted the util_cleanup branch July 5, 2016 13:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants