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(tests): submit_withdrawal_request random failure #774

Merged

Conversation

zeroqn
Copy link
Contributor

@zeroqn zeroqn commented Jul 28, 2022

Withdrawal request is pushed to mem pool immediately
https://github.com/nervosnetwork/godwoken/runs/7533962286?check_suite_focus=true#step:9:741

In this case, poll withdrawal until it's pushed to mem pool.

@zeroqn zeroqn requested a review from blckngm July 28, 2022 07:59
@gw-bot

This comment was marked as outdated.

@zeroqn zeroqn requested review from Flouse and magicalne July 28, 2022 08:01
if !is_in_queue {
chain.produce_block(vec![], vec![withdrawal]).await.unwrap();
} else {
chain.produce_block(vec![], vec![]).await.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the withdrawal request remains in the queue for a while? The mem pool state subsequently loaded won't include it, right?

Copy link
Contributor Author

@zeroqn zeroqn Jul 28, 2022

Choose a reason for hiding this comment

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

Maybe. By default produce_block fn will refresh mem pool, it will be included if it's pushed to mem pool. (aka produce_block fail to acquire lock)
Add new produce_block_and_refresh_mem_pool to allow skip refresh mem pool.

Copy link
Contributor

Choose a reason for hiding this comment

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

My point is that this test is not waiting for the withdrawal to be actually pushed to the mem pool.

I think it should poll until the request is not in queue.

The latest commit still fails if I add a sleep before locking mem pool in line 602 in registry.rs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Fixed.

@gw-bot

This comment was marked as outdated.

@zeroqn zeroqn force-pushed the fix-tests-rpc-submit-withdrawal-failure branch from 3d54888 to 9e87fe0 Compare July 28, 2022 10:25
@gw-bot

This comment was marked as outdated.

jjyr
jjyr previously approved these changes Jul 28, 2022
@jjyr jjyr requested a review from blckngm July 28, 2022 10:50
blckngm
blckngm previously approved these changes Jul 28, 2022
Flouse
Flouse previously approved these changes Jul 28, 2022
@zeroqn zeroqn dismissed stale reviews from Flouse, blckngm, and jjyr via 47ea1f3 July 28, 2022 11:16
@zeroqn zeroqn force-pushed the fix-tests-rpc-submit-withdrawal-failure branch from 9e87fe0 to 47ea1f3 Compare July 28, 2022 11:16
@gw-bot
Copy link

gw-bot bot commented Jul 28, 2022

Running integration test

Workflow Run Id: 2753361099

Components:

Manually running integration test

Post a comment contains

/itest
[prebuilds: tag]
[godwoken: branch/ref]
[scripts: branch/ref]
[polyjuice: branch/ref]
[web3: branch/ref]
[kicker: branch/ref]
[tests: branch/ref]

Note: [] means optional, for example

/itest
prebuilds: dev-202203280240
godwoken: develop
scripts: 81676d9d53ffdf5bbaa60483928d07da16eb4a88
polyjuice: e37553b9

Run Result

success

@jjyr jjyr requested review from Flouse and blckngm July 28, 2022 14:08
@Flouse Flouse merged commit d053221 into godwokenrises:develop Jul 29, 2022
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.

5 participants