Skip to content
New issue

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

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

Already on GitHub? # to your account

Align messages golden master tests #2221

Merged
merged 6 commits into from
Jan 14, 2025

Conversation

Alenar
Copy link
Collaborator

@Alenar Alenar commented Jan 13, 2025

Content

This PR align all mithril-common messages golden master test to the same test model:

  • A const CURRENT_JSON string containing a exhaustive example of json currently exchanged between nodes
  • A golden_message_current method that returns the representation of the CURRENT_JSON using the current structure
  • A test_current_json_deserialized_into_current_message test that check that de-serializing the CURRENT_JSON into the current structure yield the output stored in golden_message_current
  • For each supported backward compatible previous version of a structure (example based on the CertificateMessage):
    • A copy of the previous version structure as it was before the backward-compatible change, the copy is suffixed with the last version of the openapi before the change, ie: CertificateMessageUntilV0_1_32
    • A golden_message_until_open_api_X_Y_ZZ method that returns the representation of the CURRENT_JSON using the previous structure (also suffixed)
    • A test_current_json_deserialized_into_message_supported_until_open_api_X_Y_ZZ test that check that de-serializing the CURRENT_JSON into the previous structure yield the output stored in golden_message_until_open_api_X_Y_ZZ.

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
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

Issue(s)

Closes #2217

@Alenar Alenar added the testing 🔁 Something related to tests label Jan 13, 2025
@Alenar Alenar self-assigned this Jan 13, 2025
Copy link

github-actions bot commented Jan 13, 2025

Test Results

    4 files  ±0     52 suites  ±0   10m 29s ⏱️ -3s
1 493 tests  - 1  1 493 ✅  - 1  0 💤 ±0  0 ❌ ±0 
1 749 runs   - 1  1 749 ✅  - 1  0 💤 ±0  0 ❌ ±0 

Results for commit 71b01e9. ± Comparison against base commit 584e2c6.

This pull request removes 35 and adds 34 tests. Note that renamed tests count towards both.
mithril-common ‑ messages::aggregator_features::tests::test_actual_json_deserialized_into_actual_message
mithril-common ‑ messages::aggregator_features::tests::test_actual_json_deserialized_into_previous_message
mithril-common ‑ messages::aggregator_status::tests::test_actual_json_deserialized_into_actual_message
mithril-common ‑ messages::cardano_database::tests::test_actual_json_deserialized_into_actual_message
mithril-common ‑ messages::cardano_database_digest_list::tests::test_actual_json_deserialized_into_actual_message
mithril-common ‑ messages::cardano_database_list::tests::test_actual_json_deserialized_into_actual_message
mithril-common ‑ messages::cardano_stake_distribution::tests::test_v1
mithril-common ‑ messages::cardano_stake_distribution_list::tests::test_v1
mithril-common ‑ messages::cardano_transaction_snapshot::tests::test_v1
mithril-common ‑ messages::cardano_transaction_snapshot_list::tests::test_v1
…
mithril-common ‑ messages::aggregator_features::tests::test_current_json_deserialized_into_current_message
mithril-common ‑ messages::aggregator_features::tests::test_current_json_deserialized_into_message_supported_until_open_api_0_1_27
mithril-common ‑ messages::aggregator_status::tests::test_current_json_deserialized_into_current_message
mithril-common ‑ messages::cardano_database::tests::test_current_json_deserialized_into_current_message
mithril-common ‑ messages::cardano_database_digest_list::tests::test_current_json_deserialized_into_current_message
mithril-common ‑ messages::cardano_database_list::tests::test_current_json_deserialized_into_current_message
mithril-common ‑ messages::cardano_stake_distribution::tests::test_current_json_deserialized_into_current_message
mithril-common ‑ messages::cardano_stake_distribution_list::tests::test_current_json_deserialized_into_current_message
mithril-common ‑ messages::cardano_transaction_snapshot::tests::test_current_json_deserialized_into_current_message
mithril-common ‑ messages::cardano_transaction_snapshot_list::tests::test_current_json_deserialized_into_current_message
…

♻️ This comment has been updated with latest results.

@Alenar Alenar temporarily deployed to testing-preview January 13, 2025 17:27 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet January 13, 2025 17:27 — 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 👍

Maybe the naming actual could be replaced with current or latest?

Copy link
Collaborator

@sfauvel sfauvel left a comment

Choose a reason for hiding this comment

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

LGTM
We may keep the PR explanation of how we test messages in another place like ADR or documentation.

@Alenar Alenar force-pushed the djo/2217/align_messages_golden_master_tests branch from e88d4cb to d926078 Compare January 14, 2025 09:27
@Alenar Alenar temporarily deployed to testing-preview January 14, 2025 09:36 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet January 14, 2025 09:36 — with GitHub Actions Inactive
As it wrongly tested that a previous structure could be deserialized into
the actual
We want to test the other way, that the actual structure can be
deserialized into older's, and in this case they were incompatible (the
previous structure asked for fields that were removed).
Now they all specify at minimum that they target the 'actual' version
and if we test a compatibility with previous messages structure the
related openapi version is specified.
@Alenar Alenar force-pushed the djo/2217/align_messages_golden_master_tests branch 2 times, most recently from 908071a to d4119de Compare January 14, 2025 10:50
@Alenar Alenar force-pushed the djo/2217/align_messages_golden_master_tests branch from d4119de to 71b01e9 Compare January 14, 2025 11:06
@Alenar Alenar temporarily deployed to testing-preview January 14, 2025 11:14 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet January 14, 2025 11:14 — with GitHub Actions Inactive
@Alenar Alenar merged commit ff02ea5 into main Jan 14, 2025
43 checks passed
@Alenar Alenar deleted the djo/2217/align_messages_golden_master_tests branch January 14, 2025 11:16
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
testing 🔁 Something related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Align golden tests implementations in messages
4 participants