Change in vdsm[master]: tests: Cleanup apiTests exception handling

Adam Litke has uploaded a new change for review.

Change subject: tests: Cleanup apiTests exception handling
......................................................................

tests: Cleanup apiTests exception handling

As Dan reported in http://gerrit.ovirt.org/#/c/9442/ the behavior of
SocketServer differs between versions of python which causes the exception
raised by sendMessage() to change.  Rather than key the expected exception
based on the Python version, clean up the flow so it will behave the same across
Python versions.

Change-Id: I818e5e7b8dd0a1abad94deb65e32c1d76225b839
Signed-off-by: Adam Litke 
---
M tests/apiTests.py
1 file changed, 22 insertions(+), 7 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/99/9499/1

diff --git a/tests/apiTests.py b/tests/apiTests.py
index ff0a785..3e4be6a 100644
--- a/tests/apiTests.py
+++ b/tests/apiTests.py
@@ -176,6 +176,10 @@
         _fakeret = {}

+class ProtocolError(Exception):
+    pass
+
+
 class JsonRawTest(APITest):
     _Size = struct.Struct("!Q")

@@ -191,12 +195,23 @@
         try:
             sock.connect((ip, port))
             sock.sendall(msg)
+        except socket.error, e:
+            raise ProtocolError("Unable to send request: %s", e)
+        try:
             data = sock.recv(JsonRawTest._Size.size)
-            msgLen = JsonRawTest._Size.unpack(data)[0]
+        except socket.error, e:
+            raise ProtocolError("Unable to read response length: %s", e)
+        if not data:
+            raise ProtocolError("No data received")
+        msgLen = JsonRawTest._Size.unpack(data)[0]
+        try:
             data = sock.recv(msgLen)
-            return json.loads(data)
-        finally:
-            sock.close()
+        except socket.error, e:
+            raise ProtocolError("Unable to read response body: %s", e)
+        if len(data) != msgLen:
+            raise ProtocolError("Response body length mismatch")
+        return json.loads(data)
+        sock.close()

def testPing(self):
         self.clearAPI()
@@ -227,14 +242,14 @@
         self.assertEquals(4, reply['error']['code'])

