trezor / trezor-firmware

:lock: Trezor Firmware Monorepo
https://trezor.io
Other
1.36k stars 661 forks source link

Rust unit test dependency and initialization issues #4189

Open cepetr opened 1 month ago

cepetr commented 1 month ago

This issue report addresses two system problem with our Rust unit tests (make test_rust):

1. Dependency on make build_unix:

The make test_rust target currently relies on build artifacts from make build_unix. If the emulator hasn't been built beforehand, the test build will fail. Ideally, these two targets should be independent, so that running tests does not require the emulator to be built first.

2. Improper Initialization of C Functions in Rust Tests:

In Rust unit tests, functions from the C world (e.g., trezorhal) can be invoked without proper prior initialization. While tests can call any function defined in the ffi module, there is currently no mechanism to initialize trezorhal. Normally, this is handled in main.c, which isn't included or called during test execution.

This problem became evident with the introduction of the system module, specifically with the new system_init() function. In the firmware/bootloader/emulator, system_init() is the first function called in main() to initialize system services (e.g., systick). Without this initialization, functions like systick_ms() will always return 0. Since systick_ms() is used in other trezorhal functions (as well as in storage), these functions do not work correctly without the necessary initialization.

We’ve applied a temporary fix for this issue by automatically initializing the systick module whenever any of its functions are called. However, similar initialization issues could arise in other parts of the system in the future.

To prevent these issues, systematic approach to properly initialize trezorhal drivers in the unit test binary before any test is executed should be introduced.

matejcik commented 1 month ago

Ideally, these two targets should be independent, so that running tests does not require the emulator to be built first.

this seemed impractical in the past: the cargo mechanism can't easily produce the micropython and trezorhal obj files that it requires, because their build is itself rather complicated

istm we can just add build_unix to dependencies of test_rust to patch over this problem :woman_shrugging:

  1. Improper Initialization of C Functions in Rust Tests:

the easiest solution is to expose a test_setup() function from C that we'll stick into a lazy static on the Rust side, and test writers will be responsible for running it at start

more complex solution is replacing the Rust test harness with something that supports global setup, but that doesn't seem that easy