Skip to content

Improve node network test #11867

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Dec 19, 2017
Merged

Improve node network test #11867

merged 5 commits into from
Dec 19, 2017

Conversation

jnewbery
Copy link
Contributor

Fixes race condition in the node_network_limited test case introduced in #11740. Also tidies up the test and removes redundant duplicate tests.

@jonasschnelli
Copy link
Contributor

utACK 9bb57c926665ecfeb61b185563757b3bd3452cbe
Would prefer a squash.

@jnewbery
Copy link
Contributor Author

Would prefer a squash.

Was just updating the original comment to say "Split into small commits for ease of review, but can be squashed into a single commit for merge." 🙂

@laanwj
Copy link
Member

laanwj commented Dec 17, 2017

utACK.

I'm ok with leaving this as seperate commits, however ecfe41d fails on its own:

2017-12-17 10:40:04.636000 TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
  File "/store/orion/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 118, in main
    self.run_test()
  File "test/functional/node_network_limited.py", line 52, in run_test
    assert_equal(self.getSignaledServiceFlags(), 1036)  # 1036 == 0x40C == 0100 0000 1100
AttributeError: 'NodeNetworkLimitedTest' object has no attribute 'getSignaledServiceFlags'

@promag
Copy link
Contributor

promag commented Dec 17, 2017

Yeah, separate commits make it easy to review IMO.

@jnewbery jnewbery force-pushed the improve_node_network_test branch from 9bb57c9 to b9f8840 Compare December 18, 2017 01:22
@jnewbery
Copy link
Contributor Author

however ecfe41d fails on its own

Thanks @laanwj . Now fixed.

@maflcko maflcko mentioned this pull request Dec 19, 2017
Merged
@maflcko
Copy link
Member

maflcko commented Dec 19, 2017

Sorry, needs rebase. Didn't notice there was already a pull open to fixup this test.

@jnewbery
[tests] fix flake8 warnings in node_network_limited.py
1285312
@jnewbery
[tests] define NODE_NETWORK_LIMITED in test framework
dbfe294
@jnewbery
[tests] node_network_limited - remove race condition
2e02984 
node_network_limited had a race condition, since wait_for_block()
doesn't do what you might expect. It only checks the most recent block
received over the P2P interface (perhaps we should rename the method
wait_for_most_recent_block() to avoid future confusion). The test can
fail if the node sends us invs for other blocks, we respond with a
getdata, and the node sends us one of those blocks in the 0.05 second
wait_until loop window.

Fix this by not responding to inv messages with getdata messages.
@jnewbery
[tests] remove redundant duplicate tests from node_network_limited
b425131
@jnewbery
[tests] refactor node_network_limited
ee5efad
@jnewbery jnewbery force-pushed the improve_node_network_test branch from b9f8840 to ee5efad Compare December 19, 2017 16:25
@jnewbery
Copy link
Contributor Author

rebased

@maflcko
Copy link
Member

maflcko commented Dec 19, 2017

Thanks for the quick rebase. Checked that each commit does what it advertises and the tests pass. The refactoring will make the test twice as fast.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

utACK ee5efad6cfb60d8efe678b1a9285a73d265ea79b
-----BEGIN PGP SIGNATURE-----

iQIcBAEBCgAGBQJaOXMsAAoJENLqSFDnUoslz6MP/iidLoKZOE32Nyl/Sbxf8+WN
BOQyFIZtXVarlLu3CiPfQId+lvaVlBPMRU0JEmp4JF0KAqWUsEgYphlQ2R0SBuvK
QnKiBJ0P4XazREr/yHVPTgog2S/SehfDN7hWwVcGrpyX1ryf1xV6RhCZIfGfxX8w
BLVB2+S1DZDwzClfax7hZYf1KYNoQIeTyQRd67HJ+5tpxISM56HJGZh58v/9zowg
nsaDA0Sz2HngjRP0sm5MXq2aFEbUT4VpHuMwt9xT48n9TqcwVMc3euUrKHNLeHS+
JP8T/1owhD6JMegfhKiT+wLvvvmBk6l71zrWxQfUnxiYIo1yL9FFYdKDCPja+DXB
oGHFHH8DMzPZ7RJGJdajaVZjHfNTLOkgflV8utFWvQk54Pq6ChyPmbmuw801XsWD
rIsQeU1C18rwWdADlkDk1Q2yPiYpc8htW9BVJGBNVbq9+ol7BYqUzpBLHEuhm4Kp
5ZS2C1dHNiyzb7vifGlWFvbkWnP/afievVOoGkqQ3D9NYSaQH3gXbzcWbwZZB1ye
vM2PxZ2hVcC7VbmKu1u6QwVMzGCQRl+aFp0P44uhRJ3QxCSc15JeUDLF9ATcR5px
AtSEo0ibPXMK9GcmU1vAcL1WlxhbrtlVg5kEIvKJE82JM5O3LTkfs3+J+UvS+zv1
bHZmiWFPNNDTxQ0lvbU4
=Rjh4
-----END PGP SIGNATURE-----

