Skip to content

test: use a in memory logger for tests that needs to check logs #2442

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Apr 29, 2025

Conversation

Alenar
Copy link
Collaborator

@Alenar Alenar commented Apr 28, 2025

Content

This PR add the MemoryDrainForTest that write log to memory, it allow to remove the need to write logs to files for tests that needs to check log content.

Logging to file in test was error prone as it was difficult to guarantee that all logs were effectively flushed to the file before the assertions, leading to error, often in the hydra ci:

Details

  • mithril-common: Add MemoryDrainForTest in test tools
    • A slog::Drain that write logs to in memory Vec<String> shared using a Arc<RwLock<..>>
    • Optimized for tests, not speed, it should not be used in production.
    • It's created along a MemoryDrainForTestInspector:
      • it can search for log entries or if a log was written
      • this struct allow to move the MemoryDrainForTest to the logger used by tested component while providing an api to do the check needed by tests
  • Replace all TestLogger::file in all crates with a TestLogger::memory that returns a tuple containing a logger that logs to MemoryDrainForTest alongside a MemoryDrainForTestInspector to do assertions on logs
  • mithril-client: Remove slog level configuration in dev-dependencies and use slog default instead. This was making the test log_a_info_message_telling_that_the_feature_does_not_use_mithril_certification fail because hydra run test in --release removing the log that was tested (cf: hydra run)

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
    • No new TODOs introduced

Issue(s)

Relates to #2436

@Alenar Alenar self-assigned this Apr 28, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces an in-memory logging solution for tests by replacing file‐based logging with MemoryDrainForTest and its corresponding inspector. Key changes include:

  • Updating test setup in multiple modules (mithril-signer, mithril-client, mithril-aggregator) to use TestLogger::memory.
  • Removing file-based log flushing blocks and associated file I/O in favor of in-memory log inspection.
  • Introducing a new memory_logger module in mithril-common and updating relevant test_utils components.

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.

Show a summary per file
File Description
mithril-signer/src/services/upkeep_service.rs Updated test cases to use in-memory logging.
mithril-signer/src/services/aggregator_client.rs Replaced file-based logger with in-memory logger in tests.
mithril-signer/src/runtime/error.rs Updated error logging tests to use MemoryDrainForTest.
mithril-signer/src/lib.rs Modified test_tools module to support in-memory logging.
mithril-common/src/test_utils/mod.rs & memory_logger.rs Added and integrated a new in-memory logger for test utilities.
mithril-common/src/logging.rs Updated tests to verify log content using the in-memory logger.
mithril-client/src/snapshot_client.rs & lib.rs Switched file logger to memory logger and updated corresponding assertions.
mithril-aggregator/* Adjusted tests in upkeep, artifact builders, and runtime errors to use the new memory logger.
Comments suppressed due to low confidence (1)

path/to/file/with/immmutable_file_number_extractor.rs:1

  • The function name 'immmutable_file_number_extractor' appears to have a typo with an extra 'm'. Consider renaming it to 'immutable_file_number_extractor'.
fn immmutable_file_number_extractor(file_uri: &str) -> StdResult<Option<String>> {

@Alenar Alenar force-pushed the djo/2436/hydra-ci-flaky-on-file-logger branch from f2442ad to ec62b6e Compare April 28, 2025 14:01
Copy link

github-actions bot commented Apr 28, 2025

Test Results

    3 files  ±0     57 suites  ±0   11m 26s ⏱️ -7s
1 880 tests +5  1 880 ✅ +5  0 💤 ±0  0 ❌ ±0 
2 342 runs  +5  2 342 ✅ +5  0 💤 ±0  0 ❌ ±0 

Results for commit 3917613. ± Comparison against base commit 29719ca.

♻️ This comment has been updated with latest results.

@Alenar Alenar force-pushed the djo/2436/hydra-ci-flaky-on-file-logger branch from ec62b6e to 6a75be8 Compare April 28, 2025 15:00
@Alenar Alenar temporarily deployed to testing-preview April 28, 2025 15:09 — with GitHub Actions Inactive
Copy link
Collaborator

@dlachaume dlachaume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@Alenar Alenar force-pushed the djo/2436/hydra-ci-flaky-on-file-logger branch from 6a75be8 to 76a0b3e Compare April 29, 2025 09:42
@Alenar Alenar temporarily deployed to testing-preview April 29, 2025 09:51 — with GitHub Actions Inactive
@Alenar Alenar force-pushed the djo/2436/hydra-ci-flaky-on-file-logger branch from 76a0b3e to 5b343ef Compare April 29, 2025 10:21
@Alenar Alenar temporarily deployed to testing-preview April 29, 2025 10:30 — with GitHub Actions Inactive
Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@Alenar Alenar requested a review from Copilot April 29, 2025 12:39
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the testing infrastructure to use an in-memory logger instead of writing logs to files, improving test stability by ensuring log messages are flushed immediately. In addition, a minor renaming correction was applied to fix an identifier typo.

  • Replaces file-based logging with memory logging across multiple test suites.
  • Updates Cargo.toml logging configurations and refactors test utilities to use the new memory logger.
  • Corrects the name of the function from “immmutable_file_number_extractor” to “immutable_file_number_extractor”.

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.

Show a summary per file
File Description
mithril-signer/src/services/upkeep_service.rs Replaced file logger with in-memory logger in test_cleanup_database and related tests.
mithril-signer/src/services/aggregator_client.rs Updated test logger usage from file to memory in aggregator client tests.
mithril-signer/src/runtime/error.rs Refactored tests to use memory logger instead of file logging.
mithril-signer/src/lib.rs Updated test_tools to include memory logger functionality.
mithril-relay/Cargo.toml Changed logging feature from “release_max_level_trace” to “release_max_level_debug”.
mithril-common/src/test_utils/mod.rs Updated test utilities to use the memory logger and exported the new module.
mithril-common/src/test_utils/memory_logger.rs New module providing an in-memory logging drain and inspector with concurrent log support.
mithril-common/src/logging.rs Updated tests to validate output from the memory logger instead of file-based logger.
mithril-client/src/snapshot_client.rs & lib.rs & Cargo.toml Refactored tests to utilize memory logger for log assertions.
mithril-aggregator/src/services/upkeep.rs Replaced file-based logging with memory logger throughout test cases.
mithril-aggregator/src/runtime/error.rs & lib.rs Updated tests to assert log messages via the memory logger inspector.
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/immutable.rs Fixed identifier typo and updated logger usage in tests.
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/digest.rs Updated logger usage to validate log content via memory logger.
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/ancillary.rs Updated logger usage to validate log content via memory logger.

Alenar added 6 commits April 29, 2025 14:41
…encies

Warning: not optimised for speed so it may cause slowdown or issue if
used in heavely async/threaded contexts.
instead of logging to a file, which can cause issue in some context
(i.e: the log file doens't seems to always be correctly flushed).
slog remove trace and debug level logs at compile time when targeting
release.
This means that we should at maximum use info level for logs that are
checked in tests else those tests would fail if run with `--release`
(such as in the hydra ci).
trace were enabled on release build since the relay was considered as
unstable at start, since it has been stable for some time now we can
align it to other bin crate (debug: all level enabled, release: max
level debug).
* mithril-aggregator from `0.7.43` to `0.7.44`
* mithril-client from `0.11.23` to `0.11.24`
* mithril-common from `0.5.26` to `0.5.27`
* mithril-relay from `0.1.41` to `0.1.42`
* mithril-signer from `0.2.242` to `0.2.243`
@Alenar Alenar force-pushed the djo/2436/hydra-ci-flaky-on-file-logger branch from 5b343ef to 3917613 Compare April 29, 2025 12:55
@Alenar Alenar temporarily deployed to testing-preview April 29, 2025 13:05 — with GitHub Actions Inactive
@Alenar Alenar merged commit 9f4f380 into main Apr 29, 2025
38 checks passed
@Alenar Alenar deleted the djo/2436/hydra-ci-flaky-on-file-logger branch April 29, 2025 13:09
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.

3 participants