-
Notifications
You must be signed in to change notification settings - Fork 447
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
Tests: Add basic interface tests #61
Conversation
9bdc957
to
943ab8f
Compare
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.
Thank you for the PR! Certainly, tests for the Ethernet interface are long overdue, and I am grateful for your work. However, it still needs some improvement.
src/iface/ethernet.rs
Outdated
0xff, 0xff, 0xff, 0xff, | ||
]; | ||
|
||
// Create a basic device |
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.
This common initialization code can be taken out of the tests and into a setup function.
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.
Good point
src/iface/ethernet.rs
Outdated
// Ensure that the unknown protocol frame does not trigger an | ||
// ICMP error response when the destination address is a | ||
// broadcast address | ||
match iface.process_ipv4(&mut socket_set, 0, &frame).unwrap() { |
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.
A better way to express this would be:
assert_eq!(iface.process_ipv4(&mut socket_set, 0, &frame),
Ok(Packet::None));
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.
👍
src/iface/ethernet.rs
Outdated
|
||
// Ensure an ARP Request for us triggers an ARP Reply | ||
match iface.process_ethernet(&mut socket_set, 0, &bytes).unwrap() { | ||
Packet::None => { panic!("Respond to ARP Requests") }, |
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.
Same as above.
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.
👍
src/iface/ethernet.rs
Outdated
|
||
// Ensure an ARP Request for someone else does not trigger an ARP Reply | ||
match iface.process_ethernet(&mut socket_set, 0, &bytes).unwrap() { | ||
Packet::None => {}, |
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.
Same as above.
src/iface/ethernet.rs
Outdated
// response. See RFC 1122 § 3.2.2. | ||
let bytes = vec![ | ||
// Ethernet Frame | ||
0xff, 0xff, 0xff, 0xff, 0xff, 0xff, |
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.
Since we are not testing the packet parsing/construction routines here, do you think you could use EthernetFrame
/IpRepr
/ArpRepr
/etc to construct the bytewise representations? That would make tests much easier to understand and write.
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.
This would definitely be feasible for the ARP tests. I'm not sure about how feasible it would be for this test since the protocol type is a protocol that isn't in IpProtocol
.
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.
IpProtocol::Unknown(0xwhatever)
943ab8f
to
e1fa197
Compare
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.
Updated to use assert_eq!
, a interface creation helper, and the Repr
structures to improve readability.
@@ -32,6 +32,7 @@ pub struct Interface<'a, 'b, 'c, DeviceT: Device + 'a> { | |||
ipv4_gateway: Option<Ipv4Address>, | |||
} | |||
|
|||
#[derive(Debug, PartialEq)] |
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.
Added these so that assert_eq!
would work
src/iface/ethernet.rs
Outdated
frame.set_src_addr(EthernetAddress([0x52, 0x54, 0x00, 0x00, 0x00, 0x00])); | ||
frame.set_ethertype(EthernetProtocol::Ipv4); | ||
repr.emit(frame.payload_mut(), &ChecksumCapabilities::default()); | ||
EthernetFrame::new(&*frame.into_inner()) |
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.
Kind of ugly, but inner type mutability differs without this
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.
That's a perfectly fine construct in smoltcp.
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.
Just a few nits. Afterwards I think I'll merge with a few stylistic changes.
src/iface/ethernet.rs
Outdated
// Create a basic device | ||
let mut device = Loopback::new(); | ||
|
||
let mut arp_cache_entries: [_; 8] = Default::default(); |
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.
You can box everything here and then you can just return EthernetInterface by value.
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.
Good point. It's a bit more readable that way
src/iface/ethernet.rs
Outdated
let mut arp_cache_entries: [_; 8] = Default::default(); | ||
let mut arp_cache = SliceArpCache::new(&mut arp_cache_entries[..]); | ||
|
||
let mut ip_addrs = [IpCidr::new(IpAddress::v4(127, 0, 0, 1), 8)]; |
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.
Same here.
src/iface/ethernet.rs
Outdated
}; | ||
|
||
let mut socket_set_entries: [_; 1] = Default::default(); | ||
let mut socket_set = SocketSet::new(&mut socket_set_entries[..]); |
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.
Same here.
- Add tests for the following - ICMP error responses are not sent in response to broadcast requests - ARP requests are responded to and inserted into the cache - ARP requests for someone else are not responded to, but the sender is still inserted in the cache
e1fa197
to
2d990a8
Compare
Was looking at how the
EthernetInterface
worked and wrote some basic unit tests. Feel free to close this if you feel these tests aren't that useful.isn't super relevant now, but this ensures we don't cause broadcast storms
once we consume broadcast packets).
is still inserted in the cache