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

Move Merkle ledger tests to Alcotest #15744

Merged
merged 5 commits into from
Jun 19, 2024
Merged

Conversation

rbonichon
Copy link
Contributor

Use alcotest to express tests for Merkle ledger(s).

Sneaked in a small update in one the test Modules.

@rbonichon rbonichon requested a review from a team as a code owner June 13, 2024 14:45
@rbonichon rbonichon self-assigned this Jun 13, 2024
@rbonichon
Copy link
Contributor Author

!ci-build-me

Copy link
Member

@svv232 svv232 left a comment

Choose a reason for hiding this comment

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

the refactor logic makes sense to me, but i think this is breaking dev unit tests right now. The syncable ledger tests seem to reference merkle_ledger_tests.

@rbonichon
Copy link
Contributor Author

!ci-build-me

1 similar comment
@rbonichon
Copy link
Contributor Author

!ci-build-me

- dune: sort dependencies by alphabetical order
- dune: removed dependency on ppx_inline_test
Avoid mixing sprintf and ^ string concatenation operator
@rbonichon rbonichon force-pushed the rb/alcotest_merkle_ledger_tests branch from 71e63ac to 070a164 Compare June 14, 2024 13:30
@rbonichon
Copy link
Contributor Author

!ci-build-me

1 similar comment
@rbonichon
Copy link
Contributor Author

!ci-build-me

@rbonichon rbonichon requested a review from svv232 June 17, 2024 07:37
@rbonichon
Copy link
Contributor Author

!ci-build-me

@@ -59,12 +59,12 @@ module Hash = struct
* important impossible to create an account such that (merge a b = hash_account account) *)

let hash_account account =
Md5.digest_string ("0" ^ Format.sprintf !"%{sexp: Account.t}" account)
Md5.digest_string (Format.sprintf !"0%{sexp: Account.t}" account)
Copy link
Member

Choose a reason for hiding this comment

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

Is this the only non-refactoring code modification of this PR @rbonichon ?

Copy link
Contributor Author

@rbonichon rbonichon Jun 18, 2024

Choose a reason for hiding this comment

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

Kind of. This is also refactoring, isn't it?. But this is why there is a different commit for this change. We could remove it. But I'm not sure it's worth making a separate PR for this specific change.

Copy link
Member

Choose a reason for hiding this comment

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

it's alright to keep it as is

src/lib/merkle_ledger_tests/test_mask.ml Outdated Show resolved Hide resolved
accum + Balance.to_nanomina_int (Account.balance acct)
in
let parent_sum =
Maskable.foldi maskable ~init:0 ~f:balance_summer
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Maskable.foldi maskable ~init:0 ~f:balance_summer
Maskable.fold maskable ~init:0 ~f:balance_summer

and remove unused parameter from the balance_summer function (same modification could be applied below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately fold does not exist.

src/lib/merkle_ledger_tests/test_mask.ml Outdated Show resolved Hide resolved
@@ -156,477 +172,505 @@ module Make (Test : Test_intf) = struct
in
test_hashes_at_address addr

let%test "parent, mask agree on set" =
Copy link
Member

Choose a reason for hiding this comment

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

Do I understand correctly that this test was written twice (same code, duplicated)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly

@rbonichon
Copy link
Contributor Author

!ci-build-me

@rbonichon rbonichon merged commit b92247d into develop Jun 19, 2024
46 checks passed
@rbonichon rbonichon deleted the rb/alcotest_merkle_ledger_tests branch June 19, 2024 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants