Change in vdsm[master]: tests: reduce noice after running detector test

piotr.kliczewski at gmail.com piotr.kliczewski at gmail.com
Thu Apr 9 14:31:31 UTC 2015


Piotr Kliczewski has posted comments on this change.

Change subject: tests: reduce noice after running detector test
......................................................................


Patch Set 6:

(10 comments)

https://gerrit.ovirt.org/#/c/39031/6/lib/yajsonrpc/betterAsyncore.py
File lib/yajsonrpc/betterAsyncore.py:

Line 18: # while enabling compositing instead of inheritance.
Line 19: import asyncore
Line 20: import types
Line 21: from errno import EWOULDBLOCK
Line 22: from socket import error
> How is this change related? We don't want to import stuff from modules, we 
As part of clean up when interpreter is shutdown we get:

exceptions.AttributeError'>: 'NoneType' object has no attribute 'error'

when we import error like above we do not get this.
Line 23: 
Line 24: from vdsm.infra.eventfd import EventFD
Line 25: 
Line 26: 


Line 111:                 self.handle_close()
Line 112:                 return ''
Line 113:             else:
Line 114:                 return data
Line 115:         except error, why:
> Unneeed - please remove these changes.
Please see comment above.
Line 116:             # winsock sometimes raises ENOTCONN
Line 117:             if why.args[0] == EWOULDBLOCK:
Line 118:                 return None
Line 119:             elif why.args[0] in asyncore._DISCONNECTED:


https://gerrit.ovirt.org/#/c/39031/6/tests/protocoldetectorTests.py
File tests/protocoldetectorTests.py:

Line 22: import ssl
Line 23: import threading
Line 24: import time
Line 25: from contextlib import contextmanager
Line 26: from socket import socket, error
> Do not use this import, import socket and use socket.socket() and socket.er
Please see similar comment in the other file.
Line 27: 
Line 28: from vdsm import sslutils
Line 29: from protocoldetector import MultiProtocolAcceptor
Line 30: from sslhelper import KEY_FILE, CRT_FILE


Line 41:     NAME = None
Line 42:     REQUIRED_SIZE = None
Line 43: 
Line 44:     def __init__(self):
Line 45:         self.dispatcher = None
> We don't need the dispatcher, as I explained in previous versions.
We need it to close socket properly every time.
Line 46: 
Line 47:     def detect(self, data):
Line 48:         return data.startswith(self.NAME)
Line 49: 


Line 47:     def detect(self, data):
Line 48:         return data.startswith(self.NAME)
Line 49: 
Line 50:     def handle_dispatcher(self, dispatcher, socket_address):
Line 51:         self.dispatcher = dispatcher
> We don't need the dispatcher, as I explained in previous versions.
As above
Line 52:         client_socket = dispatcher.socket
Line 53:         dispatcher.del_channel()
Line 54: 
Line 55:         def run():


Line 57:             # the socket after the dispatcher was removed.
Line 58:             time.sleep(0.05)
Line 59:             request = ""
Line 60:             while "\n" not in request:
Line 61:                 try:
> We must keep the read loop inside a try finally, to ensure that the socket 
In the original code we not always close socket. For the tests when we receive no data interpreter shutdown cleans every thing which in my opinion is wrong.
Line 62:                     request += dispatcher.recv(1024)
Line 63:                 except error:
Line 64:                     # for some test we never receive data
Line 65:                     pass


Line 61:                 try:
Line 62:                     request += dispatcher.recv(1024)
Line 63:                 except error:
Line 64:                     # for some test we never receive data
Line 65:                     pass
> You are hiding errors again
This one is only when we close socket but we still listen. It is expected so I do not want to have it on the console.
Line 66: 
Line 67:             response = self.response(request)
Line 68:             client_socket.setblocking(1)
Line 69:             client_socket.sendall(response)


Line 73:         t.start()
Line 74: 
Line 75:     def close(self):
Line 76:         if self.dispatcher is not None:
Line 77:             self.dispatcher.close()
> We don't need the dispatcher, as I explained in previous versions.
As above.
Line 78: 
Line 79: 
Line 80: class Echo(Detector):
Line 81:     """ A detector echoing sent line """


Line 102: 
Line 103:     TIMEOUT = 2.0
Line 104:     GRACETIME = 0.5
Line 105:     CONCURRENCY = 5
Line 106:     PERMUTATIONS = ((False, False), (True, False), (True, True))
> We don't need this change, and it is not related to the "noise"
Sure. We can have it in other patch.
Line 107:     SSLCTX = sslutils.SSLContext(CRT_FILE, KEY_FILE, ca_cert=CRT_FILE)
Line 108:     BUFSIZE = 512
Line 109: 
Line 110:     def setUp(self):


Line 232:                     s = self.SSLCTX.wrapSocket(s)
Line 233:                 else:
Line 234:                     s = ssl.wrap_socket(s, KEY_FILE, CRT_FILE,
Line 235:                                         ca_certs=CRT_FILE,
Line 236:                                         server_side=False)
> We don't need this, we don't test SSLCTX here, this is the client side.
I am confused by:

"Nir Soffer
Apr 4 1:30 AM
I think that the reason I did not use SSLCTX it when I added these test is I was trying to simulate real connections, where client and server are *not* sharing the same ssl context."

If we see a value in using ssl module why no to check with m2c?
Line 237:             s.connect(self.acceptor_address)
Line 238:             yield s
Line 239:         finally:
Line 240:             s.close()


-- 
To view, visit https://gerrit.ovirt.org/39031
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I23302c7f742f31f0ab5e12265da3bb7f57a81d80
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Dima Kuznetsov <dkuznets at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list