Skip to content
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

Sfa/improve signed entity test #1460

Merged
merged 12 commits into from Feb 9, 2024

Conversation

sfauvel
Copy link
Collaborator

@sfauvel sfauvel commented Jan 24, 2024

Content

Simplification proposal for signed_entity tests to improve understanding by hiding some technical code.

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
  • Documentation
    • Update README file (if relevant)
    • Update documentation website (if relevant)
    • Add dev blog post (if relevant)

Comments

To hide code needed to handle mocks, a MockDependencyInjector struct is added.
It creates and keeps all mocks needed during tests.
It provide functions that build struct with mocks.

Issue(s)

Relates to #YYY or Closes #YYY

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.

It looks like your branch is far behind as there should also be a build_artifact_for_cardano_transactions_store_nothing_in_db test to adapt in this module 🤔

Otherwise LGTM 👍

@sfauvel sfauvel force-pushed the sfa/improve_signed_entity_test branch from b1a90f0 to 3a22e8b Compare January 25, 2024 15:45
@sfauvel
Copy link
Collaborator Author

sfauvel commented Jan 25, 2024

It looks like your branch is far behind as there should also be a build_artifact_for_cardano_transactions_store_nothing_in_db test to adapt in this module 🤔

Otherwise LGTM 👍

I rebased to main

@sfauvel
Copy link
Collaborator Author

sfauvel commented Jan 29, 2024

I add 3 versions of the same test on create_artifact.
We have to choose the version to keep.

  • 3 independent tests, 1 for each artifact
  • 1 test that test create_artifact for each of the 3 artifacts
  • Create a generic function that test 1 artifact. This function can be used in one test or in 3 independent tests.

@sfauvel sfauvel force-pushed the sfa/improve_signed_entity_test branch from ef064a6 to 64543bb Compare January 31, 2024 09:45
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 👍

mithril-aggregator/src/services/signed_entity.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Alenar Alenar left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@jpraynaud
Copy link
Member

It looks like the cargo fmt is not happy yet 🙂
https://github.com/input-output-hk/mithril/actions/runs/7724637715/job/21059899630#step:6:18

@jpraynaud
Copy link
Member

@sfauvel can you fix the clippy warning and bump the patch version of the aggregator?
That's all that is missing before we can merge the PR 🙂

@sfauvel sfauvel force-pushed the sfa/improve_signed_entity_test branch 2 times, most recently from 28c4db0 to 6941472 Compare February 8, 2024 15:52
@sfauvel sfauvel force-pushed the sfa/improve_signed_entity_test branch from a636a55 to d9d71ae Compare February 9, 2024 13:05
@sfauvel sfauvel merged commit d711970 into input-output-hk:main Feb 9, 2024
26 of 30 checks passed
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.

None yet

4 participants