Zhou Zheng Sheng has posted comments on this change.
Change subject: Improvement of the GuestAgent class memory usage
......................................................................
Patch Set 8: I would prefer that you didn't submit this
(3 inline comments)
I think you can improve the testing code.
....................................................
File tests/guestIFTests.py
Line 102: t = testCase(*t)
Line 103: fakeGuestAgent._handleMessage(t.msgType, t.message)
Line 104: for (k, v) in t.assertDict.iteritems():
Line 105: self.assertEqual(fakeGuestAgent.guestInfo[k], v)
Line 106:
1. Is it better to put the _handelData test code to a test_handleData() test
method? The variable definition for testCase, msgTypes, inputs and outpust can
be moved to a setUp() method, and assign the values to self._testCase,
self._msgTypes, ... nose runs setUp() for each test method so I think it's a
good place to put common definitions. Then in a separate test_handleData()
method you can reuse those definitions.
2.
"config.set('vars', 'guest_agent_max_allowed_message_size', '100')"
and
"fakeGuestAgent._clearReadBuffer()"
is not related to test_handleMessage(), so move them to test_handleData()
maybe better.
Line 107: # Guest agent must not be stopped
Line 108: fakeGuestAgent._stopped = False
Line 109: # Retrieving configured message size
Line 110: maxMessageSize = config.getint('vars',
Line 125: for chunk in messageChunks(msg_str):
Line 126: fakeGuestAgent._handleData(chunk)
Line 127: if chunk[-1] != '\n':
Line 128: self.assertEqual(fakeGuestAgent._messageState,
Line 129: guestIF.MessageState.TOO_BIG)
If chunk[-1] equals to '\n', the fakeGuestAgent._messageState should be NORMAL.
Line 130:
Line 131: # At the end the message state has to be NORMAL again
Line 132: self.assertEqual(fakeGuestAgent._messageState,
Line 133: guestIF.MessageState.NORMAL)
Line 132: self.assertEqual(fakeGuestAgent._messageState,
Line 133: guestIF.MessageState.NORMAL)
Line 134:
Line 135: for (k, v) in t.assertDict.iteritems():
Line 136: self.assertEqual(fakeGuestAgent.guestInfo[k], v)
After the _handleMessage() testing is executed, the fakeGuestAgent.guestInfo is
updated to match the values in the assertDict. Then all the above messages for
_handleData() exceeds 100B and are discarded, and the fakeGuestAgent.guestInfo
remains the same, so all the assertEquals will be absolutely OK. Even if the
messages are not discarded, they will be parsed as before and update the
fakeGuestAgent.guestInfo again with the same values. So these assertEquals
proves nothing, they can be dropped. Also, if you move the _handleData()
testing code to a separate test method, these assertEquals will fail.
I think we can test the following situations.
1. Message is too big and gets discarded, so the fakeGuestAgent.guestInfo is
not updated.
2. Message is normal and gets accepted, so the fakeGuestAgent.guestInfo is
updated.
3. A big message + normal message are send to _handleMessage() in one time, and
the big one gets discarded, the normal one gets accepted, the
fakeGuestAgent.guestInfo is updated only by the normal one.
--
To view, visit http://gerrit.ovirt.org/9239
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf6274bb10c9e3b80962b69c5df316f03ee21214
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vinzenz Feenstra <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Gal Hammer <[email protected]>
Gerrit-Reviewer: Igor Lvovsky <[email protected]>
Gerrit-Reviewer: Peter V. Saveliev <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: ShaoHe Feng <[email protected]>
Gerrit-Reviewer: Vinzenz Feenstra <[email protected]>
Gerrit-Reviewer: Zhou Zheng Sheng <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