def testMissingSize(self):
-        self.assertRaises(struct.error, self.sendMessage,
+        self.assertRaises(ProtocolError, self.sendMessage,
                           "malformed message")

def testClientNotJson(self):
         msg = "malformed message"
         msize = JsonRawTest._Size.pack(len(msg))
         msg = msize + msg
-        self.assertRaises(struct.error, self.sendMessage, msg)
+        self.assertRaises(ProtocolError, self.sendMessage, msg)

def testSynchronization(self):
         def doPing(msg):
@@ -245,7 +260,7 @@

msg = self.buildMessage({'id': 1, 'method': 'Host.ping'})
         # Send Truncated message
-        self.assertRaises(struct.error, doPing, msg[:-1])
+        self.assertRaises(ProtocolError, doPing, msg[:-1])

# Test that the server recovers
         doPing(msg)

--
To view, visit http://gerrit.ovirt.org/9499
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I818e5e7b8dd0a1abad94deb65e32c1d76225b839
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: tests: Cleanup apiTests exception handling
......................................................................

Patch Set 1:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/189/ (1/2)

--
To view, visit http://gerrit.ovirt.org/9499
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I818e5e7b8dd0a1abad94deb65e32c1d76225b839
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: tests: Cleanup apiTests exception handling
......................................................................

Patch Set 1:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/155/ (2/2)

--
To view, visit http://gerrit.ovirt.org/9499
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I818e5e7b8dd0a1abad94deb65e32c1d76225b839
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: tests: Cleanup apiTests exception handling
......................................................................

Patch Set 1:

Build Successful

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/155/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/189/ : SUCCESS

--
To view, visit http://gerrit.ovirt.org/9499
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I818e5e7b8dd0a1abad94deb65e32c1d76225b839
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: oVirt Jenkins CI Server

ShaoHe Feng has posted comments on this change.

Change subject: tests: Cleanup apiTests exception handling
......................................................................

Patch Set 1: I would prefer that you didn't submit this

(1 inline comment)

....................................................
File tests/apiTests.py
Line 194:         sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
Line 195:         try:
Line 196:             sock.connect((ip, port))
Line 197:             sock.sendall(msg)
Line 198:         except socket.error, e:
if there is something wrong with JsonRpc server,  client will fail to connect it,  but  for testMissingSize,  the test will pass.
Line 199:             raise ProtocolError("Unable to send request: %s", e)
Line 200:         try:
Line 201:             data = sock.recv(JsonRawTest._Size.size)
Line 202:         except socket.error, e:

--
To view, visit http://gerrit.ovirt.org/9499
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I818e5e7b8dd0a1abad94deb65e32c1d76225b839
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: oVirt Jenkins CI Server

Adam Litke has posted comments on this change.

Change subject: tests: Cleanup apiTests exception handling
......................................................................

Patch Set 1: (1 inline comment)

....................................................
File tests/apiTests.py
Line 194:         sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
Line 195:         try:
Line 196:             sock.connect((ip, port))
Line 197:             sock.sendall(msg)
Line 198:         except socket.error, e:
You're right.  I'll create a new exception for connection error.
Line 199:             raise ProtocolError("Unable to send request: %s", e)
Line 200:         try:
Line 201:             data = sock.recv(JsonRawTest._Size.size)
Line 202:         except socket.error, e:

--
To view, visit http://gerrit.ovirt.org/9499
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I818e5e7b8dd0a1abad94deb65e32c1d76225b839
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: tests: Cleanup apiTests exception handling
......................................................................

Patch Set 2: Verified; Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/9499
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I818e5e7b8dd0a1abad94deb65e32c1d76225b839
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: oVirt Jenkins CI Server

ShaoHe Feng has posted comments on this change.

Change subject: tests: Cleanup apiTests exception handling
......................................................................

Patch Set 2: I would prefer that you didn't submit this

(1 inline comment)

....................................................
File tests/apiTests.py
Line 217:             raise ProtocolError("Unable to read response body: %s", e)
Line 218:         if len(data) != msgLen:
Line 219:             raise ProtocolError("Response body length mismatch")
Line 220:         return json.loads(data)
Line 221:         sock.close()
when will socket close?
 Hopefully, there is a context manager for socket.  like "with socket.socket() as sock"
Line 222: 
Line 223:     def testPing(self):
Line 224:         self.clearAPI()
Line 225:         self.programAPI("testPing")

--
To view, visit http://gerrit.ovirt.org/9499
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I818e5e7b8dd0a1abad94deb65e32c1d76225b839
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: tests: Cleanup apiTests exception handling
......................................................................

Patch Set 2: I would prefer that you didn't submit this

(1 inline comment)

....................................................
File tests/apiTests.py
Line 217:             raise ProtocolError("Unable to read response body: %s", e)
Line 218:         if len(data) != msgLen:
Line 219:             raise ProtocolError("Response body length mismatch")
Line 220:         return json.loads(data)
Line 221:         sock.close()
correct. with closing(socket.socket()) would have been in place. I did not notice that the "finally" clause was dropped.
Line 222: 
Line 223:     def testPing(self):
Line 224:         self.clearAPI()
Line 225:         self.programAPI("testPing")

--
To view, visit http://gerrit.ovirt.org/9499
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I818e5e7b8dd0a1abad94deb65e32c1d76225b839
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: tests: Cleanup apiTests exception handling
......................................................................

Patch Set 3: Verified; Looks good to me, approved

I suppose you would not mind if I push this version after verifying it both on f18 and el6.

--
To view, visit http://gerrit.ovirt.org/9499
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I818e5e7b8dd0a1abad94deb65e32c1d76225b839
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: tests: Cleanup apiTests exception handling
......................................................................

tests: Cleanup apiTests exception handling

As Dan reported in http://gerrit.ovirt.org/#/c/9442/ the behavior of
SocketServer differs between versions of python which causes the
exception raised by sendMessage() to change.  Rather than key the
expected exception based on the Python version, clean up the flow so it
will behave the same across Python versions.

Change-Id: I818e5e7b8dd0a1abad94deb65e32c1d76225b839
Signed-off-by: Adam Litke 
---
M tests/apiTests.py
1 file changed, 34 insertions(+), 11 deletions(-)

Approvals:
  Dan Kenigsberg: Verified; Looks good to me, approved

--
To view, visit http://gerrit.ovirt.org/9499
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I818e5e7b8dd0a1abad94deb65e32c1d76225b839
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: oVirt Jenkins CI Server