Resolve some falky tests and improve CI times #2401
+569
−525
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AurelienFT
added
the
no changelog
AurelienFT
changed the title
…ow manual in future commit
AurelienFT
changed the title
AurelienFT
requested review from
rafal-ch,
xgreenx and
Dentosal
as code owners
rymnc
reviewed
Nov 12, 2024
rymnc
previously approved these changes
Nov 12, 2024
xgreenx
reviewed
Nov 12, 2024
MitchTurner
reviewed
Nov 13, 2024
|
For clarity, this PR will not solve all the problems but at least a big chunks of it and is a big step forward we could do a round 2 with everything we catch in the next days. |
rymnc
previously approved these changes
Nov 13, 2024
xgreenx
reviewed
Nov 13, 2024
| // When | ||
| for _ in 0..1000 { | ||
| let result = executor.validate(&block).map(|_| ()); | ||
|
|
||
| if start.elapsed().as_secs() > 60 { |
There was a problem hiding this comment.
Suggested change
| if start.elapsed().as_secs() > 60 { | |
| if start.elapsed().as_secs() > 1 { |
There was a problem hiding this comment.
It doesn't get reset between each loop and before the timeout was 60 sec for the whole test, no ?
There was a problem hiding this comment.
Oh, yeah, it is true. Maybe it is better to create start before each executor.validate and check that it less than a second
| // When | ||
| for _ in 0..1000 { | ||
| let result = executor.validate(&block).map(|_| ()); | ||
|
|
||
| if start.elapsed().as_secs() > 60 { |
There was a problem hiding this comment.
Suggested change
| if start.elapsed().as_secs() > 60 { | |
| if start.elapsed().as_secs() > 1 { |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Linked Issues/PRs
Fix #2408
Fix #2407
Fix #2406
Fix #2351
Fix #2393
Fix #2394
Fix #2395
Description
This PR fix an issue in P2P heartbeat. The problem was that P2P heartbeat was updated only if new blocks were received or produced. This means that if we start the node from an existing db but doesn't produce blocks and not connect it to anyone it will send block height 0 to the peers that connects to him. We believe that this fix, resolves #2408 #2407 #2406 and #2351.
For #2394 we just increased the timeouts.
For #2393 we removed the panic in the test and just let p2p reconnect
For #2395 we launch this test using multi-threads mode of Tokio to follow the convention of all the others tests that launch a node using
FuelCoreDriver. Also we added a kill of the driver to try to kill the node in a more graceful way in all of the test, it should fix a lot of flakyness in these testsThis PR also change the CI workflow by removing all docker related jobs and codecov job. These two set of jobs has been moved to separated workflow that are not triggered automatically but can be triggered manually on the "Actions" tab of this repository (after the merge of this PR).
The tests launched by the CI job now use
nextestthat allow us to add timeout for each test and provide more detailed output. The timeout is currently 5 min (and 8 for two really big tests) because we have tests that take a long time but we should lower it in the future.The steps on the matrix are not cancelled anymore when one failed to allow possible other success and cache their success for a relaunch of the tests.
There is still more improve to do on our tests especially on timeout and rapid execution but this should improve a lot our workflow.
Checklist
Before requesting review