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

Fix quorum pre-image unit test #3182

Open
wants to merge 8 commits into
base: v0.x.x
Choose a base branch
from

Conversation

hewison-chris
Copy link
Contributor

@hewison-chris hewison-chris commented Mar 11, 2022

This test has often failed recently. See commits for details on fix.

@codecov
Copy link

codecov bot commented Mar 11, 2022

Codecov Report

Merging #3182 (0a0be38) into v0.x.x (0a0be38) will not change coverage.
The diff coverage is n/a.

❗ Current head 0a0be38 differs from pull request most recent head ba7f961. Consider uploading reports for the commit ba7f961 to get more accurate results

@@           Coverage Diff           @@
##           v0.x.x    #3182   +/-   ##
=======================================
  Coverage   88.68%   88.68%           
=======================================
  Files         165      165           
  Lines       17131    17131           
=======================================
  Hits        15192    15192           
  Misses       1939     1939           
Flag Coverage Δ
integration 39.42% <0.00%> (ø)
unittests 87.71% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@linked0 linked0 left a comment

Choose a reason for hiding this comment

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

LGTM

@hewison-chris hewison-chris force-pushed the fix_quorum_preimage_test branch from edc2eeb to 4fa452f Compare March 11, 2022 04:57
@hewison-chris
Copy link
Contributor Author

This test is never failing locally so is really hard to fix at the moment.

@linked0
Copy link
Contributor

linked0 commented Mar 11, 2022

This test is never failing locally so is really hard to fix at the moment.

If this fails again on CI even with this change, How about solving this problem with other approaches later.

@linked0
Copy link
Contributor

linked0 commented Mar 11, 2022

But if you think there are any improvements in the test result, it's not bad to merge it.

@hewison-chris hewison-chris force-pushed the fix_quorum_preimage_test branch 8 times, most recently from f13fa34 to c05fa03 Compare March 14, 2022 08:50
@hewison-chris hewison-chris self-assigned this Mar 14, 2022
@hewison-chris hewison-chris force-pushed the fix_quorum_preimage_test branch 8 times, most recently from 905f495 to 80e9ac5 Compare March 21, 2022 06:36
@hewison-chris hewison-chris force-pushed the fix_quorum_preimage_test branch 7 times, most recently from 47cdc41 to 5db20f9 Compare March 23, 2022 07:27
@hewison-chris hewison-chris force-pushed the fix_quorum_preimage_test branch 3 times, most recently from 4116762 to fdc6b0a Compare March 30, 2022 21:28
@hewison-chris hewison-chris force-pushed the fix_quorum_preimage_test branch from fdc6b0a to f797d13 Compare April 11, 2022 01:56
The max listeners should be the sum of the validators, fullnodes and
registry. In `base.d` it is correctly calculated so just remove the
`testConf` hardcoded setting.
The test relies on the fact that all Genesis and outsider validators
are enrolled in the last block of the cycle. Let's just assert this is
the case. Maybe we will need to refactor the test if this assert fails.
These steps should not be required as the next step has retries.
For blocks 21 -> 39 it is better to send the tx to one client and check
for the height in the test code as there are outsider validators. In the
`generateBlocks` function it checks that the txs are gossiped to all the
validators before setting the time for the next block. But as the
outsiders were not enrolled until block 20 the client may gossip
the tx before it has updated the peers it sends to. Normally this does
not matter because once an envelope is recieved by the newly enrolled
node it will try to fetch it.
If we use auto enroll then we do not need to manually enroll the
outsiders for their second enrollment.
@hewison-chris hewison-chris force-pushed the fix_quorum_preimage_test branch from f797d13 to ba7f961 Compare May 17, 2022 01:04
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.

2 participants