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.

Sign up for GitHub

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

Jump to bottom

common prefix test #8233

Merged
merged 38 commits into from Mar 31, 2021
Merged

common prefix test #8233

merged 38 commits into from Mar 31, 2021

Conversation

ghost-not-in-the-shell
Copy link
Member

Thank you for contributing to Mina! Please see CONTRIBUTING.md if you haven't
yet. In that doc, there are more details around how to start our CI.

Explain your changes here.

Explain how you tested your changes here.

Checklist:

  • Document code purpose, how to use it
    • Mention expected invariants, implicit constraints
  • Tests were added for the new behavior
    • Document test purpose, significance of failures
    • Test names should reflect their purpose
  • All tests pass (CI will check this if you didn't)
  • Serialized types are in stable-versioned modules
  • Does this close issues? List them:

Closes #0000
Closes #0000

@ghost-not-in-the-shell ghost-not-in-the-shell requested a review from a team as a code owner March 12, 2021 19:01
@ghost-not-in-the-shell ghost-not-in-the-shell changed the base branch from develop to compatible March 12, 2021 19:01
@ghost-not-in-the-shell ghost-not-in-the-shell added the ci-build-me Add this label to trigger a circle+buildkite build for this branch label Mar 12, 2021
psteckler
psteckler reviewed Mar 12, 2021
View reviewed changes
src/app/test_executive/common_prefix.ml Outdated

let check_common_prefixes prefixes =
let recent_chains =
List.map prefixes ~f:(take prefixes 5 |> Hash_set.of_list (module String))
Copy link
Member

Choose a reason for hiding this comment

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

it might be better to pass the prefix length to check_common_prefixes, perhaps with a default

psteckler
psteckler reviewed Mar 12, 2021
View reviewed changes
src/app/test_executive/common_prefix.ml Outdated
Malleable_error.List.iter block_producers ~f:(fun bp ->
wait_for t @@ Wait_condition.node_to_initialize bp )
in
let%bind.Async.Deferred.Let_syntax () =
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to wait for some number of blocks to be produced? That number could be the same or greater than the expected common prefix length.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. I would change it to wait for a certain amount of blocks.

@ghost-not-in-the-shell ghost-not-in-the-shell requested a review from a team as a code owner March 16, 2021 19:20
@aneesharaines
Copy link
Contributor

!approved-for-mainnet

@mrmr1993 mrmr1993 merged commit 5e51473 into compatible Mar 31, 2021
@mrmr1993 mrmr1993 deleted the integration_tests/common_prefix branch March 31, 2021 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@psteckler psteckler psteckler approved these changes

@nholland94 nholland94 nholland94 approved these changes

Assignees
No one assigned
Labels
ci-build-me Add this label to trigger a circle+buildkite build for this branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ghost-not-in-the-shell @aneesharaines @psteckler @nholland94 @mrmr1993 @QuiteStochastic