Skip to content

[Test] Tests for zmqpubrawtx and zmqpubrawblock #10552

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 1 commit into from
Sep 29, 2017
Merged

[Test] Tests for zmqpubrawtx and zmqpubrawblock #10552

maflcko merged 1 commit into from
Sep 29, 2017

Conversation

@achow101
Copy link
Member

@achow101 achow101 commented Jun 8, 2017

No description provided.

@fanquake fanquake added the Tests label Jun 8, 2017
@fanquake
Copy link
Member

fanquake commented Jun 8, 2017

Both linux builds timed out after ~48 minutes.

@jnewbery
Copy link
Contributor

jnewbery commented Jun 8, 2017

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 jnewbery mentioned this pull request Jun 8, 2017
Merged
@jnewbery
Copy link
Contributor

jnewbery commented Jun 8, 2017

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

@achow101
Copy link
Member Author

achow101 commented Jun 8, 2017

Rebased on top of #10555

@maflcko
Copy link
Member

maflcko commented Jun 18, 2017

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

@maflcko maflcko reopened this Jun 18, 2017
@achow101 achow101 force-pushed the zmq-raw-tests branch 4 times, most recently from 87f833e to b734d91 Compare June 18, 2017 17:26
@achow101
Copy link
Member Author

achow101 commented Jun 18, 2017

Rebased after #10555 merge.

@achow101
Copy link
Member Author

achow101 commented Aug 15, 2017

rebased

jnewbery
jnewbery suggested changes Aug 23, 2017
View reviewed changes
Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

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

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.

test/functional/test_framework/util.py Outdated
Copy link
Contributor

@jnewbery jnewbery Aug 23, 2017

Choose a reason for hiding this comment

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

nit: why not placed alphabetically like the other imports?

test/functional/test_framework/util.py Outdated
Copy link
Contributor

@jnewbery jnewbery Aug 23, 2017

Choose a reason for hiding this comment

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

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

test/functional/zmq_test.py Outdated
Copy link
Contributor

@jnewbery jnewbery Aug 23, 2017

Choose a reason for hiding this comment

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

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

test/functional/zmq_test.py Outdated
Copy link
Contributor

@jnewbery jnewbery Aug 23, 2017

Choose a reason for hiding this comment

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

No need for the duplicated comment

test/functional/zmq_test.py Outdated
Copy link
Contributor

@jnewbery jnewbery Aug 23, 2017

Choose a reason for hiding this comment

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

No need for this line

test/functional/zmq_test.py Outdated
Copy link
Contributor

@jnewbery jnewbery Aug 23, 2017

Choose a reason for hiding this comment

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

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

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

achow101 commented Aug 23, 2017

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

@jnewbery
Copy link
Contributor

jnewbery commented Sep 6, 2017

Tested ACK ba0b4f1c0a2c47f2f8c342686c94c6e358ea4efe, but needs rebase.

@achow101
Copy link
Member Author

achow101 commented Sep 6, 2017

Rebased

@jnewbery
Copy link
Contributor

jnewbery commented Sep 6, 2017 •
edited
Loading

Tested ACK a3696dcec45f11f202c1666084c4aa1f8fabd37f

promag
promag reviewed Sep 7, 2017
View reviewed changes
Copy link
Contributor

@promag promag 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.

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.

test/functional/test_framework/util.py Outdated
Copy link
Contributor

@promag promag Sep 6, 2017

Choose a reason for hiding this comment

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

Nit, rename to hash256 to resemble CHash256?

Copy link
Member Author

@achow101 achow101 Sep 18, 2017

Choose a reason for hiding this comment

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

Done

test/functional/zmq_test.py Outdated
Copy link
Contributor

@promag promag Sep 6, 2017

Choose a reason for hiding this comment

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

Ultra nit, sort arguments.

Copy link
Member Author

@achow101 achow101 Sep 18, 2017

Choose a reason for hiding this comment

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

Done

@promag
Copy link
Contributor

promag commented Sep 7, 2017 •
edited
Loading

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
Copy link
Contributor

jnewbery commented Sep 18, 2017

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
Copy link
Contributor

promag commented Sep 18, 2017

@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
promag reviewed Sep 18, 2017
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

utACK.

test/functional/test_framework/util.py Outdated
Copy link
Contributor

@promag promag Sep 18, 2017

Choose a reason for hiding this comment

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

Move before hex_str_to_bytes.

Copy link
Member Author

@achow101 achow101 Sep 19, 2017

Choose a reason for hiding this comment

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

Done

@jnewbery
Copy link
Contributor

jnewbery commented Sep 19, 2017

@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
Copy link
Member Author

achow101 commented Sep 28, 2017

Can this be merged?

@maflcko maflcko merged commit d3677ab into bitcoin:master Sep 29, 2017
maflcko pushed a commit that referenced this pull request Sep 29, 2017
Merge #10552: [Test] Tests for zmqpubrawtx and zmqpubrawblock
9c3c9cd 
d3677ab Tests for zmqpubrawtx and zmqpubrawblock (Andrew Chow)

Pull request description:

Tree-SHA512: 9e367fd8936514bfb567ef3f3d83770d374287354b59c9187e844056dd086e8aa2de32ce55d35486cecd706e7c93cd1c1e2709ee82d3dddb805827be8d2bcb14
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Oct 3, 2017
@achow101
Tests for zmqpubrawtx and zmqpubrawblock
e4605d9 
Github-Pull: bitcoin#10552
Rebased-From: d3677ab
codablock pushed a commit to codablock/dash that referenced this pull request Sep 24, 2019
@codablock
Merge bitcoin#10552: [Test] Tests for zmqpubrawtx and zmqpubrawblock
7ddd169 
d3677ab Tests for zmqpubrawtx and zmqpubrawblock (Andrew Chow)

Pull request description:

Tree-SHA512: 9e367fd8936514bfb567ef3f3d83770d374287354b59c9187e844056dd086e8aa2de32ce55d35486cecd706e7c93cd1c1e2709ee82d3dddb805827be8d2bcb14
@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

@promag promag promag left review comments

@jnewbery jnewbery jnewbery requested 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.

5 participants

@achow101 @fanquake @jnewbery @maflcko @promag