Skip to content

Split mithril-common phase 2: extract Mithril test http server #2594

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 8 commits into from
Jun 20, 2025

Conversation

Alenar
Copy link
Collaborator

@Alenar Alenar commented Jun 19, 2025

Content

This PR extract the TestHttpServer tooling from mithril-common into a dedicated internal/tests/mithril-test-http-server crate.

Additional changes

  • Removal fs, apispec, test_http_server from mithril-common
  • Bump of mithril-common minor version to 0.6.0
  • The http server used in mithril-client integration tests have been switched from TestHttpServer to axum_test::TestServer
    • This mean that the routes code had to be converted from warp to axum
    • This have the udge benefits of avoiding a publication of the new mithril-test-http-server to crates.io
  • Reactivation of testing of most internal crates in the CI when targeting windows
  • Build and test missing, almost all, internal crates in Hydra CI
  • Refactor MetricServer init in a two part process MetricServer::build(ip, port, {args..}).bind().serve() in order to be able to get the effective bound port even if 0 is provided to let the system pick a available port.

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

Comments

Issue(s)

Relates to #2392

@Alenar Alenar self-assigned this Jun 19, 2025
@Alenar Alenar added the refactoring 🛠️ Code refactoring and enhancements label Jun 19, 2025
Copilot

This comment was marked as outdated.

Copy link

github-actions bot commented Jun 19, 2025

Test Results

    3 files  ±  0    102 suites  +8   16m 18s ⏱️ + 2m 48s
2 036 tests ±  0  2 036 ✅ ±  0  0 💤 ±0  0 ❌ ±0 
4 209 runs  +507  4 209 ✅ +507  0 💤 ±0  0 ❌ ±0 

Results for commit 72dd3e4. ± Comparison against base commit 93d00d6.

This pull request removes 4 and adds 4 tests. Note that renamed tests count towards both.
mithril-common ‑ test_utils::test_http_server::tests::test_server_simple_http
mithril-common ‑ test_utils::test_http_server::tests::test_server_simple_json
mithril-common ‑ test_utils::test_http_server::tests::test_server_specific_route
mithril-common ‑ test_utils::test_http_server::tests::test_server_unbind_route_yield_404
mithril-test-http-server ‑ test_http_server::tests::test_server_simple_http
mithril-test-http-server ‑ test_http_server::tests::test_server_simple_json
mithril-test-http-server ‑ test_http_server::tests::test_server_specific_route
mithril-test-http-server ‑ test_http_server::tests::test_server_unbind_route_yield_404

♻️ This comment has been updated with latest results.

@Alenar Alenar force-pushed the djo/2392/split_common/extract-test-http-server branch 2 times, most recently from a31280e to 591a242 Compare June 19, 2025 10:36
Copy link
Collaborator

@turmelclem turmelclem 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/2392/split_common/extract-test-http-server branch from 591a242 to 9e66893 Compare June 19, 2025 13:51
@Alenar Alenar force-pushed the djo/2392/split_common/extract-test-http-server branch from 9e66893 to 1720796 Compare June 19, 2025 14:38
@Alenar Alenar requested a review from Copilot June 19, 2025 14:40
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 extracts the TestHttpServer tooling from mithril‐common into a dedicated internal/tests/mithril-test-http-server crate and updates related tests and components to use the new axum‐based API and builder patterns. Key changes include:

  • Renaming and updating MetricsServer API calls from .start() to .serve() via a builder.
  • Migrating test server return types in FakeAggregator’s spawn methods from TestHttpServer to Self.
  • Updating client tests to use the new server URL accessor (server_root_url) and corresponding dependency updates.

Reviewed Changes

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

Show a summary per file
File Description
mithril-signer/src/main.rs Updated MetricsServer instantiation and method call to match the new builder API.
mithril-relay/src/relay/signer.rs Adjusted import paths to reference the new mithril-test-http-server crate.
mithril-client/tests/* Updated FakeAggregator spawn methods and client URL accessor to use the new server API.
mithril-common/Cargo.toml, mithril-aggregator/Cargo.toml Modified dependency definitions to include and reference the new test-http-server crate.
internal/tests/mithril-test-http-server/* New crate providing dedicated test HTTP server functionality.
Comments suppressed due to low confidence (3)

mithril-signer/src/main.rs:284

  • The MetricsServer API call has been updated from .start() to .serve() via a builder. Ensure that the new builder pattern and graceful shutdown behavior are correctly integrated here.
            .serve(stop_rx_clone)

mithril-client/tests/extensions/fake_aggregator/certificate.rs:8

  • The spawn_with_certificate method's return type has been changed from TestHttpServer to Self. Verify that all consumers of this method are updated to handle the new return type accordingly.
    pub fn spawn_with_certificate(certificate_hash_list: &[String]) -> Self {

mithril-client/tests/snapshot_list_get_show_download_verify.rs:38

  • The client initialization now uses fake_aggregator.server_root_url() instead of test_http_server.url(), reflecting the updated test server API. Confirm that this change is consistent across all affected tests.
        ClientBuilder::aggregator(&fake_aggregator.server_root_url(), genesis_verification_key)

@Alenar Alenar marked this pull request as ready for review June 19, 2025 14:47
@Alenar Alenar temporarily deployed to testing-preview June 19, 2025 14:48 — 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 added 8 commits June 20, 2025 11:08
This allow to remove dependency to `mithril-test-http-server` and
instead use a well used crate already in crates.io, avoiding the need to
publish our own crate.
They are deprecated and even if mithril-common is published to crates.io
it's considered an internal crates, users should not use it directly, so
we may remove them without warning.
* mithril-cardano-node-internal-database from `0.1.0` to `0.1.1`
* mithril-build-script from `0.2.22` to `0.2.23`
* mithril-metric from `0.1.14` to `0.1.15`
* mithril-resource-pool from `0.0.5` to `0.0.6`
* mithril-aggregator from `0.7.65` to `0.7.66`
* mithril-client from `0.12.16` to `0.12.17`
* mithril-relay from `0.1.43` to `0.1.44`
* mithril-signer from `0.2.254` to `0.2.255`
@Alenar Alenar force-pushed the djo/2392/split_common/extract-test-http-server branch from 1720796 to 72dd3e4 Compare June 20, 2025 09:20
@Alenar Alenar temporarily deployed to testing-preview June 20, 2025 09:41 — with GitHub Actions Inactive
@Alenar Alenar merged commit f28a611 into main Jun 20, 2025
66 of 67 checks passed
@Alenar Alenar deleted the djo/2392/split_common/extract-test-http-server branch June 20, 2025 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring 🛠️ Code refactoring and enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants