Skip to content

Flaky tests #374

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 11 commits into from
Aug 26, 2022
Merged

Flaky tests #374

merged 11 commits into from
Aug 26, 2022

Conversation

iquerejeta
Copy link
Contributor

@iquerejeta iquerejeta commented Jul 27, 2022

To resolve the flaky_tests issue, we simply need to use the portable feature from blst. I've made the portable feature optional in all crates, and it is activated only in the CI, but not by default.

With the resolution of the flaky tests with the blast backend, we fully remove the zcash backend.

This closes #207

@github-actions
Copy link

github-actions bot commented Jul 27, 2022

Unit Test Results

    7 files  ±0    22 suites  ±0   2m 5s ⏱️ - 3m 5s
287 tests ±0  287 ✔️ ±0  0 💤 ±0  0 ±0 
288 runs  ±0  288 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 85ca5a0. ± Comparison against base commit 2c28687.

♻️ This comment has been updated with latest results.

@iquerejeta
Copy link
Contributor Author

Tests still fail even with the updated version of blst dependency :( @jpraynaud

I would suggest that we keep this open regardless, so that we can share this concern with the blast developers. If there is no further way for us to understand why this is happening, we should remove the feature flag, and the dependency altogether.

@iquerejeta
Copy link
Contributor Author

We have our hopes high again with the solution proposed by @Alenar. In particular using these environment flags for the github actions.

@iquerejeta iquerejeta force-pushed the flaky_tests branch 4 times, most recently from 5507822 to 6c72e57 Compare August 23, 2022 15:37
@iquerejeta
Copy link
Contributor Author

I always get these errors in this branch, of assertions failed or no quorum reached.. Are you aware of this happening elsewhere? Or is it only happening in this branch?

cc: @Alenar @jpraynaud

@jpraynaud
Copy link
Member

jpraynaud commented Aug 23, 2022

@iquerejeta, I think there was a problem with the Protocol Parameters number of lotteries that were tried (10 lotteries instead of the intended 100, with 5 signers that could easily lead to not getting the quorum of 5). I also fixed the broken test in the Signer.

I launched several times the CI jobs and it went all green 100% (see)

However, the fact that the behavior is not the same with blast and zcash backends should be probably investigated further 🤔

@iquerejeta iquerejeta marked this pull request as ready for review August 24, 2022 07:42
@iquerejeta iquerejeta requested review from jpraynaud and Alenar August 24, 2022 07:42
@iquerejeta
Copy link
Contributor Author

To resolve the flaky_tests issue, I've made the portable feature optional in all crates, and it is activated only in the CI, but not by default. To simplify the code, I've also exposed mithril-core in mithril-common, so that we don't need to import both. Happy to revert that if you don't think it is best 👍

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 🥳

Let's merge tomorrow!

@jpraynaud
Copy link
Member

I have aligned the portable feature on the mithril-aggregator to be the same as on mithril-signer and mithril-client:
portable = ["mithril-common/portable"] and also rebased the branch on main

@iquerejeta iquerejeta merged commit 0b7b547 into main Aug 26, 2022
@iquerejeta iquerejeta deleted the flaky_tests branch August 26, 2022 08:15
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.

Flaky Tests CI
2 participants