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 p2p test timeout on coverage #1264

Merged
merged 2 commits into from
Oct 9, 2023

Conversation

azarovh
Copy link
Member

@azarovh azarovh commented Oct 8, 2023

This is the fix for the failed job that Boris reported earlier:
https://github.com/mintlayer/mintlayer-core/actions/runs/6425530789/job/17448253640?pr=1262

I see no other reason for the failure except that the syncing of 1000 blocks took more than 60s that is the standard timeout for the p2p tests. I suggest decreasing the number of processing blocks to 500 which won't make a difference for the quality of the test but allows to keep timeout the same.

@azarovh azarovh added the p2p p2p related issues label Oct 8, 2023
@TheQuantumPhysicist
Copy link
Collaborator

This error has nothing to do with the 60 seconds you're seeing first. That's just rust testing framework warning that a test took more than 60 seconds.

@TheQuantumPhysicist
Copy link
Collaborator

The error can be seen here:

2023-10-05T23:48:04.7806169Z 2023-10-05T23:48:02.104855Z  INFO initial_download_unexpected_disconnect: mempool::pool: new tip: block Id<Block>{0x3331e96a5d554fc6ae3e7e8e873e0864d4a7b4e777b50bf5d526d8342eff92bf} height BlockHeight(895)    
2023-10-05T23:48:04.7806521Z thread 'tokio-runtime-worker' panicked at 'Failed to receive value in time: Elapsed(())', p2p/src/sync/tests/helpers/mod.rs:501:9
2023-10-05T23:48:04.7806616Z stack backtrace:
2023-10-05T23:48:04.7807188Z 2023-10-05T23:48:02.205146Z  INFO initial_download_unexpected_disconnect: mempool::pool: mempool: Processing chainstate event NewTip(Id<Block>{0x62ef6e3e55101b0e071bd2374c87081e569a69192a1cbd33f43ed67e75da5644}, BlockHeight(896))    

@azarovh
Copy link
Member Author

azarovh commented Oct 9, 2023

The error can be seen here:

That's exactly the error that fires on timeout. Every test calls expect_future_val on SyncingEventReceiver with 60s timeout.

That panic happens exactly 60s from the start. Also you can see that only block at BlockHeight(895) was processed.

@TheQuantumPhysicist
Copy link
Collaborator

The error can be seen here:

That's exactly the error that fires on timeout. Every test calls expect_future_val on SyncingEventReceiver with 60s timeout.

That panic happens exactly 60s from the start. Also you can see that only block at BlockHeight(895) was processed.

Alrighty. I don't like the idea of reducing the test coverage just to appease a timeout. Maybe we should increase the timeout to 120 seconds. I think that's better.

@azarovh
Copy link
Member Author

azarovh commented Oct 9, 2023

Alrighty. I don't like the idea of reducing the test coverage just to appease a timeout. Maybe we should increase the timeout to 120 seconds. I think that's better.

Ok if you think so

@TheQuantumPhysicist TheQuantumPhysicist merged commit af63ce2 into master Oct 9, 2023
23 checks passed
@TheQuantumPhysicist TheQuantumPhysicist deleted the fix/p2p_test_timeout_on_coverage branch October 9, 2023 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2p p2p related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants