Turn strings in headers into bytes. Necessary for Py3 compatibility.

kbandla / dpkt

fast, simple packet creation / parsing, with definitions for the basic TCP/IP protocols

Other

1.09k stars 270 forks source link

Turn strings in headers into bytes. Necessary for Py3 compatibility. #461

Closed mx closed 4 years ago

mx commented 4 years ago

I was performing a Python3 migration of code that depends on dpkt when I found a problem with bytes(packet) or str(packet). The header packing methods would throw with the following error: "struct.error: argument for 's' must be a bytes object"

Some other github issues have been created on this same topic for specific packet types; I went through and fixed every example that I could find. This should be a NOP in Python2 (as b'' is an alias for '' there), and fix the above errors in Python3.

coveralls commented 4 years ago

Coverage decreased (-0.007%) to 89.093% when pulling 22c9d3dc090dc099efb8c0e60dcc6092a84ca7f1 on mx:py3 into 2c6aada35c38c93061ac6d4c47862174ffcc4799 on kbandla:master.

obormot commented 4 years ago

Good stuff. I'm surprised the unit tests didn't catch any of this. Please add at least one unit test that would fail before the change and work after the change.

Some other github issues have been created on this same topic

Please reference these here so we could close them by merging this PR

mx commented 4 years ago

Please reference these here so we could close them by merging this PR

385

Please add at least one unit test that would fail before the change and work after the change. Done

mx commented 4 years ago

Not sure how it thinks coverage decreased when I added that test, but I'm gonna guess that's a false positive.

obormot commented 4 years ago

Not sure how it thinks coverage decreased when I added that test, but I'm gonna guess that's a false positive.

It decreased because the 2 lines you added in the unit test

except:
  raised = True

were not executed hence decreased coverage (dumb I know..)

Perhaps restructure the unit test into something like

eth = Ethernet(data=b'12345')
assert str(eth)

mx commented 4 years ago

Not sure how it thinks coverage decreased when I added that test, but I'm gonna guess that's a false positive.

It decreased because the 2 lines you added in the unit test
except:
  raised = True
were not executed hence decreased coverage (dumb I know..)

Perhaps restructure the unit test into something like
eth = Ethernet(data=b'12345')
assert str(eth)

This feels less clear to the viewer than what's going on there now. I can do something like that if you want, but it feels like making it worse in order to please a metric (coverage).

obormot commented 4 years ago

This feels less clear to the viewer than what's going on there now. I can do something like that if you want, but it feels like making it worse in order to please a metric (coverage).

It's rather to satisfy the metric in order to be able to merge. Can't merge it with decreased coverage

mx commented 4 years ago

This feels less clear to the viewer than what's going on there now. I can do something like that if you want, but it feels like making it worse in order to please a metric (coverage).

It's rather to satisfy the metric in order to be able to merge. Can't merge it with decreased coverage

Done, then.

mx commented 4 years ago

The remaining coverage failure is because I added the test to main with the other tests.