-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Tiffany/snake case #1589
Tiffany/snake case #1589
Conversation
Hi. In the example shared under issue #1429 for the geth admin module, there were changes to be made to integration tests. However, I was a bit confused about how I could modify similar tests for the parity personal module in the common.py, http.py, ipc.py, and ws.py files. I notice for the most part that the modified code in these integration test files was just functions with this line of code: super().test_admin_start_stop_rpc(web3). However, I think I am having trouble understanding how exactly this tests the module, and how to adjust that appropriately for the parity personal module. Also for the core tests, based on the geth admin PR example, it seems as though there were individual core test files such as 'test_admin_addPeer.py' that were modified for different geth admin methods. However, I don't see files like this for the parity personal module. Should I create these files? Would you be able to assist me with this? @kclowes @pipermerriam |
Thanks for getting this started @tmckenzie51!
If you follow the inheritance up from the individual |
@kclowes Thank you for your helpful feedback. I followed the step you described above in your comment and made the necessary changes to the personal_module.py file on both GoEthereumPersonalModuleTest and ParityPersonalModuleTest classes. What is the difference between these 2 classes? I notice that they both seem to have similar test methods despite being 2 separate classes. Is there a reason for this that could help me understand the codebase better? Also, after making the changes mentioned in your comment above, I was able to make some progress. However, I have been getting CI errors related to one of the _deprecated tests. The link to an example of this CI test error is here - https://circleci.com/gh/ethereum/web3.py/100596?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link . For example, in this test, the error arises when test_personal_importRawKey_deprecated is called. This returns the error ‘account already exists’. I’m wondering if the new snake_cased tests are being run first, then the deprecated tests are being run after, which could potentially lead to an account state tracking error, which would trigger that error message. Would you be able to comment on this? |
Yep, they do. We split them up because the Geth and Parity clients sometimes have different implementations of the same methods. Instead of having
Yep, I think you're absolutely right. I would just add a new key by changing a few digits in the old key, and then use the new address it comes up with. Let me know if that doesn't make sense! |
Oh okay. That makes sense. Thank you for explaining that.
Oh okay. When I change a few digits of the hexadecimal private key (NEW_PRIVATE_KEY_HEX), and then call importRawKey with the NEW_PRIVATE_KEY_HEX and PASSWORD parameters, it will return the new_actual address. In the assertion, I would then have new_actual == NEW_ADDRESS. However, since the ADDRESS previously used was a hardcoded global variable, what would be the NEW_ADDRESS for this assertion? I’m assuming that this value has to be obtained outside of using the same function that we’re trying to test. How can I obtain the NEW_ADDRESS value without using importRawKey, in order to hard code it and test if the importRawKey method is working correctly? |
I think just using the address that it comes up with is sufficient. Then we'll know if we make a breaking change in the future. But, if you really wanted to be thorough, you could use eth_account to create an address and then make sure that
You may need to do some tweaking to the above code, but that's the general gist. Let me know if you have other questions! |
@kclowes Thank you for your help with that bug! For the importRawKey deprecated tests, I did pytest.raises(ValueError) since I would always expect test_importRawkey_deprecated to always return the 'account already exists' error. That led to most of the CI tests passing, at least the ones that were having errors triggered by this test. However, there's an error that I did not previously notice, which is related to both test_personal_unlock_account_failure and test_personal_unlockAccount_failure_deprecated . For both tests, the CI test fails with the error message "DID NOT RAISE <class 'ValueError'>". See: https://circleci.com/gh/ethereum/web3.py/101784?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link . I'm not sure why this would happen. I made very few changes to these methods - mostly just putting their names to snake_case. Do you have any idea as to what may be causing these errors? |
e4ba307
to
2b35857
Compare
2b35857
to
3d0f834
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @tmckenzie51!
What was wrong?
Parity Personal methods were camelCased rather than snake_cased
Related to Issue #
1429
How was it fixed?
Parity Personal method names were changed from camelCase to snake_case. Deprecation warnings and tests for the snake_cased methods were also added.
Todo:
Cute Animal Picture