Improve ZMQ functional test #11452

promag commented at 10:37 PM on October 4, 2017: member

After #11439, this PR only improves:

test comments;
simplicity by removing duplicate tests;
also removes duplicate code.

promag commented at 10:38 PM on October 4, 2017: member

Ping @jnewbery, @ryanofsky and @TheBlueMatt.

fanquake added the label Tests on Oct 4, 2017

in test/functional/zmq_test.py:74 in db3b646bac outdated

  80 | +
  81 | +        # Subscribe to all available topics.
  82 | +        self.subscribe("hashblock")
  83 | +        self.subscribe("hashtx")
  84 | +        self.subscribe("rawblock")
  85 | +        self.subscribe("rawtx")

esotericnonsense commented at 3:48 AM on October 5, 2017:

I found 'subscribe' a bit misleading here, though it could be that I'm thinking too much. It's not merely a standard ZeroMQ subscription; the ZMQSubscriber class created tracks sequence. As it's written, my immediate thought was 'why aren't we just doing self.subscribe("")' (pre this commit, the four setsockopt calls could have been replaced with just one). Perhaps 'create_subscriber' or fluffing up the comment a bit.

promag commented at 9:23 AM on October 5, 2017:

I can rename.

in test/functional/zmq_test.py:36 in db3b646bac outdated

  31 | +        self.socket.setsockopt(zmq.SUBSCRIBE, type.encode('latin-1'))
  32 | +        self.subscribers[type] = ZMQSubscriber(self.socket, self.address)
  33 | +
  34 | +    def receive(self, type):
  35 | +        subscriber = self.subscribers[type]
  36 | +        topic, body, seq = subscriber.socket.recv_multipart()

esotericnonsense commented at 3:51 AM on October 5, 2017:

The subscribers don't actually have independent sockets, perhaps just use self.socket?

promag commented at 9:24 AM on October 5, 2017:

I would like to leave it this way so in the future we can have more sockets.

in test/functional/zmq_test.py:34 in db3b646bac outdated

  29 | +    def subscribe(self, type):
  30 | +        import zmq
  31 | +        self.socket.setsockopt(zmq.SUBSCRIBE, type.encode('latin-1'))
  32 | +        self.subscribers[type] = ZMQSubscriber(self.socket, self.address)
  33 | +
  34 | +    def receive(self, type):

esotericnonsense commented at 3:58 AM on October 5, 2017:

nit: assert_receive? My initial thought was that you're selectively receiving a type (which makes sense if different sockets, but if the sockets are all identical, you get what you're given, as this code tests)

promag commented at 9:29 AM on October 5, 2017:

Below feels better to read foo = self.receive(...), no need for the assert_ IMO.

but if the sockets are all identical, you get what you're given

Not really, if below you swap 2 consecutive receives the test will fail. What comes from the socket has a specific order and we must follow it when reading.

in test/functional/zmq_test.py:48 in db3b646bac outdated

  47 | @@ -24,7 +48,7 @@ def setup_nodes(self):
  48 |          except ImportError:
  49 |              raise SkipTest("python3-zmq module not available.")
  50 |  
  51 | -        # Check that bitcoin has been built with ZMQ enabled
  52 | +        # Check that bitcoin has been built with ZMQ enabled.
  53 |          config = configparser.ConfigParser()
  54 |          if not self.options.configfile:
  55 |              self.options.configfile = os.path.dirname(__file__) + "/../config.ini"

esotericnonsense commented at 4:03 AM on October 5, 2017:

Likely a pre-existing issue, on my box this prevents python zmq_test.py running in isolation (works through test_runner because the option already exists). If you wrap it e.g. os.path.abspath(os.path.dirname(__file__)) (as test_runner.py does) it works.

promag commented at 9:22 AM on October 5, 2017:

It works for me 🙄

promag commented at 9:29 AM on October 5, 2017:

I'll check with your suggestion.

MarcoFalke commented at 11:07 AM on October 5, 2017:

Also should use os.path.join()

promag commented at 5:12 PM on October 5, 2017:

Ok.

esotericnonsense commented at 4:05 AM on October 5, 2017: contributor

Tested ACK db3b646bac37a54cb09f121b048fcfbb6d522808, few nits above.

promag commented at 9:29 AM on October 5, 2017: member

Thanks for the review @esotericnonsense.

in test/functional/zmq_test.py:27 in db3b646bac outdated

  22 |  class ZMQTest (BitcoinTestFramework):
  23 |      def set_test_params(self):
  24 | +        self.address = "tcp://127.0.0.1:28332"
  25 | +        self.num_blocks = 5
  26 |          self.num_nodes = 2
  27 | +        self.subscribers = {}

jnewbery commented at 2:17 PM on October 5, 2017:

I don't like adding these assignments to set_test_params.

num_blocks and subscribers can be local variables in run_test()
address should be a local variable in setup_nodes() and passed into the ZMQSubscriber constructor. Conceptually, address is the address of the ZMQ subscriber, not the test framework.

set_test_params() should be used for overriding the number of nodes, network topology, etc.

promag commented at 10:16 PM on October 6, 2017:

Ok.

