Skip to content
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

Merged
merged 1 commit into from
Oct 28, 2017

Conversation

dlrobertson
Copy link
Collaborator

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.

  • Add tests for the following
    • ICMP error responses are not sent in response to broadcast requests (this
      isn't super relevant now, but this ensures we don't cause broadcast storms
      once we consume broadcast packets).
    • 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

Copy link
Contributor

@whitequark whitequark left a 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.

0xff, 0xff, 0xff, 0xff,
];

// Create a basic device
Copy link
Contributor

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point

// 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() {
Copy link
Contributor

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));

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


// 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") },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


// 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 => {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

// response. See RFC 1122 § 3.2.2.
let bytes = vec![
// Ethernet Frame
0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IpProtocol::Unknown(0xwhatever)

Copy link
Collaborator Author

@dlrobertson dlrobertson left a 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)]
Copy link
Collaborator Author

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

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())
Copy link
Collaborator Author

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

Copy link
Contributor

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.

Copy link
Contributor

@whitequark whitequark left a 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.

// Create a basic device
let mut device = Loopback::new();

let mut arp_cache_entries: [_; 8] = Default::default();
Copy link
Contributor

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.

Sorry, something went wrong.

Copy link
Collaborator Author

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

Sorry, something went wrong.

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)];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

};

let mut socket_set_entries: [_; 1] = Default::default();
let mut socket_set = SocketSet::new(&mut socket_set_entries[..]);
Copy link
Contributor

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
@whitequark whitequark merged commit 7a2271d into smoltcp-rs:master Oct 28, 2017
@dlrobertson dlrobertson deleted the add_iface_tests branch October 28, 2017 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants