Skip to content

Add ability to write unittests#5

Open
ImplOfAnImpl wants to merge 9 commits into
nanox_so_fixfrom
add_ability_to_write_unittests
Open

Add ability to write unittests#5
ImplOfAnImpl wants to merge 9 commits into
nanox_so_fixfrom
add_ability_to_write_unittests

Conversation

@ImplOfAnImpl
Copy link
Copy Markdown
Collaborator

@ImplOfAnImpl ImplOfAnImpl commented May 28, 2026

Unstable rust has a way to specify a custom test runner and this is used e.g. in the ledger rust sdk itself to implement unit tests that can be run on device.
The way it works is:

  1. You specify a custom test runner, which will receve the collection of test cases, via #![test_runner(...)].
    In our case the test runner is ledger_device_sdk::testing::sdk_test_runner - it iterates over the passed collection of test cases (each of which is ledger_device_sdk::testing::TestType), does some address adjustment (apparently, link-time addresses differ from run-time ones in ledger apps, and for some reason in this case the adjustment doesn't happen automatically), and calls the test body stored inside TestType.
  2. The normal macro #[test] doesn't work in this case and the low-level #[test_case] must be used (this is also from unstable rust). Ledger has the testmacro crate, which provides the #[test_item] macro, which encapsulates #[test_case] and generates a test case item of the ledger_device_sdk::testing::TestType type.
  3. You then call test_main from sample_main. If a test fails, the return value will be non-zero and it will become the exit code of speculos itself.

In this PR:

  1. The app source code was moved to a separate lib crate mintlayer-app-core. This is because I'd prefer the lib's sample_main to be dedicated to testing only and the app's sample_main to be production-only.
    To make the src structure look better, I created a crates directory in the root and put both the "app-core" and "messages" there.
    For consistency, the package name of "messages" is now "mintlayer-messages".
  2. I added a workspace in the root Carego.toml, so that versions of dependencies can be specified in one place.
  3. It doesn't seem like the "official" Ledger CI jobs run clippy, so I added a script that runs it and a CI job that invokes the script.
    The main reason is to avoid accidental "result unused" warnings in tests, but having clippy is nice in general.
  4. Before doing the changes I decided to update the test snapshots, to make sure I don't break anything, so the new snapshots are also in this PR.

For now I've only implemented a sample test in crates/app-core/src/handlers/sign_tx/summary_collector.rs. More tests need to be added.

Additional notes:

  1. This sdk machinery doesn't seem to be well-documented, so I'm not sure how "stable" it is.
    But some existing ledger apps seem to also use it (e.g. app-sui), so I guess it's consedered to be "stable enough".
  2. When a single test case panics, the whole runner will be aborted. The sdk test machinery allows tests to return Result<(), ()> and if an error is returned from one test case, the rest will still be run. But there is no way to specify the reason for the failure (except for logging something before a failure can occur), which makes failures not very informative.
    With panicking, the panic reason will be logged by our custom panic handler and will appear in speculos console output. I.e. both approaches have drawbacks; I'd say let's stick to panicking, because it's simpler.
  3. All device models seem to have a limit of 400KB for the flash occupied by the app. So if we write lots of tests, the test executable may exceed the limit. The easiest solution will be to split all tests into logical "test suites" and put each one under a feature, so that running all tests will require several invokations of cargo test. I.e. though the solution is not pretty, the problem is solvable.

P.S. I also removed this:

[target.apex_p]
runner = "speculos -m apex_p"

[build]
target = "apex_p"

from .cargo/config.toml. I'm not sure why it's needed there (because normally you would always specify the model explicitly anyway) and it interferes with VSCode's attempts to run cargo check. E.g. I have this in my .vscode/settings.json:

"rust-analyzer.check.overrideCommand": [
    "bash", "-lc",
    "docker run --rm -v \"${workspaceFolder}:${workspaceFolder}\" -w \"${workspaceFolder}\" ghcr.io/ledgerhq/ledger-app-builder/ledger-app-dev-tools:latest bash -lc 'cargo check --quiet --workspace --message-format=json --keep-going --target apex_p'"
],

and rust-analyzer works fine if those lines in .cargo/config.toml are removed, but if they're present, it fails with some weird errors. Let me know if you needed them for some reason.

@ImplOfAnImpl ImplOfAnImpl force-pushed the add_ability_to_write_unittests branch 2 times, most recently from 1d65aae to 9abba00 Compare May 29, 2026 08:47
@ImplOfAnImpl ImplOfAnImpl force-pushed the add_ability_to_write_unittests branch from 9abba00 to 8d6c07a Compare May 29, 2026 08:54
@ImplOfAnImpl ImplOfAnImpl marked this pull request as ready for review May 29, 2026 10:11
@ImplOfAnImpl ImplOfAnImpl requested a review from OBorce May 29, 2026 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant