Restore test #1729
Restore test #1729
Conversation
So unit tests don't have to perform intricate mocks to enter a mneomnic using enter_string, menu, etc. Same mnemonic as in mnemonic_c_unit_tests.rs, as it will be used in the unit tests of api::restore.
No need to keep that.
Since a1c553b, the mnemonic module was not compiled during testing by accident, as a specific implementation was compiled in for c-unit-testing. This means that the unit tests and clippy were not running for mnemonic.rs.
cedwies
left a comment
cedwies
left a comment
There was a problem hiding this comment.
Good PR, did not spot major issues. Apart from the comments I have another question:
The test mnemonic boring mistake dish oyster... appears in both testing.rs and mnemonic_c_unit_tests.rs. If someone updates one but forgets the other, tests could silently pass with different data. Would a shared constant be better, or is the duplication intentional to keep the Rust tests and C tests completely isolated?
| } | ||
| } | ||
|
|
||
| /// Retrieve a BIP39 mnemonic sentence of 12, 18 or 24 words from the user. |
There was a problem hiding this comment.
Just so I am not missing anything: The comment was wrong anyway, right?. Did we even support 18 words?
There was a problem hiding this comment.
We used to until recently.
| pub mod confirm; | ||
| pub mod menu; | ||
| #[cfg_attr(feature = "c-unit-testing", path = "workflow/mnemonic_c_unit_tests.rs")] | ||
| #[cfg_attr( |
There was a problem hiding this comment.
With cfg_attr(all(feature = "c-unit-testing", not(feature = "testing")), ...), if someone (accidentally) enables both c-unit-testing and testing at the same time, the code silently uses the "real" mnemonic.rs. Would it be safer to add a compile_error! to catch this?
There was a problem hiding this comment.
In Rust unit tests, both features are active (all features are active in Rust unit tests), and the "real" mnemonic.rs should be used. That's why I did this change. Or am I missing something?
There was a problem hiding this comment.
ah no I was missing something, thanks for clarifying
The mnemonic in mnemonic_c_unit_tests.rs is used by the (non-graphical) simulator and shouldn't change, as probably integration tests in various repos depend on it. (The same mnemonic also exists once more in graphical simulator main.rs). The new one in testing.rs is merely for Rust unit tests, and could be a different one, or be replaced by some sort of mocking facility later, so I don't think there should be a shared constant. |
cedwies
left a comment
cedwies
left a comment
There was a problem hiding this comment.
Thanks for clarifying, utACK
2 participants
No description provided.