test: add tests to cover new Exceptions in arguments.py #2
test: add tests to cover new Exceptions in arguments.py #2qlrd wants to merge 4 commits intoodudex:replace_main_code_assertsfrom
Conversation
qlrd
force-pushed
the
replace_main_code_asserts
branch
from
926a713 to
036c75e
Compare
qlrd
changed the title
tests/tests/test_bip85.py
| @classmethod | ||
| def from_string(cls, s: str): | ||
| arr = s.split("/") | ||
| if len(arr[0]) % 2 != 0: |
There was a problem hiding this comment.
I believe this check aims to avoid unhandled exceptions with unhexlify, right? Maybe unhexlify could be inside a try/except to also catch other invalid strings and non hex characters.
Also, remembering this lib is made to run on limited devices, I would reduce verbosity to a minimum:
"Invalid fingerprint format"
"Invalid fingerprint length"
There was a problem hiding this comment.
Fair, this will allow another tests
qlrd
force-pushed
the
replace_main_code_asserts
branch
2 times, most recently
from
4cadb16 to
bf6d69c
Compare
| try: | ||
| mfp = unhexlify(arr[0]) | ||
| except Exception as err: | ||
| raise ArgumentError(err) from err |
There was a problem hiding this comment.
Just to be sure, did you test in device if raise from is functional in micropython?
If it was not used so for by Krux or Embit, it would be good to test.
There was a problem hiding this comment.
Mmm, good point. Didn't tested. My bad for this lack of attention. I believe micropython do not supoort it right? Maybe something like the below can be better?
| raise ArgumentError(err) from err | |
| raise ArgumentError(str(err)) |
There was a problem hiding this comment.
Maybe it works the way it is, but it needs to be tested
There was a problem hiding this comment.
now that you said, maybe these are theoretical cases, but I believe that in real devices, they may not happen. Maybe it's a dead-code?
There was a problem hiding this comment.
tested in a wondermv, but did not get any of these theoretical errors (IMO because the devices, in a real situations, will never load incorrect fingerprints).
These tests cover, indirectly, the new exceptions in `test_descriptor` (covering wrong fingerprints lenghts) as well in `test_taproot` (covering a try to tweak taproot in a non taproot generated key from a wpkh descriptor).
qlrd
force-pushed
the
replace_main_code_asserts
branch
from
bf6d69c to
502e4f6
Compare
…ptree The current `Descriptor` class do not verify if the instances are comming from `Key`, `Miniscript` or `TapTree`. This commit add these checks and raise exceptions when needed.
2 participants
Add tests for arguments.py