LVFM-488: Major rewrite of the Bitcoin app APDU-level tests, using a in-house transaction parser #157
Conversation
…se, Fix parser issues related to GetTrustedInput, reworked sendApdus() method, Update txparser, Helpers reworked to make segwit tests pass, More fixes in helpers for segwit inputs, Deactivate expected_signature as a test data (still WIP), Fix issue with input script not sent in some cases, Rework hash computation to fix wrong segwit tx hash + stuff, Adaptations for test data move to conftest.py + initial mods for test automation (unfinished)
…offsets, Removed some more indexes + fixed issues in segwit inputs script handling, Fixed Zcash Overwinter tests + adapted other tests to TxHashMode being instanciable, Finish Segwit / Zcash tests adaptations to new tx parser, Moved test data from test scripts to new conftest.py & use fixtures to access them
There was a problem hiding this comment.
Replied to @onyb 's review remarks. Suggestions will be resumed in an update to the tests/README.md file for follow-up by @grydz / @TamtamHero
There was a problem hiding this comment.
This looks like a good first step towards having something readable, which is the most important thing for a test suite. 🚀
Also great work on the README! 📝
The next steps are already documented in the README file, but from my perspective (as an external guy), I would like to emphasize on the need to have the following things as future improvements:
-
Cross-project Python factories to generate APDUs
It could utilize the awesome
constructpackage under the hood, which I think also has a convincing use-case throughout the codebase of this test suite. -
Raw transaction generator
This will allow us to exercise the Bitcoin app against a wide range of tests, instead of just a couple of them. I learnt that the parser is very picky about what it considers as a valid transaction, but we could explicitly mark such cases as known failures, serving as a kind of documentation as well.
-
Seed independent tests
This will allow us to write tests that are not already known to pass.
-
Continuous integration
To ensure anyone can improve the tests without the fear of breaking something.
Happy to help along the way!
UPDATE: some remark taken into account (code style & quick wins), others related to structural changes compiled in the "Next steps" section of the
tests/README.mdfile. PR can be merged IMO.The previous version of the BTC tests used error-prone and difficult-to-maintain offset calculations to get the various parts of a raw tx. This version uses an in-house transaction parser
TxParsethat is simply adataclasswith afrom_raw()method which identify each element of the tx named attributes.The tests have also been rewritten to isolate the raw tx data into the file conftest.py and use fixtures to pass them to the tests. Using trusted inputs or not by a same test is achieved with the
use_trusted_inputspytest parameter, resulting in shorter test files.Note: each test repeats the same APDU sequence so it can be envisaged in the future to isolate that sequence into a method of class
BaseTestBtc. This way, adding new nominal tests would be done by simply providing some raw tx data and calling that method.Note 2: Some work on adding click/event automation was started but not finished.