Teststruct improvement application#404
Teststruct improvement application#404erickvermot wants to merge 53 commits intokytos:masterfrom erickvermot:teststruct_improvement_application
Conversation
diraol
left a comment
diraol
left a comment
There was a problem hiding this comment.
Besides the comments I've made, please, review all changes considering the following:
To be in compliance with PEP8, when passing arguments to functions/methods or objects instantiations try filling out the line until the line width limit (79 characters).
Which means, avoid passing one argument per line. This syntax (one per line) is used when it is expected to have more items added to that "list".
|
|
||
| class TestUBInt8(unittest.TestCase): | ||
| """Test of UBInt8 BasicType.""" | ||
| class TestUBInt8_(unittest.TestCase): |
There was a problem hiding this comment.
Shouldn't this inherit from TestStructDump ?
|
|
||
| class TestUBInt8(unittest.TestCase): | ||
| """Test of UBInt8 BasicType.""" | ||
| class TestUBInt8_(unittest.TestCase): |
There was a problem hiding this comment.
And why are you using an underscore on the end of the class name?
| """Basic test setup.""" | ||
| self.ubint8 = basic_types.UBInt8() | ||
| dump = b'\xff' | ||
| obj = basic_types.UBInt32(2**8 - 1) |
There was a problem hiding this comment.
If you are testing the UBInt8 class, why are you using UBInt32 here?
| def test_pack(self): | ||
| """[Foundation/BasicTypes/UBInt8] - packing.""" | ||
| pass | ||
| class TestUBInt16_(unittest.TestCase): |
There was a problem hiding this comment.
Shouldn't this inherit from TestStructDump?
|
|
||
| class TestUBInt16(unittest.TestCase): | ||
| """Test of UBInt16 BasicType.""" | ||
| class TestUBInt32_(unittest.TestCase): |
There was a problem hiding this comment.
Shouldn't this inherit from TestStructDump?
| super().set_minimum_size(16) | ||
| dumpfile = 'v0x01/ofpt_action_enqueue.dat' | ||
| obj = ActionEnqueue(port=Port.OFPP_CONTROLLER, | ||
| queue_id=4) |
There was a problem hiding this comment.
No need for line break here.
| super().set_minimum_size(8) | ||
| dumpfile = 'v0x01/ofpt_action_output.dat' | ||
| obj = ActionOutput(port=Port.OFPP_CONTROLLER, | ||
| max_length=8) |
There was a problem hiding this comment.
No need for line break here
| tp_dst=22) | ||
|
|
||
|
|
||
| class TestMatch_2(TestStructDump): |
| """Each Header instantiations without xid should call randint.""" | ||
| Header(), Header() # noqa | ||
| self.assertEqual(m.call_count, 2) | ||
| # @patch('pyof.v0x01.common.header.randint') |
There was a problem hiding this comment.
Why leave it here commented out?
There was a problem hiding this comment.
because it was meant for us to discuss during review. I did not understand this test.
| def test_get_size(self): | ||
| """[Common/PacketQueue] - size 8.""" | ||
| self.assertEqual(self.message.get_size(), 8) | ||
| dump = b'\x00\x00\x00\x01\x00\x08\x00\x00' # needs to be checked |
|
A few comments:
Apart from these little changes, anything important to point out about the proposed mods? |
|
Let's close this for now. |
3 participants
relates to #398
it applies the logic in #406
still needs to implement to v0x04
this needs:
#397
#403