Skip to content

[tests] various improvements to zmq_test.py #10555

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
maflcko merged 5 commits into from
Jun 18, 2017
Merged

[tests] various improvements to zmq_test.py #10555

maflcko merged 5 commits into from
Jun 18, 2017

Conversation

@jnewbery
Copy link
Contributor

@jnewbery jnewbery commented Jun 8, 2017

This PR contains various improvements to zmq_test.py, including stopping it from hanging and causing travis builds to time out. Individual commits are:

  • update zmq test to use correct config.ini file - follows Share config between util and functional tests #10331, which moved the config.ini file from /bitcoin/test/functional to /bitcoin/test. zmq_test.py was not updated to find the config file in the correct location. This isn't a problem when running through test_runner.py, but would prevent zmq_test.py from being run directly.
  • tidy up zmq_test.py - general code tidy-up. Cuts run time from ~12s to ~6s
  • in zmq test, timeout if message not received - stops a race condition where we could try to receive a message on the zmq socket before it's been sent
  • destroy zmq context in zmq_tests.py - fixes an issue where the zmq contest would not be close properly at the end of the test, causing sys.exit() to fail. This would cause travis builds to hang. See https://stackoverflow.com/questions/17140417/termination-of-python-script-while-using-zeromq-with-dead-server for more details.
  • timeout integration tests on travis after 20 minutes - causes travis builds to timeout individual tests if they run for more than 20 minutes. That means we'll get logs from hung tests, rather than travis itself timing out after ~50 minutes (in which case we don't get any useful diagnostics).

I need this for the next step of #10082, since this test was causing the next PR to hang in travis.

I believe this will also fix #10552 once that is rebased on top of this.

This was referenced Jun 8, 2017
Merged
Merged
@jnewbery jnewbery force-pushed the zmqtestimprovements branch from a4ad588 to a0af693 Compare June 8, 2017 17:59
@achow101
Copy link
Member

achow101 commented Jun 8, 2017 •
edited
Loading

utACK
Tested ACK c4f6b7f

@jnewbery jnewbery force-pushed the zmqtestimprovements branch from a0af693 to c4f6b7f Compare June 8, 2017 19:22
jimmysong
jimmysong approved these changes Jun 8, 2017
View reviewed changes
Copy link
Contributor

@jimmysong jimmysong left a comment •
edited
Loading

Choose a reason for hiding this comment

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

tested ACK c4f6b7f

@jnewbery
Copy link
Contributor Author

jnewbery commented Jun 8, 2017

Oops. Accidentally squashed two commits together in my last fixup. I've divided this PR back into the correct number of commits.

https://github.com/jnewbery/bitcoin/tree/pr10555.1 == https://github.com/jnewbery/bitcoin/tree/pr10555.2

@fanquake fanquake added the Tests label Jun 8, 2017
somdoron
somdoron reviewed Jun 10, 2017
View reviewed changes
test/functional/zmq_test.py Outdated
assert_equal(hashRPC, hashZMQ) # txid from sendtoaddress must be equal to the hash received over zmq

# Destroy the zmq context
self.zmqContext.destroy(linger=None)
Copy link

@somdoron somdoron Jun 10, 2017

Choose a reason for hiding this comment

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

when test failed this won't be called and the process will hang, might be good to call this in finally block

Copy link
Contributor Author

@jnewbery jnewbery Jun 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Good point @somdoron. Added a fixup commit 72a54cce9ebc3ee1efd69c5d16b11ca54c25783a. Can you take a look and let me know what you think. If you're happy I'll squash with the previous commit.

Copy link

@somdoron somdoron Jun 12, 2017

Choose a reason for hiding this comment

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

looks good

@somdoron somdoron mentioned this pull request Jun 10, 2017
Closed
@jnewbery jnewbery force-pushed the zmqtestimprovements branch from c4f6b7f to a2106d8 Compare June 11, 2017 22:45
somdoron
somdoron reviewed Jun 12, 2017
View reviewed changes
test/functional/zmq_test.py Outdated
Copy link

@somdoron somdoron Jun 12, 2017

Choose a reason for hiding this comment

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

You can also call socket.SET(zmq.RCVTIMEO, 60000) when creating the socket instead of the NOBLOCK and the loop. It will raise zmq.error.Again when time is over

Copy link
Contributor Author

@jnewbery jnewbery Jun 12, 2017

Choose a reason for hiding this comment

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

Thanks! I didn't know about zmq.RCVTIMEO. Updated to use that instead of the timeout loop in b1bac1c

@jnewbery jnewbery force-pushed the zmqtestimprovements branch from a2106d8 to d8a3616 Compare June 12, 2017 13:15
@jnewbery jnewbery force-pushed the zmqtestimprovements branch from d8a3616 to 0a4912e Compare June 12, 2017 13:17
@jnewbery jnewbery mentioned this pull request Jun 14, 2017
Closed
@maflcko
Copy link
Member

maflcko commented Jun 18, 2017

Concept ACK 0a4912e

@maflcko maflcko merged commit 0a4912e into bitcoin:master Jun 18, 2017
maflcko pushed a commit that referenced this pull request Jun 18, 2017
Merge #10555: [tests] various improvements to zmq_test.py
643fa0b 
0a4912e [tests] timeout integration tests on travis after 20 minutes (John Newbery)
7c51e9f [tests] destroy zmq context in zmq_tests.py (John Newbery)
b1bac1c [tests] in zmq test, timeout if message not received (John Newbery)
5ebd5f9 [tests] tidy up zmq_test.py (John Newbery)
4a0c08f [tests] update zmq test to use correct config.ini file (John Newbery)

Tree-SHA512: 5e607af2f2dc5c73fba4b2d3890097580a7525f6a4996c7c78f01822e45e0054fd0a225ea53fce6308804e560cced6de6cf5d16137469dcf100b2e9643e08d03
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 9, 2019
@PastaPastaPasta
Merge bitcoin#10555: [tests] various improvements to zmq_test.py
aa364bf 
0a4912e [tests] timeout integration tests on travis after 20 minutes (John Newbery)
7c51e9f [tests] destroy zmq context in zmq_tests.py (John Newbery)
b1bac1c [tests] in zmq test, timeout if message not received (John Newbery)
5ebd5f9 [tests] tidy up zmq_test.py (John Newbery)
4a0c08f [tests] update zmq test to use correct config.ini file (John Newbery)

Tree-SHA512: 5e607af2f2dc5c73fba4b2d3890097580a7525f6a4996c7c78f01822e45e0054fd0a225ea53fce6308804e560cced6de6cf5d16137469dcf100b2e9643e08d03
@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

2 more reviewers

@jimmysong jimmysong jimmysong approved these changes

@somdoron somdoron somdoron approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jnewbery @achow101 @maflcko @jimmysong @somdoron @fanquake