Skip to content

Commit

Permalink
refactor: use matches! instead of to_string() for tests
Browse files Browse the repository at this point in the history 
Some tests that were explicitly testing for error conditions used
converted errors to strings to determine whether two errors are the same
(by saying they're only the same if their string representation was
identical). Replace this with more roboust assertions on `matches!`.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
  • Loading branch information
@roypat
roypat committed Feb 10, 2025
1 parent b31bfd0 commit df86175
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 198 deletions.
179 changes: 56 additions & 123 deletions src/mmap.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ mod tests {

use crate::bitmap::tests::test_guest_memory_and_region;
use crate::bitmap::AtomicBitmap;
use crate::{Error, GuestAddressSpace, GuestMemory};
use crate::{Error, GuestAddressSpace, GuestMemory, GuestMemoryError};

use std::io::Write;
use std::mem;
Expand Down Expand Up @@ -481,129 +481,66 @@ mod tests {
fn test_no_memory_region() {
let regions_summary = [];

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap(&regions_summary).err().unwrap()
),
format!("{:?}", Error::NoMemoryRegion)
);

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap_with_files(&regions_summary)
.err()
.unwrap()
),
format!("{:?}", Error::NoMemoryRegion)
);

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap_from_regions(&regions_summary)
.err()
.unwrap()
),
format!("{:?}", Error::NoMemoryRegion)
);

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap_from_arc_regions(&regions_summary)
.err()
.unwrap()
),
format!("{:?}", Error::NoMemoryRegion)
);
assert!(matches!(
new_guest_memory_mmap(&regions_summary).unwrap_err(),
Error::NoMemoryRegion
));
assert!(matches!(
new_guest_memory_mmap_with_files(&regions_summary).unwrap_err(),
Error::NoMemoryRegion
));
assert!(matches!(
new_guest_memory_mmap_from_regions(&regions_summary).unwrap_err(),
Error::NoMemoryRegion
));
assert!(matches!(
new_guest_memory_mmap_from_regions(&regions_summary).unwrap_err(),
Error::NoMemoryRegion
));
}

#[test]
fn test_overlapping_memory_regions() {
let regions_summary = [(GuestAddress(0), 100_usize), (GuestAddress(99), 100_usize)];

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap(&regions_summary).err().unwrap()
),
format!("{:?}", Error::MemoryRegionOverlap)
);

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap_with_files(&regions_summary)
.err()
.unwrap()
),
format!("{:?}", Error::MemoryRegionOverlap)
);

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap_from_regions(&regions_summary)
.err()
.unwrap()
),
format!("{:?}", Error::MemoryRegionOverlap)
);

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap_from_arc_regions(&regions_summary)
.err()
.unwrap()
),
format!("{:?}", Error::MemoryRegionOverlap)
);
assert!(matches!(
new_guest_memory_mmap(&regions_summary).unwrap_err(),
Error::MemoryRegionOverlap
));
assert!(matches!(
new_guest_memory_mmap_with_files(&regions_summary).unwrap_err(),
Error::MemoryRegionOverlap
));
assert!(matches!(
new_guest_memory_mmap_from_regions(&regions_summary).unwrap_err(),
Error::MemoryRegionOverlap
));
assert!(matches!(
new_guest_memory_mmap_from_regions(&regions_summary).unwrap_err(),
Error::MemoryRegionOverlap
));
}

#[test]
fn test_unsorted_memory_regions() {
let regions_summary = [(GuestAddress(100), 100_usize), (GuestAddress(0), 100_usize)];

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap(&regions_summary).err().unwrap()
),
format!("{:?}", Error::UnsortedMemoryRegions)
);

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap_with_files(&regions_summary)
.err()
.unwrap()
),
format!("{:?}", Error::UnsortedMemoryRegions)
);

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap_from_regions(&regions_summary)
.err()
.unwrap()
),
format!("{:?}", Error::UnsortedMemoryRegions)
);

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap_from_arc_regions(&regions_summary)
.err()
.unwrap()
),
format!("{:?}", Error::UnsortedMemoryRegions)
);
assert!(matches!(
new_guest_memory_mmap(&regions_summary).unwrap_err(),
Error::UnsortedMemoryRegions
));
assert!(matches!(
new_guest_memory_mmap_with_files(&regions_summary).unwrap_err(),
Error::UnsortedMemoryRegions
));
assert!(matches!(
new_guest_memory_mmap_from_regions(&regions_summary).unwrap_err(),
Error::UnsortedMemoryRegions
));
assert!(matches!(
new_guest_memory_mmap_from_regions(&regions_summary).unwrap_err(),
Error::UnsortedMemoryRegions
));
}

