Skip to content

Commit

Permalink
Fix packet reader test
Browse files Browse the repository at this point in the history
Clippy nightly has a new warning for us:

    error: this loop never actually loops
         --> x11rb-protocol/src/packet_reader.rs:135:9
          |
      135 | /         for mut packet in packets {
      136 | |             let original_packet = packet.clone();
      137 | |
      138 | |             loop {
      ...   |
      148 | |             }
      149 | |         }
          | |_________^
          |
          = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#never_loop
          = note: `#[deny(clippy::never_loop)]` on by default
      help: if you need the first element of the iterator, try writing
          |
      135 |         if let Some(mut packet) = packets.into_iter().next() {
          |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

It turns out that the function test_packets(), which is supposed to test
that a list of packets is correctly read by PacketReader, only ever
tested the first packet and ignored everything else.

This commit fixes the test.

First, test_packets() now puts all the packet data into one, big chunk,
instead of feeding the packets already on the right boundaries to the
PacketReader. After all, we want to test that this correctly determines
the boundaries again.

After fixing that, the test started to fail. It turns out that two of
the tests actually produced packages whose length is not a multiple of
four, but all X11 packets must have a length that is a multiple of four.
Fix this by rounding down to a correct multiple.

These tests were added in commit 2082b2b. Before that, there were
no tests at all.

Signed-off-by: Uli Schlachter <[email protected]>
  • Loading branch information
psychon committed Oct 6, 2023
1 parent aca14e3 commit 8105170
Showing 1 changed file with 16 additions and 8 deletions.
24 changes: 16 additions & 8 deletions x11rb-protocol/src/packet_reader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,19 +131,21 @@ mod tests {
use alloc::{vec, vec::Vec};

fn test_packets(packets: Vec<Vec<u8>>) {
let mut reader = PacketReader::default();
for mut packet in packets {
let original_packet = packet.clone();
// Combine all packet data into one big chunk and test that the packet reader splits things
let mut all_data = packets.iter().flat_map(|packet| packet).copied().collect::<Vec<u8>>();

let mut reader = PacketReader::default();
for (i, packet) in packets.into_iter().enumerate() {
std::println!("Checking packet {i}");
loop {
let buffer = reader.buffer();
let amount = std::cmp::min(buffer.len(), packet.len());
buffer.copy_from_slice(&packet[..amount]);
let _ = packet.drain(..amount);
let amount = std::cmp::min(buffer.len(), all_data.len());
buffer.copy_from_slice(&all_data[..amount]);
let _ = all_data.drain(..amount);

if let Some(read_packet) = reader.advance(amount) {
assert_eq!(read_packet, original_packet);
return;
assert_eq!(read_packet, packet);
break;
}
}
}
Expand Down Expand Up @@ -193,9 +195,12 @@ mod tests {
let variation = ((i - 50) * (i - 50)) as f32;
let variation = -1.0 / 25.0 * variation + 100.0;
let variation = variation as usize;
// round to a multiple of 4
let variation = variation / 4 * 4;

let mut len = 1200 + variation;
let mut packet = vec![0; len];
assert_eq!(0, len % 4);
len = (len - 32) / 4;

// write "len" to bytes 4..8 in the packet
Expand All @@ -219,11 +224,14 @@ mod tests {
let variation = ((i - 50) * (i - 50)) as f32;
let variation = -1.0 / 25.0 * variation + 100.0;
let variation = variation as usize;
// round to a multiple of 4
let variation = variation / 4 * 4;

1200 + variation
} else {
32
};
assert_eq!(0, len % 4);
let mut packet = vec![0; len];
len = (len - 32) / 4;

Expand Down

0 comments on commit 8105170

Please sign in to comment.