Run doctests with tox #49
Conversation
|
Yes please, enabling doctest is something we've been adding to all of our repos and it should be in place here as well. |
|
Yup, this is great, thanks!
Please fix in this PR. As a rule we never merge a failing CI run. FWIW, it's important that the readability of the docs not suffer in order to get the tests passing. |
|
Great, then I may pick this up over the weekend if I get a chance. |
|
Seems like this can be closed? |
I don't think this change was done yet. We are already test appropriately-marked snippets in the |
Fixes some syntax errors in previous doctests. Some tests are disabled with `doctest: +SKIP` as they couldn't be ran or fixed as it is for now. Note that this `doctest: +SKIP` comment won't appear in the generated documentation.
AndreMiras
force-pushed
the
feature/ticket9_run_doctests
branch
from
d49b431
to
8d99dea
Compare
|
Hi guys, I will eventually resume my contributions starting from this one as I have more time 🎉 |
| ... 'salt': '45cf943b4de2c05c2c440ef96af914a2'}, | ||
| ... 'mac': 'f5e1af09df5ded25c96fcf075ada313fb6f79735a914adc8cb02e8ddee7813c3'}, | ||
| ... 'id': 'b812f3f9-78cc-462a-9e89-74418aa27cb0', | ||
| ... 'version': 3} |
There was a problem hiding this comment.
Enabling doctests helped in detecting this "syntax error"
|
|
||
| >>> import getpass | ||
| >>> Account.decrypt(encrypted, getpass.getpass()) | ||
| >>> Account.decrypt(encrypted, getpass.getpass()) # doctest: +SKIP |
There was a problem hiding this comment.
I had to disable because obviously getpass() would ask for terminal input.
We could also give an example with an hardcoded value for the password that always produces an expected output.
There was a problem hiding this comment.
Yeah, maybe worth doing both: demonstrating good practices but skipping the test (as you already did), and adding a version that can be doctest-ed.
There was a problem hiding this comment.
Yes that could be it. Actually I initially wanted to find the password used to generate this output, but haven't found it so far.
I've created a task for adding a doctest that works #81
Maybe add an issue for fixing & re-enabling these tests? |
| @@ -187,13 +187,13 @@ def encode_defunct( | |||
| SignableMessage(version=b'E', header=b'thereum Signed Message:\n6', body=b'I\xe2\x99\xa5SF') | |||
|
|
|||
| # these four also produce the same hash: | |||
| >>> encode_defunct(w3.toBytes(text=message_text)) | |||
| >>> encode_defunct(w3.toBytes(text=message_text)) # doctest: +SKIP | |||
There was a problem hiding this comment.
Just curious what the failure was here (I'm fine to push it to another issue)
There was a problem hiding this comment.
NameError: name 'w3' is not defined
There was a problem hiding this comment.
Hah, ok. Well that's not an instance method so it should be straightforward to switch w3 to Web3 and just add a from web3 import Web3 up top. Just a note for when you/someone does the follow-up work.
|
Thanks for the review and approval @carver
I'm on holiday without much access to dev laptop, so I'd prefer we merge it. However I've created a followup task that I'll be happy to pick up later. |
Follow up comments on PR ethereum#49, closes ethereum#81
This is a follow up for comments to PR ethereum#49, closes ethereum#81.
Collection of updates
What was wrong?
Currently the CI doesn't run doctests.
However keep in mind that now we have 5 tests failing.
If you're interested with the approach, I can also fix them in this PR or in another one.
Issue #9
How was it fixed?
Enabling doctests via
tox.iniandpytest.iniCute Animal Picture