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

Maybe use a registry fixture to test custom registrations #77

Open
davesque opened this issue Jun 5, 2018 · 5 comments
Open

Maybe use a registry fixture to test custom registrations #77

davesque opened this issue Jun 5, 2018 · 5 comments
Labels

Comments

@davesque
Copy link
Contributor

davesque commented Jun 5, 2018

What was wrong?

In the following integration tests that test custom registrations, our method of testing is a bit stateful since it registers then unregisters coders for the custom data type. If one test fails, that could lead to a confusing failure in another test when the registry complains that a registration already exists for the custom type:

https://github.com/ethereum/eth-abi/blob/master/tests/test_integration/test_custom_registrations.py#L69-L99

How can it be fixed?

We could modify those tests to use a fixture registry instance that is created per test run. If we want to continue to do true integration testing via encode_single and decode_single, we will probably need to modify the API to support use of a custom registry instance.

@carver
Copy link
Collaborator

carver commented Jun 5, 2018

we will probably need to modify the API to support use of a custom registry instance.

Something like an optional registry kwarg, maybe? eg~ encode_single(typ, arg, registry=None)

@davesque
Copy link
Contributor Author

davesque commented Jun 5, 2018

That seems like the most natural way to do it.

@pipermerriam
Copy link
Member

I've wanted to do this the other way around.

# eth_abi/__init__.py

from eth_abi.wherever import default_registry

encode_single = default_registry.encode_single
decode_single = default_registry.decode_single
...

So in tests we can just have a registry test fixture which we can then call these functions directly (and still probably some smoke tests to ensure that the default registry works as expected).

@davesque
Copy link
Contributor Author

davesque commented Jun 6, 2018

Yep, I remember some conversations we had about that now. Seems reasonable.

@carver
Copy link
Collaborator

carver commented Jun 6, 2018

👍 registry.encode_single is better

pacrob added a commit to pacrob/eth-abi that referenced this issue Apr 14, 2023
* convert bash scripts to py
pacrob added a commit to pacrob/eth-abi that referenced this issue Dec 9, 2023
* convert bash scripts to py
@pacrob pacrob added the p4 label Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants