refactor: common test utils #2650

Alenar · 2025-07-24T16:41:57Z

Content

This PR:

rename mithril-common test_utils module to simply test: this align common to our latest practice
reorganize mithril_common::test module:
- add test::assert includes: assert_dir_eq, assert_equivalent, assert_same_json
- add test::builder includes builder of: cardano transactions, certificates chain, mithril fixture
- move fake_data, fake_keys, precomputed_kes_key to test::double
- add logging which includes the memory logger and a new define_test_logger! macros to avoid redefining the TestLogger in every crates
move all test utilities that where alongside production code but gated behind cfg_test to the test module:
- move FakeCertificaterRetriever and DummyApiVersionDiscriminantSource to test::double
- move crypto_helper test utilities to test::crypto_helper, test_utils methods attached to MKProof and ProtocolInitializer are transformed into extension traits
- transform test_utils methods attached to entities to extension traits located in test::entities_extensions
remove CardanoTransactionsSigningConfig::new as it was behind test_utils and bring more confusion than value

With those changes the test_utils feature of mithril-common can be removed easily.

Resulting structure

src/test/: (renamed) was src/test_utils/
├── assert : (new) module for custom assertions
│   ├── dir_eq.rs: (moved) from src/test_utils/
│   ├── iters_equivalent.rs: (new) new module for assert_equivalent that was in src/test_utils/mod.rs
│   ├── json.rs: (new) new module for assert_same_json that was in src/test_utils/mod.rs
│   └── mod.rs
├── builder: (new) module for test specialized builders of common, mostly business, objects
│   ├── cardano_transactions_builder.rs: (moved) from src/test_utils/
│   ├── certificate_chain_builder.rs: (moved) from src/test_utils/
│   ├── fixture_builder.rs: (moved) from src/test_utils/
│   ├── mithril_fixture.rs: (moved) from src/test_utils/
│   └── mod.rs
├── crypto_helper: (new) test utilities moved from the src/crypto_helper module
│   ├── cardano
│   │   ├── extensions.rs: (new) define extension traits to replace test_tools only methods for types in crypto_helper
│   │   ├── kes: (moved) from src/crypto_helper/cardano/kes
│   │   │   ├── mod.rs
│   │   │   ├── setup.rs: (moved) from src/crypto_helpercardano/kes/tests_setup.rs
│   │   │   └── signer_fake.rs: (moved) from src/crypto_helpercardano/kes/signer_fake.rs
│   │   └── mod.rs
│   ├── mod.rs
│   └── setup.rs: (moved) from src/crypto_helper/tests_setup.rs
├── double
│   ├── api_version.rs: (new) file for DummyApiVersionDiscriminantSource that was in src/api_version
│   ├── certificate_retriever.rs: (moved) from src/certificate_chain/fake_certificate_retriever
│   ├── dummies.rs
│   ├── fake_data.rs: (moved) from src/test_utils/
│   ├── fake_keys.rs: (moved) from src/test_utils/
│   ├── mod.rs
│   └── precomputed_kes_key.rs: (moved)
├── entities_extensions.rs: (new) define extension traits to replace test_tools only methods for types in src/entities
├── logging: (new) utilities to use logging in tests
│   ├── memory_logger.rs
│   └── mod.rs: (new) define_test_logger macro to simplify creation of TestLogger in child crates
├── mock_extensions.rs
├── mod.rs
└── temp_dir.rs

Pre-submit checklist
Branch
- Tests are provided (if possible)
- Crates versions are updated (if relevant)
- Commit sequence broadly makes sense
- Key commits have useful messages
PR
- All check jobs of the CI have succeeded
- Self-reviewed the diff
- Useful pull request description
- Reviewer requested
Documentation
- Add ADR blog post or Dev ADR entry (if relevant)
- No new TODOs introduced

Comments

The strategy used to move test_utils specific methods attached to public types was to transform them into extensions traits, this strategy allow to force import of a item located in the test module, making explicit the usage of a test only tool, but have one caveat:
if the trait impl is not located beside the type some private properties must be made pub(crate) in order to access them (problem raised with BlockRange::inner_range and StmInitializerWrapper::stm_initializer).
A way to solve this problem is to move the trait impl, and only the impl, to the type, this have the cost of keeping part of the test code alongside the production code.

edit: The extensions traits for test impl were moved alongside the type to avoid making their internal pub(crate).

Next step

Write one or more DEV-ADR to specify how crates public test modules should be made
Remove test_utils feature from mithril-common

Issue(s)

Relates to #2580

plus `precomputed_kes_key`

…::builder`

As it produce the error directly in the caller test, allowing to give the line of the failure.

…ger` creation Also move this new and the existing test logging utilities to a `test::logging` module.

And move its definition to a `test` module instead of a `test_utils` to be in line with the latest naming scheme (only for crates that already have a `test` module).

As mocks are test double, it should have been there in the first place

…o_helper`

…double`

…ion traits This allow a clean split between the production and test code even when using thoses methods as to do so you will now need to import a trait located in the `mithril_common::test` module. one caveat: in order to transform `Blockrange::new` into a extension trait method, its `inner_range` property had to be made `pub(crate)`.

…:new` Has its value was low and it was even confusing since both parameters use the same type. This allow to remove the last usage of `cfg_test_tools` in mithril common, beside the one on the test module.

mithril-common/src/test/mod.rs

Copilot

Pull Request Overview

This pull request refactors the mithril-common crate's test utilities by renaming the test_utils module to test and reorganizing its structure to follow a more consistent pattern across the codebase.

Key changes:

Renamed test_utils module to test throughout the codebase
Reorganized test utilities into logical submodules (assert, builder, double, crypto_helper, logging)
Moved test-only methods from production code to extension traits in the test module
Introduced a define_test_logger! macro to standardize test logging across crates

Reviewed Changes

Copilot reviewed 268 out of 272 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
mithril-common/src/test_utils/mod.rs	Removed old test utilities module (251 lines deleted)
mithril-common/src/test/mod.rs	New organized test module with submodule structure
mithril-common/src/test/logging/mod.rs	New logging utilities with `define_test_logger!` macro
mithril-common/src/test/entities_extensions.rs	Extension traits for test utilities on entity types
mithril-common/src/test/crypto_helper/	Moved crypto helper test utilities to dedicated module
Multiple files across all crates	Updated imports from `test_utils` to `test` with new submodule paths

mithril-common/src/test/assert/iters_equivalent.rs

mithril-common/src/test/temp_dir.rs

mithril-common/src/test/mod.rs

mithril-common/src/test/mock_extensions.rs

github-actions · 2025-07-24T16:49:18Z

Test Results

4 files ±0 154 suites ±0 22m 27s ⏱️ -1s
2 118 tests ±0 2 118 ✅ ±0 0 💤 ±0 0 ❌ ±0
6 466 runs ±0 6 466 ✅ ±0 0 💤 ±0 0 ❌ ±0

Results for commit f0e30e0. ± Comparison against base commit 377d7f6.

This pull request removes 81 and adds 81 tests. Note that renamed tests count towards both.

mithril-common ‑ certificate_chain::fake_certificate_retriever::tests::fake_certificate_retriever_retrieves_existing_certificate
mithril-common ‑ certificate_chain::fake_certificate_retriever::tests::test_fake_certificate_fails_retrieving_unknown_certificate
mithril-common ‑ crypto_helper::cardano::kes::signer_fake::tests::fake_kes_signer_returns_signature_batches_in_expected_order
mithril-common ‑ test_utils::cardano_transactions_builder::test::build_block_ranges_return_many_transactions_per_block_when_requested
mithril-common ‑ test_utils::cardano_transactions_builder::test::build_block_ranges_return_the_number_of_block_ranges_requested
mithril-common ‑ test_utils::cardano_transactions_builder::test::build_block_ranges_with_many_blocks_per_block_ranges
mithril-common ‑ test_utils::cardano_transactions_builder::test::build_transactions_with_many_blocks_per_block_ranges
mithril-common ‑ test_utils::cardano_transactions_builder::test::generate_one_block_range_return_one_transaction_by_default
mithril-common ‑ test_utils::cardano_transactions_builder::test::only_the_last_block_is_not_full_when_we_can_not_fill_all_blocks
mithril-common ‑ test_utils::cardano_transactions_builder::test::return_all_transactions_in_same_block_when_ask_less_transactions_than_transactions_per_block
…

mithril-common ‑ test::assert::dir_eq::tests::can_compare_two_path
mithril-common ‑ test::assert::dir_eq::tests::can_provide_additional_comment
mithril-common ‑ test::assert::dir_eq::tests::dir_eq_multiple_files_and_dirs
mithril-common ‑ test::assert::dir_eq::tests::dir_eq_single_dir
mithril-common ‑ test::assert::dir_eq::tests::dir_eq_single_file
mithril-common ‑ test::assert::dir_eq::tests::dir_structure_diff
mithril-common ‑ test::assert::dir_eq::tests::path_to_dir_structure
mithril-common ‑ test::assert::dir_eq::tests::trim_whitespaces_at_lines_start
mithril-common ‑ test::assert::iters_equivalent::tests::test_equivalent_to
mithril-common ‑ test::builder::cardano_transactions_builder::test::build_block_ranges_return_many_transactions_per_block_when_requested
…

♻️ This comment has been updated with latest results.

dlachaume

LGTM 🚀

- fix incorrect paths in tests - fix making pub(crate) previously internal fields for test extension traits by implementing the trait alongside the type (at the cost of mixing tests tools code with production code, but we keep the benefit of the trait import tagged as a test tool) - update `mithril_common::test` module doc - fix `FakeCertificaterRetriever` async trait use in wasm - remove `cfg_test_tools` macro to limit additional uses until the `test_tools` feature is removed

* mithril-cardano-node-chain from `0.1.5` to `0.1.6` * mithril-cardano-node-internal-database from `0.1.2` to `0.1.3` * mithril-dmq from `0.1.5` to `0.1.6` * mithril-era from `0.1.3` to `0.1.4` * mithril-metric from `0.1.16` to `0.1.17` * mithril-persistence from `0.2.56` to `0.2.57` * mithril-signed-entity-preloader from `0.0.8` to `0.0.9` * mithril-api-spec from `0.1.3` to `0.1.4` * mithril-aggregator from `0.7.75` to `0.7.76` * mithril-client-cli from `0.12.23` to `0.12.24` * mithril-client from `0.12.23` to `0.12.24` * mithril-common from `0.6.10` to `0.6.11` * mithril-relay from `0.1.48` to `0.1.49` * mithril-signer from `0.2.261` to `0.2.262` * mithril-end-to-end from `0.4.97` to `0.4.98`

turmelclem

LGTM ☀️

Alenar added 13 commits July 24, 2025 18:03

refactor(common): move fake_data & fake_keys to test_utils::double

5851af0

plus `precomputed_kes_key`

refactor(common): rename mithril_common::test_utils module to test

8e64b7f

refactor(common): move all builder in mithril_common::test to `test…

5c2b7c5

…::builder`

refactor(common): move assert tools to new test::assert module

8467d3b

refactor(common): supersede assert_equivalent fun with a macro

d58a9f3

As it produce the error directly in the caller test, allowing to give the line of the failure.

refactor(common): add define_test_logger macro for unified `TestLog…

15f18a2

…ger` creation Also move this new and the existing test logging utilities to a `test::logging` module.

refactor(tests): use define_test_logger instead of manual creation

0ef8112

And move its definition to a `test` module instead of a `test_utils` to be in line with the latest naming scheme (only for crates that already have a `test` module).

refactor(aggregator): move test::mocks to test::double::mocks

f0f4f26

As mocks are test double, it should have been there in the first place

refactor(common): move FakeCertificaterRetriever to test::double

901f644

refactor(common): move crypto_helper test utilities to `test::crypt…

34ba519

…o_helper`

refactor(common): move DummyApiVersionDiscriminantSource to `test::…

6945748

…double`

refactor(common): remove test only `CardanoTransactionsSigningConfig:…

d9b1851

…:new` Has its value was low and it was even confusing since both parameters use the same type. This allow to remove the last usage of `cfg_test_tools` in mithril common, beside the one on the test module.

Alenar requested review from Copilot, dlachaume and turmelclem July 24, 2025 16:41

Alenar self-assigned this Jul 24, 2025

Alenar added refactoring 🛠️ Code refactoring and enhancements testing 🔁 Something related to tests labels Jul 24, 2025

github-advanced-security bot found potential problems Jul 24, 2025

View reviewed changes

mithril-common/src/test/mod.rs Fixed Show fixed Hide fixed

Copilot AI reviewed Jul 24, 2025

View reviewed changes

dlachaume approved these changes Jul 25, 2025

View reviewed changes

Alenar added 2 commits July 25, 2025 12:41

Alenar temporarily deployed to testing-preview July 25, 2025 11:24 — with GitHub Actions Inactive

turmelclem approved these changes Jul 25, 2025

View reviewed changes

Alenar merged commit 2ab36d3 into main Jul 25, 2025
70 of 72 checks passed

Alenar deleted the djo/2580/refactor-common-test-utils branch July 25, 2025 12:48

Alenar mentioned this pull request Jul 25, 2025

fix: mithril metric tests build #2654

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: common test utils #2650

refactor: common test utils #2650

Uh oh!

Alenar commented Jul 24, 2025 •

edited

Loading

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jul 24, 2025 •

edited

Loading

Uh oh!

dlachaume left a comment

Uh oh!

turmelclem left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

refactor: common test utils #2650

refactor: common test utils #2650

Uh oh!

Conversation

Alenar commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Content

Resulting structure

Pre-submit checklist

Comments

Next step

Issue(s)

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

dlachaume left a comment

Choose a reason for hiding this comment

Uh oh!

turmelclem left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Alenar commented Jul 24, 2025 •

edited

Loading

github-actions bot commented Jul 24, 2025 •

edited

Loading