Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Package up base db and atomic db test suites into re-usable classes #1813

Conversation

pipermerriam
Copy link
Member

@pipermerriam pipermerriam commented Aug 6, 2019

fixes #1812

What was wrong?

The atomic and base DB test suites are useful outside of the py-evm library but they were not re-usable in the form they were written. Also most of the base database test suite was duplicated.

How was it fixed?

Packaged the test suites up into class-based tests and put them under eth.tools.db.

I also expanded the test suite to cover the batch object for the AtomicDB as well as and the base db suite to cover the CacheDB.

To-Do

  • Clean up commit history

Cute Animal Picture

put a cute animal picture link inside the parentheses

@pipermerriam pipermerriam force-pushed the piper/re-usable-atomic-and-base-db-test-suites branch from 5c2bb36 to a26e986 Compare August 6, 2019 17:42
@pipermerriam pipermerriam requested a review from lithp August 6, 2019 17:53
Copy link
Member Author

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

The tests are not exact 1:1 copies as I added a missing case or two and split one or two tests into multiple cases for more granular testing.

You can also see that I found a place where LevelDB behavior diverged from the defined API behavior.

def test_database_api_item_delete_missing_key(self, db: DatabaseAPI) -> None:
assert b'key-1' not in db
with pytest.raises(KeyError):
del db[b'key-1']
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised that .delete() doesn't throw an exception but __delitem__ does! It seems like those two should have the same behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just realized that MutableMapping doesn't have a set, delete or exists API....

Copy link
Contributor

@lithp lithp left a comment

Choose a reason for hiding this comment

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

Don't forget the cute animal picture!

No complaints here, the classes are an elegant way of exposing acceptance tests for later implementers of these interfaces to use.

@pipermerriam pipermerriam merged commit 384fdf0 into ethereum:master Aug 13, 2019
@pipermerriam pipermerriam deleted the piper/re-usable-atomic-and-base-db-test-suites branch August 13, 2019 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make test suites for DatabaseAPI and AtomicDatabaseAPI re-usable
2 participants