in test/functional/zmq_test.py:61 in db3b646bac outdated

  67 | -        ip_address = "tcp://127.0.0.1:28332"
  68 | -        self.zmqSubSocket.connect(ip_address)
  69 | -        self.extra_args = [['-zmqpubhashblock=%s' % ip_address, '-zmqpubhashtx=%s' % ip_address,
  70 | -                       '-zmqpubrawblock=%s' % ip_address, '-zmqpubrawtx=%s' % ip_address], []]
  71 | +        # Initialize ZMQ context and socket.
  72 | +        # At the moment all messages are received in the same socket which means

jnewbery commented at 2:20 PM on October 5, 2017:

Nit: remove At the moment

promag commented at 12:03 AM on October 10, 2017:

Ok.

in test/functional/zmq_test.py:29 in db3b646bac outdated

  24 | +        self.address = "tcp://127.0.0.1:28332"
  25 | +        self.num_blocks = 5
  26 |          self.num_nodes = 2
  27 | +        self.subscribers = {}
  28 | +
  29 | +    def subscribe(self, type):

jnewbery commented at 2:23 PM on October 5, 2017:

As in my comment here #11439 (review), I think it makes sense for the subscribe and receive logic to be owned by ZMQSubscriber

promag commented at 12:02 AM on October 10, 2017:

Ok.

jnewbery commented at 2:27 PM on October 5, 2017: member

Concept ACK. A few nits inline

promag force-pushed on Oct 10, 2017

promag commented at 12:04 AM on October 10, 2017: member

Rebased and added a commit to address comments.Will squash after reviews.

promag force-pushed on Oct 10, 2017

in test/functional/zmq_test.py:17 in 480c4526b8 outdated

  12 | @@ -13,6 +13,25 @@
  13 |                                   hash256,
  14 |                                  )
  15 |  
  16 | +class ZMQSubscriber:
  17 | +    def __init__(self, socket, type):

jnewbery commented at 3:59 PM on October 10, 2017:

type is a builtin. Perhaps notification _type is better?

jnewbery commented at 4:04 PM on October 10, 2017: member

One small nit, but otherwise looks great.

Tested ACK 480c4526b845f69072306a047a4718d769403cdd. Please squash commits before merge.

Improve ZMQ functional test cc9ee809ad

promag force-pushed on Oct 10, 2017

promag commented at 10:50 PM on October 10, 2017: member

@jnewbery Thanks, renamed to topic. Now squashed.

in test/functional/zmq_test.py:57 in cc9ee809ad

  65 | -        self.zmqSubSocket.connect(ip_address)
  66 | -        self.extra_args = [['-zmqpubhashblock=%s' % ip_address, '-zmqpubhashtx=%s' % ip_address,
  67 | -                       '-zmqpubrawblock=%s' % ip_address, '-zmqpubrawtx=%s' % ip_address], []]
  68 | +        # Initialize ZMQ context and socket.
  69 | +        # All messages are received in the same socket which means
  70 | +        # that this test fails if the publishing order changes.

ryanofsky commented at 6:25 PM on October 11, 2017:

The ZMQSubscriber class makes it harder for the test to work if topics are sent in a different order.

Can you take a look at and consider squashing 3668dcdd5a7a12dea0ce5fd9a1d7d17ade0f7b75, which builds on your change, but gets rid of ZMQSubscriber, shortens the test, and lets it pass regardless of ordering of messages from different topics?

promag commented at 8:16 AM on October 12, 2017:

@ryanofsky Thanks for the time spent on this. For the moment we want this test to fail if the order changes, see #11439 (comment).

promag commented at 8:17 AM on October 12, 2017:

BTW, if the order changes it's only a matter of changing the order of the sub.receive() calls.

jnewbery commented at 12:29 PM on October 12, 2017:

Thanks for the input @ryanofsky . I personally think the ZMQSubscriber class is clearer, but it was me who pushed @promag in that direction initially, so we should see what other reviewers think.

ryanofsky commented at 4:45 PM on October 12, 2017:


EDIT: Actually, this isn't true. I was thinking of the other unnecessarily fragile version of the test that used multiple sockets :)

promag commented at 10:41 PM on October 13, 2017:

Ah right, that one hangs :+1:

ryanofsky commented at 4:58 PM on October 12, 2017: member

ACK cc9ee809ad19a63ca284d2fbc327ac1cbcee31e4. This is an improvement over the current test, though I think it should be simplified more and made less fragile by getting rid of the ZMQSubscriber class.

laanwj merged this on Oct 18, 2017

laanwj closed this on Oct 18, 2017

laanwj referenced this in commit 02ac8c892b on Oct 18, 2017

PastaPastaPasta referenced this in commit fa9ecedb3a on Dec 22, 2019

PastaPastaPasta referenced this in commit 2a633bdf83 on Jan 2, 2020

PastaPastaPasta referenced this in commit d9a22ee1ff on Jan 4, 2020

PastaPastaPasta referenced this in commit 1c0ce972af on Jan 12, 2020

PastaPastaPasta referenced this in commit 7bd96317f5 on Jan 12, 2020

PastaPastaPasta referenced this in commit 97ef3db0bf on Jan 12, 2020

PastaPastaPasta referenced this in commit 245cbbbc21 on Jan 12, 2020

PastaPastaPasta referenced this in commit 258bf99f73 on Jan 12, 2020

PastaPastaPasta referenced this in commit 8ce540d588 on Jan 12, 2020

PastaPastaPasta referenced this in commit 81ebfb7f76 on Jan 16, 2020

ckti referenced this in commit 1270e2888b on Mar 28, 2021

DrahtBot locked this on Sep 8, 2021