[Test] Tests for zmqpubrawtx and zmqpubrawblock by achow101 · Pull Request #10552 · bitcoin/bitcoin

achow101 · 2017-06-08T01:02:57Z

No description provided.

fanquake · 2017-06-08T07:21:45Z

Both linux builds timed out after ~48 minutes.

jnewbery · 2017-06-08T14:30:37Z

I suspect that this timeout is due to the python3-zmq Context not closing cleanly at the end of the zmq_test.py test. See https://stackoverflow.com/questions/17140417/termination-of-python-script-while-using-zeromq-with-dead-server for example.

I'm working on a branch that fixes that behaviour here: https://github.com/jnewbery/bitcoin/tree/zmqtestimprovements . It's currently building in Travis. If that works, you might want to try rebasing this PR on top of that branch to see if it resolves this for you.

jnewbery · 2017-06-08T15:05:35Z

Opened #10555 to fix the python zmq issue. @achow101 - if rebasing on that resolves your travis issue, could you review/ACK that PR?

achow101 · 2017-06-08T18:51:57Z

Rebased on top of #10555

maflcko · 2017-06-18T12:26:03Z

Reopen. Was closed due to false positive regex match by GitHub.

achow101 · 2017-06-18T17:26:57Z

Rebased after #10555 merge.

achow101 · 2017-08-15T21:55:51Z

rebased

jnewbery

An unfortunate aspect of this test is that it assumes an order for ZMQ notifications (tx > rawtx > hashblock > rawblock). I don't think that's necessary or should be asserted in the test. However, you've just extended what's already here, so concept ACK.

Please run a linter over this and fix up the warnings. There are a bunch of nits (unused imports, trailing whitespace, etc).

A few more nits inline.

jnewbery · 2017-08-23T20:13:13Z

test/functional/test_framework/util.py

nit: why not placed alphabetically like the other imports?

jnewbery · 2017-08-23T20:13:42Z

test/functional/test_framework/util.py

Please place this in the 'Utility Functions' section, rather than 'Node Functions'

jnewbery · 2017-08-23T20:14:33Z

test/functional/zmq_test.py

why not just assign txhash on line 69. There's no reason to have body in this section.

jnewbery · 2017-08-23T20:15:31Z

test/functional/zmq_test.py

No need for the duplicated comment

jnewbery · 2017-08-23T20:16:03Z

test/functional/zmq_test.py

No need for this line

jnewbery · 2017-08-23T20:16:32Z

test/functional/zmq_test.py

I'd move this assert to immediately after the topic = ... line (like in the previous tests in this script).

achow101 · 2017-08-23T23:50:59Z

Nits addresses (I think) and rebased (there were merge conflicts).

jnewbery · 2017-09-06T22:57:58Z

Tested ACK ba0b4f1c0a2c47f2f8c342686c94c6e358ea4efe, but needs rebase.

achow101 · 2017-09-06T23:07:39Z

Rebased

jnewbery · 2017-09-06T23:53:13Z

Tested ACK a3696dcec45f11f202c1666084c4aa1f8fabd37f

promag

In the future we could (partially) change to one socket per notification. As it is, the order matters and requires checking for topic when iterating.

promag · 2017-09-06T23:54:45Z

test/functional/test_framework/util.py

Nit, rename to hash256 to resemble CHash256?

promag · 2017-09-06T23:57:42Z

test/functional/zmq_test.py

Ultra nit, sort arguments.

promag · 2017-09-07T00:14:02Z

Just saw @jnewbery comment above:

An unfortunate aspect of this test is that it assumes an order for ZMQ notifications (tx > rawtx > hashblock > rawblock).

Can be fixed with one socket per notification.

jnewbery · 2017-09-18T15:51:19Z

Can be fixed with one socket per notification.

@promag - sounds like a very sensible change. This PR simply extends what's already there so I think we should merge this as is. I'd be happy to review a follow-up PR if you want to implement that.

promag · 2017-09-18T16:06:01Z

@jnewbery agree. As for the follow up, I think it could be added somewhere in TestFramework something to abstract the sockets creation, to ease testing subscribers. As it is, it's too much verbose for testing purpose.

promag

utACK.

promag · 2017-09-18T17:52:14Z

test/functional/test_framework/util.py

Move before hex_str_to_bytes.

jnewbery · 2017-09-19T13:34:18Z

@promag

As for the follow up, I think it could be added somewhere in TestFramework something to abstract the sockets creation

If you're thinking of abstracting away the ZMQ interface, I think it would be better to add a new class in test_node.py to be owned by the TestNode class, like how I added a TestNodeCLI class in #10798

However, the ZMQ interface is only used in this one test script, and I think it's best to keep it that way, so I think it's best to leave the implementation in the zmq_test.py file.

We've wondered off-topic a bit for this PR. If you're interested in pursuing this further, feel free to ping me on IRC or open a separate issue.

achow101 · 2017-09-28T16:15:18Z

Can this be merged?

d3677ab Tests for zmqpubrawtx and zmqpubrawblock (Andrew Chow) Pull request description: Tree-SHA512: 9e367fd8936514bfb567ef3f3d83770d374287354b59c9187e844056dd086e8aa2de32ce55d35486cecd706e7c93cd1c1e2709ee82d3dddb805827be8d2bcb14

Github-Pull: bitcoin#10552 Rebased-From: d3677ab

d3677ab Tests for zmqpubrawtx and zmqpubrawblock (Andrew Chow) Pull request description: Tree-SHA512: 9e367fd8936514bfb567ef3f3d83770d374287354b59c9187e844056dd086e8aa2de32ce55d35486cecd706e7c93cd1c1e2709ee82d3dddb805827be8d2bcb14

fanquake added the Tests label Jun 8, 2017

jnewbery mentioned this pull request Jun 8, 2017

[tests] various improvements to zmq_test.py #10555

Merged

achow101 force-pushed the zmq-raw-tests branch from 94c7794 to bb73593 Compare June 8, 2017 18:51

maflcko closed this in #10555 Jun 18, 2017

maflcko reopened this Jun 18, 2017

achow101 force-pushed the zmq-raw-tests branch 4 times, most recently from 87f833e to b734d91 Compare June 18, 2017 17:26

achow101 force-pushed the zmq-raw-tests branch from b734d91 to 5184e47 Compare June 19, 2017 18:20

achow101 force-pushed the zmq-raw-tests branch from 5184e47 to eeac6e6 Compare August 15, 2017 21:55

jnewbery suggested changes Aug 23, 2017

View reviewed changes

achow101 force-pushed the zmq-raw-tests branch 2 times, most recently from 865e07d to ba0b4f1 Compare August 23, 2017 23:50

achow101 force-pushed the zmq-raw-tests branch from ba0b4f1 to a3696dc Compare September 6, 2017 23:07

promag reviewed Sep 7, 2017

View reviewed changes

achow101 force-pushed the zmq-raw-tests branch from a3696dc to d30a9ab Compare September 18, 2017 15:44

promag reviewed Sep 18, 2017

View reviewed changes

Tests for zmqpubrawtx and zmqpubrawblock

d3677ab

achow101 force-pushed the zmq-raw-tests branch from d30a9ab to d3677ab Compare September 19, 2017 02:08

maflcko merged commit d3677ab into bitcoin:master Sep 29, 2017

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Oct 3, 2017

Tests for zmqpubrawtx and zmqpubrawblock

e4605d9

Github-Pull: bitcoin#10552 Rebased-From: d3677ab

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

achow101 commented Jun 8, 2017

Uh oh!

fanquake commented Jun 8, 2017

Uh oh!

jnewbery commented Jun 8, 2017

Uh oh!

jnewbery commented Jun 8, 2017

Uh oh!

achow101 commented Jun 8, 2017

Uh oh!

maflcko commented Jun 18, 2017

Uh oh!

achow101 commented Jun 18, 2017

Uh oh!

achow101 commented Aug 15, 2017

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Aug 23, 2017

Uh oh!

jnewbery commented Sep 6, 2017

Uh oh!

achow101 commented Sep 6, 2017

Uh oh!

jnewbery commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Sep 18, 2017

Uh oh!

promag commented Sep 18, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Sep 19, 2017

Uh oh!

achow101 commented Sep 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jnewbery commented Sep 6, 2017 •

edited

Loading

promag left a comment •

edited

Loading

promag commented Sep 7, 2017 •

edited

Loading