@maflcko maflcko merged commit ee5efad into bitcoin:master Dec 19, 2017
maflcko pushed a commit that referenced this pull request Dec 19, 2017
Merge #11867: Improve node network test
18a1bba 
ee5efad [tests] refactor node_network_limited (John Newbery)
b425131 [tests] remove redundant duplicate tests from node_network_limited (John Newbery)
2e02984 [tests] node_network_limited - remove race condition (John Newbery)
dbfe294 [tests] define NODE_NETWORK_LIMITED in test framework (John Newbery)
1285312 [tests] fix flake8 warnings in node_network_limited.py (John Newbery)

Pull request description:

  Fixes race condition in the node_network_limited test case introduced in #11740. Also tidies up the test and removes redundant duplicate tests.

Tree-SHA512: a5240fe35509d81a47c3d3b141a956378675926093e658d24be43027b20d3b5f0ba7c6017c8208487a1849d4fdfb911a361911d571423db7c50711250aba3011
@jnewbery jnewbery deleted the improve_node_network_test branch December 19, 2017 23:33
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 4, 2020
@PastaPastaPasta
Merge bitcoin#11867: Improve node network test
ef5919f 
ee5efad [tests] refactor node_network_limited (John Newbery)
b425131 [tests] remove redundant duplicate tests from node_network_limited (John Newbery)
2e02984 [tests] node_network_limited - remove race condition (John Newbery)
dbfe294 [tests] define NODE_NETWORK_LIMITED in test framework (John Newbery)
1285312 [tests] fix flake8 warnings in node_network_limited.py (John Newbery)

Pull request description:

  Fixes race condition in the node_network_limited test case introduced in bitcoin#11740. Also tidies up the test and removes redundant duplicate tests.

Tree-SHA512: a5240fe35509d81a47c3d3b141a956378675926093e658d24be43027b20d3b5f0ba7c6017c8208487a1849d4fdfb911a361911d571423db7c50711250aba3011
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 5, 2020
@PastaPastaPasta
Merge bitcoin#11867: Improve node network test
5614518 
ee5efad [tests] refactor node_network_limited (John Newbery)
b425131 [tests] remove redundant duplicate tests from node_network_limited (John Newbery)
2e02984 [tests] node_network_limited - remove race condition (John Newbery)
dbfe294 [tests] define NODE_NETWORK_LIMITED in test framework (John Newbery)
1285312 [tests] fix flake8 warnings in node_network_limited.py (John Newbery)

Pull request description:

  Fixes race condition in the node_network_limited test case introduced in bitcoin#11740. Also tidies up the test and removes redundant duplicate tests.

Tree-SHA512: a5240fe35509d81a47c3d3b141a956378675926093e658d24be43027b20d3b5f0ba7c6017c8208487a1849d4fdfb911a361911d571423db7c50711250aba3011
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Jun 30, 2021
@gades
Merge bitcoin#11867: Improve node network test
bb8fc00 
ee5efad [tests] refactor node_network_limited (John Newbery)
b425131 [tests] remove redundant duplicate tests from node_network_limited (John Newbery)
2e02984 [tests] node_network_limited - remove race condition (John Newbery)
dbfe294 [tests] define NODE_NETWORK_LIMITED in test framework (John Newbery)
1285312 [tests] fix flake8 warnings in node_network_limited.py (John Newbery)

Pull request description:

  Fixes race condition in the node_network_limited test case introduced in bitcoin#11740. Also tidies up the test and removes redundant duplicate tests.

Tree-SHA512: a5240fe35509d81a47c3d3b141a956378675926093e658d24be43027b20d3b5f0ba7c6017c8208487a1849d4fdfb911a361911d571423db7c50711250aba3011
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jnewbery @jonasschnelli @laanwj @promag @maflcko