[GitHub] [qpid-dispatch] kgiusti commented on a change in pull request #1441: DISPATCH-2258: Cleaned up log messages, created senders after receive…

[GitBox <gi...@apache.org>]

kgiusti commented on a change in pull request #1441:
URL: https://github.com/apache/qpid-dispatch/pull/1441#discussion_r751355815



##########
File path: tests/system_tests_distribution.py
##########
@@ -3884,7 +3882,7 @@ def __init__(self,
         n_links_per_message = 4
         self.n_expected_transitions = len(self.senders) * self.messages_per_sender * n_links_per_message
 
-        self.debug = False
+        self.debug = debug
 
         self.test_name = test_name
 

Review comment:
       Since github won't let me comment on lines that haven't been changed: this is regards to the debug_print() function: add a flush=True to the print statement:  print(message, flush=True).   unittest has a tendency to not flush stdout if a failure is hit.

##########
File path: tests/system_tests_distribution.py
##########
@@ -3826,6 +3823,7 @@ def __init__(self,
         self.error                = None
         self.messages_per_sender  = 100

Review comment:
       Non-patch observation: is 100 messages *really* necessary?  CI systems tend to be painfully slow.  Can the same degree of confidence that this works be achieved by say 20 messages?  

##########
File path: tests/system_tests_distribution.py
##########
@@ -3910,11 +3908,9 @@ def send_from_client(self, sender, n_messages, sender_index):
             n_sent             += 1
             self.n_sent        += 1
             self.n_transitions += 1
-            self.debug_print("send_from_client -- sender: %d n_sent: %d" % (sender_index, n_sent))
+        self.debug_print("send_from_client -- sender: %d n_sent: %d" % (sender_index, n_sent))

Review comment:
       Did you also want to do the same thing (move the debug print out of the loop) for the send_from_waypoint() method below?  Just asking...

##########
File path: tests/system_tests_distribution.py
##########
@@ -3972,19 +3958,25 @@ def on_link_opening(self, event):
                 if self.n_waypoint_receivers < 2 :
                     self.waypoints[self.n_waypoint_receivers]['receiver'] = event.receiver
                     self.n_waypoint_receivers += 1
+                # Create the senders after the waypoint receiver links have been opened.
+                if self.n_waypoint_receivers == 2:
+                    for i in range(len(self.sender_connections)):
+                        cnx = self.sender_connections[i]
+                        sender = self.senders[i]
+                        sender['sender'] = event.container.create_sender(cnx,
+                                                                         self.destination,
+                                                                         name=link_name())
+                        sender['to_send'] = self.messages_per_sender
+                        sender['n_sent'] = 0
 
     def on_sendable(self, event):
-        self.debug_print("on_sendable ------------------------------")
         for index in range(len(self.senders)) :
             sender = self.senders[index]
             if event.sender == sender['sender'] :
-                self.debug_print("    client sender %d" % index)
                 if sender['n_sent'] < sender['to_send'] :
                     self.debug_print("    sending %d" % sender['to_send'])
                     self.send_from_client(sender['sender'], sender['to_send'], index)
                     sender['n_sent'] = sender['to_send']  # n_sent = n_to_send

Review comment:
       wait a sec... can't send_from_client exit with < to_send if credit runs out?   This seems wrong.  Maybe send_from_client should be incrementing sender['n_sent'] rather than having this line assume sender['to_send'] was actually sent.
   
   And don't get me started on why the heck this test is using maps (sender['n_sent']) rather than a class (sender.n_sent) all over this test.  It's like this test was purposely written to be easy to break and maximize reader confusion.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org