Conversation
Initial version of the Python client
tomlegkov
left a comment
tomlegkov
left a comment
There was a problem hiding this comment.
Good job. The main required fix is combining all of the filter_and_parse tests into a single test and adding a few more protocols.
tests/marine/test_marine.py
Outdated
|
|
||
| def test_packet_doesnt_pass_filter_because_of_bfp(marine_instance, tcp_packet, extracted_fields_from_tcp_packet): | ||
| passed, output = marine_instance.filter_and_parse(tcp_packet, 'arp', fields=extracted_fields_from_tcp_packet) | ||
| def test_packet_doesnt_pass_filter_because_of_bfp( |
tests/marine/test_marine.py
Outdated
| extracted_fields_from_udp_packet): | ||
| expected = dict(zip(extracted_fields_from_udp_packet, map(str, [mac_1, mac_2, ip_1, ip_2, port_3, port_4]))) | ||
| passed, output = marine_instance.filter_and_parse(udp_packet, 'ip', 'udp', extracted_fields_from_udp_packet) | ||
| from pytest_lazyfixture import lazy_fixture |
There was a problem hiding this comment.
Add new tests packages to tox.ini, and remember to run tox before pushing code :)
tomlegkov
left a comment
tomlegkov
left a comment
There was a problem hiding this comment.
Fix the tox stuff and it's fine by me. Let's wait for the others to CR too.
yehonatanz
left a comment
yehonatanz
left a comment
There was a problem hiding this comment.
My main issue is about the fixtures hell in test_filter_and_parse.
Let's talk about it on Slack before you proceed
| "http_type": "GET", | ||
| "uri": f"/{create_random_string(8)}", | ||
| "version": "HTTP/1.1", | ||
| "host": create_random_url(), |
There was a problem hiding this comment.
Seem like a domain name
There was a problem hiding this comment.
Also didn't we decide to get rid of randomly generated values?
tests/marine/test_marine.py
Outdated
| zip( | ||
| extracted_fields_from_packet, | ||
| map( | ||
| lambda x: str(request.getfixturevalue(x)), fields_expected_fixture_name |
There was a problem hiding this comment.
Do we always return strings? Does wireshark always return strings? Don't you define a type for every field in lua dissectors?
There was a problem hiding this comment.
Currently all of the results in the Python client are returned as strings, see this open issue.
tests/marine/test_marine.py
Outdated
| bpf_filter="ip", | ||
| display_filter="http", | ||
| ) | ||
| def test_packet_filter_and_parse( |
There was a problem hiding this comment.
The parametrization for this test is way too complicated for what it tests.
I'd really rather you avoided fixtures in this test.
I want to see every tested packet next to every field and its value (with only relevant fields).
Something like:
@Parametrization.case(
name="HTTP",
packet=Ethernet(...) + IP(...) + TCP(...) + HTTP(...),
bpf="ip",
display_filter="http",
fields=["http.type", "http.uri", "http.version"],
expected=["GET", "/pasten", "1.1"],
)Perhaps with some predefined consts for reptetive and not interesting layers (not interesting = not tested).
There was a problem hiding this comment.
I think creating the packet for each case in the file will create a bit of a mess for example, look how many lines are needed to create a DHCP packet.
Is it really interesting to see the values being tested when you read the case, especially when there's only one packet per case? Hiding a lot of the data behind fixtures makes the case a lot more readable.
There was a problem hiding this comment.
I'm not sure about it. My instinct is to make tests as explicit as possible.
Packets with extremly long tested layer (such as DHCP) might indeed justify a fixture (or const), but a standalone one, with no long dependency graph like now.
There was a problem hiding this comment.
I understand your instinct and I agree with it, however it means some cases will contain fixtures and some won't. I think it will be inconvenient to follow.
Maybe the solution is to go back to not using parameterize? Having a separate test for every protocol will allow more flexibility, but will contain code repetition.
yehonatanz
left a comment
yehonatanz
left a comment
There was a problem hiding this comment.
I had just one tiny note.
I'm still not entirely sure about the lack of parametrization, but it's ok for now :)
tests/marine/test_marine.py
Outdated
| marine_instance.filter_and_parse(tcp_packet, display_filter='illegal_filter') | ||
| assert 'neither a field nor a protocol name' in str(excinfo) | ||
| marine_instance.filter_and_parse(tcp_packet, display_filter="illegal_filter") | ||
| assert "neither a field nor a protocol name" in str(excinfo) |
There was a problem hiding this comment.
Use pytest.raises(..., match="some regex")
tests/marine/test_marine.py
Outdated
| bpf_filter: str, | ||
| display_filter: str, |
There was a problem hiding this comment.
In lines 322 and 323 you're passing bpf_filter and display_filter as None, so this should be Optional[str].
tomlegkov
left a comment
tomlegkov
left a comment
There was a problem hiding this comment.
Fix this one tiny thing, and looks good otherwise :)
yehonatanz
left a comment
yehonatanz
left a comment
There was a problem hiding this comment.
Looks good :)
Just fix Tom's notes
4 participants
No description provided.