-
Notifications
You must be signed in to change notification settings - Fork 156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unit tests for lib/packet/scion.py #204
Conversation
b717a7a
to
5b257f0
Compare
return req | ||
|
||
def pack(self): | ||
self.payload = struct.pack("HH", self.reply_id, self.request_id) | ||
self.payload = struct.pack("!HH", self.reply_id, self.request_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After seeing the sorts of changes needed for this, i would prefer it was in a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you go: #209
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -1,4 +1,4 @@ | |||
# Copyright 2014 ETH Zurich | |||
# Copyright 2015 ETH Zurich |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know the exact legal rules here, but i would leave the year untouched.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Ok, finished this review pass, with mostly minor comments. |
e030af2
to
a2729fd
Compare
@kormat , I think this is complete, you can have a look. |
hdr.common_hdr.total_len = 200 | ||
path = MagicMock(spec_set=['pack']) | ||
path.pack.return_value = b'packed_path' | ||
hdr.set_path(path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need an assertion that pack.pack
was called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
15f31af
to
69472e8
Compare
scion_common_hdr.return_value = common_hdr | ||
scion_common_hdr.LEN = 2 | ||
scion_addr.side_effect = ['src_addr', 'dst_addr'] | ||
ntools.eq_(hdr._parse_common_hdr(data, 2), 2 + 2 + 3 + 5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest changing the initial offset to 1, so it's easier to see where the result numbers come from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In TestSIONHeaderParseCommonHdr.test
LGTM, nice work :) |
Unit tests for lib/packet/scion.py
Unit tests for lib/packet/scion.py
(work in progress)@kormat, this is ready for a review.
lib.packet.opaque_field
should come beforelib.packet.path
pack()
src_addr_len
comes beforedst_addr_len
scion_common_hdr.assert_called_once_with
is still needed, to check thatnext_hdr
does default to 0.path=None, _path=something
, this test currently coverspath=something, _path=something
, but currently nothing covers the_path=None
case. I would change this test to cover that.self._extension_hdrs
containing items.self.pop_ext_hdr()
call, how can I ensure that the while loop is terminated?spect_set
should bespec_set
**kwargs
argument, that will silently accept any unknown keyword args, which is why it was hiddenappend_ext_hdr
is using addition, and not just settingtotal_len
equal tolen(ext_hdr)
=
+
0x0102
and0x0304
'data'
pack.pack
was called