#[test]
Expand Down Expand Up @@ -828,17 +765,13 @@ mod tests {
for gm in gm_list.iter() {
let val1: u64 = 0xaa55_aa55_aa55_aa55;
let val2: u64 = 0x55aa_55aa_55aa_55aa;
assert_eq!(
format!("{:?}", gm.write_obj(val1, bad_addr).err().unwrap()),
format!("InvalidGuestAddress({:?})", bad_addr,)
);
assert_eq!(
format!("{:?}", gm.write_obj(val1, bad_addr2).err().unwrap()),
format!(
"PartialBuffer {{ expected: {:?}, completed: {:?} }}",
mem::size_of::<u64>(),
max_addr.checked_offset_from(bad_addr2).unwrap()
)
assert!(matches!(
gm.write_obj(val1, bad_addr).unwrap_err(),
GuestMemoryError::InvalidGuestAddress(addr) if addr == bad_addr
));
assert!(matches!(
gm.write_obj(val1, bad_addr2).unwrap_err(),
GuestMemoryError::PartialBuffer { expected, completed} if expected == size_of::<u64>() && completed == max_addr.checked_offset_from(bad_addr2).unwrap() as usize)
);

gm.write_obj(val1, GuestAddress(0x500)).unwrap();
Expand Down
8 changes: 4 additions & 4 deletions src/mmap_unix.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,7 @@ mod tests {
prot,
flags,
);
assert_eq!(format!("{:?}", r.unwrap_err()), "InvalidOffsetLength");
assert!(matches!(r.unwrap_err(), Error::InvalidOffsetLength));

// Offset + size is greater than the size of the file (which is 0 at this point).
let r = MmapRegion::build(
Expand All @@ -567,7 +567,7 @@ mod tests {
prot,
flags,
);
assert_eq!(format!("{:?}", r.unwrap_err()), "MappingPastEof");
assert!(matches!(r.unwrap_err(), Error::MappingPastEof));

// MAP_FIXED was specified among the flags.
let r = MmapRegion::build(
Expand All @@ -576,7 +576,7 @@ mod tests {
prot,
flags | libc::MAP_FIXED,
);
assert_eq!(format!("{:?}", r.unwrap_err()), "MapFixed");
assert!(matches!(r.unwrap_err(), Error::MapFixed));

// Let's resize the file.
assert_eq!(unsafe { libc::ftruncate(a.as_raw_fd(), 1024 * 10) }, 0);
Expand Down Expand Up @@ -621,7 +621,7 @@ mod tests {
let flags = libc::MAP_NORESERVE | libc::MAP_PRIVATE;

let r = unsafe { MmapRegion::build_raw((addr + 1) as *mut u8, size, prot, flags) };
assert_eq!(format!("{:?}", r.unwrap_err()), "InvalidPointer");
assert!(matches!(r.unwrap_err(), Error::InvalidPointer));

let r = unsafe { MmapRegion::build_raw(addr as *mut u8, size, prot, flags).unwrap() };

Expand Down
30 changes: 11 additions & 19 deletions src/mmap_xen.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1077,26 +1077,18 @@ mod tests {
range.mmap_flags = 16;

let r = MmapXen::new(&range);
assert_eq!(
format!("{:?}", r.unwrap_err()),
format!("MmapFlags({})", range.mmap_flags),
);
assert!(matches!(r.unwrap_err(), Error::MmapFlags(flags) if flags == range.mmap_flags));

range.mmap_flags = MmapXenFlags::FOREIGN.bits() | MmapXenFlags::GRANT.bits();
let r = MmapXen::new(&range);
assert_eq!(
format!("{:?}", r.unwrap_err()),
format!("MmapFlags({:x})", MmapXenFlags::ALL.bits()),
assert!(
matches!(r.unwrap_err(), Error::MmapFlags(flags) if flags == MmapXenFlags::ALL.bits())
);

range.mmap_flags = MmapXenFlags::FOREIGN.bits() | MmapXenFlags::NO_ADVANCE_MAP.bits();
let r = MmapXen::new(&range);
assert_eq!(
format!("{:?}", r.unwrap_err()),
format!(
"MmapFlags({:x})",
MmapXenFlags::NO_ADVANCE_MAP.bits() | MmapXenFlags::FOREIGN.bits(),
),
assert!(
matches!(r.unwrap_err(), Error::MmapFlags(flags) if flags == MmapXenFlags::NO_ADVANCE_MAP.bits() | MmapXenFlags::FOREIGN.bits())
);
}

Expand Down Expand Up @@ -1132,17 +1124,17 @@ mod tests {
range.file_offset = Some(FileOffset::new(TempFile::new().unwrap().into_file(), 0));
range.prot = None;
let r = MmapXenForeign::new(&range);
assert_eq!(format!("{:?}", r.unwrap_err()), "UnexpectedError");
assert!(matches!(r.unwrap_err(), Error::UnexpectedError));

let mut range = MmapRange::initialized(true);
range.flags = None;
let r = MmapXenForeign::new(&range);
assert_eq!(format!("{:?}", r.unwrap_err()), "UnexpectedError");
assert!(matches!(r.unwrap_err(), Error::UnexpectedError));

let mut range = MmapRange::initialized(true);
range.file_offset = Some(FileOffset::new(TempFile::new().unwrap().into_file(), 1));
let r = MmapXenForeign::new(&range);
assert_eq!(format!("{:?}", r.unwrap_err()), "InvalidOffsetLength");
assert!(matches!(r.unwrap_err(), Error::InvalidOffsetLength));

let mut range = MmapRange::initialized(true);
range.size = 0;
Expand All @@ -1164,7 +1156,7 @@ mod tests {
let mut range = MmapRange::initialized(true);
range.prot = None;
let r = MmapXenGrant::new(&range, MmapXenFlags::empty());
assert_eq!(format!("{:?}", r.unwrap_err()), "UnexpectedError");
assert!(matches!(r.unwrap_err(), Error::UnexpectedError));

let mut range = MmapRange::initialized(true);
range.prot = None;
Expand All @@ -1174,12 +1166,12 @@ mod tests {
let mut range = MmapRange::initialized(true);
range.flags = None;
let r = MmapXenGrant::new(&range, MmapXenFlags::NO_ADVANCE_MAP);
assert_eq!(format!("{:?}", r.unwrap_err()), "UnexpectedError");
assert!(matches!(r.unwrap_err(), Error::UnexpectedError));

let mut range = MmapRange::initialized(true);
range.file_offset = Some(FileOffset::new(TempFile::new().unwrap().into_file(), 1));
let r = MmapXenGrant::new(&range, MmapXenFlags::NO_ADVANCE_MAP);
assert_eq!(format!("{:?}", r.unwrap_err()), "InvalidOffsetLength");
assert!(matches!(r.unwrap_err(), Error::InvalidOffsetLength));

let mut range = MmapRange::initialized(true);
range.size = 0;
Expand Down
52 changes: 0 additions & 52 deletions src/volatile_memory.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1488,58 +1488,6 @@ mod tests {
slice.compute_end_offset(6, 0).unwrap_err();
}

#[test]
fn test_display_error() {
assert_eq!(
format!("{}", Error::OutOfBounds { addr: 0x10 }),
"address 0x10 is out of bounds"
);

assert_eq!(
format!(
"{}",
Error::Overflow {
base: 0x0,
offset: 0x10
}
),
"address 0x0 offset by 0x10 would overflow"
);

assert_eq!(
format!(
"{}",
Error::TooBig {
nelements: 100_000,
size: 1_000_000_000
}
),
"100000 elements of size 1000000000 would overflow a usize"
);

assert_eq!(
format!(
"{}",
Error::Misaligned {
addr: 0x4,
alignment: 8
}
),
"address 0x4 is not aligned to 8"
);

assert_eq!(
format!(
"{}",
Error::PartialBuffer {
expected: 100,
completed: 90
}
),
"only used 90 bytes in 100 long buffer"
);
}

#[test]
fn misaligned_ref() {
let mut a = [0u8; 3];
Expand Down

0 comments on commit df86175

Please sign in to comment.