Merge pull request #3 from Yoshanuikabundi/rusty-error-codes

Rusty error codes

src/errors.rs

@@ -0,0 +1,312 @@
+use crate::c_abi;
+use crate::FileMode;
+use crate::Frame;
+use std::error::Error as StdError;
+use std::path::{Path, PathBuf};
+
+/// Error type for the xdrfile library
+#[derive(Debug, Clone, PartialEq)]
+pub enum Error {
+    /// An error code from the C API
+    CApiError {
+        code: ErrorCode,
+        task: ErrorTask,
+    },
+    /// Passed in a frame of the wrong size
+    WrongSizeFrame {
+        expected: usize,
+        found: usize,
+    },
+    /// C API failed to open a file (No return code provided)
+    CouldNotOpen {
+        path: PathBuf,
+        mode: FileMode,
+    },
+    /// A path could not be converted to &OsStr
+    InvalidOsStr,
+    /// A path could not be converted to &CStr because it had a null byte
+    NullInStr(std::ffi::NulError),
+    CouldNotCheckNAtoms(Box<Error>),
+}
+
+impl Error {
+    /// Get the error code returned by the C API, if any
+    pub fn code(&self) -> Option<ErrorCode> {
+        if let Error::CApiError { code, .. } = self {
+            Some(*code)
+        } else if let Some(e) = self.source() {
+            e.downcast_ref::<Self>().and_then(Self::code)
+        } else {
+            None
+        }
+    }
+
+    /// Get the task being attempted when the C API returned an error, if any
+    pub fn task(&self) -> Option<ErrorTask> {
+        if let Error::CApiError { task, .. } = self {
+            Some(*task)
+        } else if let Some(e) = self.source() {
+            e.downcast_ref::<Self>().and_then(Self::task)
+        } else {
+            None
+        }
+    }
+
+    /// True if the error is an end of file error, false otherwise
+    pub fn is_eof(&self) -> bool {
+        self.code().map_or(false, |e| e.is_eof())
+    }
+}
+
+impl std::error::Error for Error {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        use Error::*;
+        match &self {
+            NullInStr(err) => Some(err),
+            CouldNotCheckNAtoms(err) => Some(err.as_ref()),
+            _ => None,
+        }
+    }
+}
+
+impl From<(ErrorCode, ErrorTask)> for Error {
+    fn from(value: (ErrorCode, ErrorTask)) -> Self {
+        let (code, task) = value;
+        Self::CApiError { code, task }
+    }
+}
+
+impl From<std::ffi::NulError> for Error {
+    fn from(err: std::ffi::NulError) -> Self {
+        Self::NullInStr(err)
+    }
+}
+
+impl From<(&Path, FileMode)> for Error {
+    fn from(value: (&Path, FileMode)) -> Self {
+        let (path, mode) = value;
+        Error::CouldNotOpen {
+            path: path.to_owned(),
+            mode,
+        }
+    }
+}
+
+impl From<(&Frame, usize)> for Error {
+    fn from(value: (&Frame, usize)) -> Self {
+        let (frame, num_atoms) = value;
+        Error::WrongSizeFrame {
+            expected: num_atoms,
+            found: frame.coords.len(),
+        }
+    }
+}
+
+impl std::fmt::Display for Error {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        use Error::*;
+        match &self {
+            CApiError { code, task } => write!(
+                f,
+                "Error while {task}: C API returned error code {code}",
+                task = task,
+                code = code
+            ),
+            WrongSizeFrame { expected, found } => write!(
+                f,
+                "Expected frame of size {:?}, found {:?}",
+                expected, found
+            ),
+            CouldNotOpen { path, mode } => {
+                write!(f, "Could not open file at {:?} in mode {:?}", path, mode)
+            }
+            InvalidOsStr => write!(f, "Paths must be valid unicode on this platform"),
+            NullInStr(_err) => write!(f, "Paths cannot include null bytes"),
+            CouldNotCheckNAtoms(_err) => write!(
+                f,
+                "Failed to read number of atoms in trajectory file"
+            )
+        }
+    }
+}
+
+/// The task being attempted when the C API returns an error
+#[derive(Debug, Clone, Copy, PartialEq)]
+pub enum ErrorTask {
+    /// The number of atoms was being read from a file
+    ReadNumAtoms,
+    /// A frame was being read from a file
+    Read,
+    /// A frame was being written to a file
+    Write,
+    /// An file was being flushed to disk
+    Flush,
+}
+
+impl std::fmt::Display for ErrorTask {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        use ErrorTask::*;
+        match &self {
+            ReadNumAtoms => write!(f, "reading atom number from trajectory"),
+            Read => write!(f, "reading trajectory"),
+            Write => write!(f, "writing trajectory"),
+            Flush => write!(f, "flushing trajectory"),
+        }
+    }
+}
+
+/// Error codes returned from the C API
+#[derive(Debug, Clone, PartialEq, Copy)]
+pub enum ErrorCode {
+    /// No error, C API returned successfully
+    ExdrOk,
+    /// TRR file had corrupt Header
+    ExdrHeader,
+    /// Failed to read a string where expected
+    ExdrString,
+    /// Failed to read a double where expected
+    ExdrDouble,
+    /// Failed to read an int where expected
+    ExdrInt,
+    /// Failed to read a float where expected
+    ExdrFloat,
+    /// Failed to read a uint where expected
+    ExdrUint,
+    /// Failed to read compressed XTC coordinates
+    Exdr3dx,
+    /// Error encountered while closing file
+    ExdrClose,
+    /// File had incorrect "magic number" (not an XTC file)
+    ExdrMagic,
+    /// Failed to allocate memory for TRR file
+    ExdrNoMem,
+    /// End of file was reached while trying to read
+    ExdrEndOfFile,
+    /// File was not found when trying to open
+    ExdrFileNotFound,
+    /// Failed to seek within file
+    ExdrNr,
+    /// Something unexpected happened
+    UnmatchedCode(c_abi::xdrfile::BindgenTy1),
+}
+
+impl ErrorCode {
+    /// True if the error is an end of file error, false otherwise
+    pub fn is_eof(&self) -> bool {
+        matches!(self, Self::ExdrEndOfFile)
+    }
+}
+
+impl From<c_abi::xdrfile::BindgenTy1> for ErrorCode {
+    fn from(code: c_abi::xdrfile::BindgenTy1) -> Self {
+        match code {
+            c_abi::xdrfile::exdrOK => Self::ExdrOk,
+            c_abi::xdrfile::exdrHEADER => Self::ExdrHeader,
+            c_abi::xdrfile::exdrSTRING => Self::ExdrString,
+            c_abi::xdrfile::exdrDOUBLE => Self::ExdrDouble,
+            c_abi::xdrfile::exdrINT => Self::ExdrInt,
+            c_abi::xdrfile::exdrFLOAT => Self::ExdrFloat,
+            c_abi::xdrfile::exdrUINT => Self::ExdrUint,
+            c_abi::xdrfile::exdr3DX => Self::Exdr3dx,
+            c_abi::xdrfile::exdrCLOSE => Self::ExdrClose,
+            c_abi::xdrfile::exdrMAGIC => Self::ExdrMagic,
+            c_abi::xdrfile::exdrNOMEM => Self::ExdrNoMem,
+            c_abi::xdrfile::exdrENDOFFILE => Self::ExdrEndOfFile,
+            c_abi::xdrfile::exdrFILENOTFOUND => Self::ExdrFileNotFound,
+            c_abi::xdrfile::exdrNR => Self::ExdrNr,
+            code => Self::UnmatchedCode(code),
+        }
+    }
+}
+
+impl std::fmt::Display for ErrorCode {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        if let Self::UnmatchedCode(i) = self {
+            write!(f, "{}", i)
+        } else {
+            write!(f, "{:?}", self)
+        }
+    }
+}
+
+/// `Result` type for errors in the `xdrfile` crate
+pub type Result<T, E = Error> = std::result::Result<T, E>;
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::ffi::CString;
+    use std::ffi::NulError;
+
+    #[test]
+    fn test_is_eof() {
+        use Error::CApiError;
+        let error = CApiError {
+            code: c_abi::xdrfile::exdrENDOFFILE.into(),
+            task: ErrorTask::Read,
+        };
+        assert!(error.is_eof());
+
+        let error = CApiError {
+            code: ErrorCode::ExdrEndOfFile,
+            task: ErrorTask::Read,
+        };
+        assert!(error.is_eof());
+
+        let error = CApiError {
+            code: (c_abi::xdrfile::exdrENDOFFILE + 1).into(),
+            task: ErrorTask::Read,
+        };
+        assert!(!error.is_eof());
+
+        let error = CApiError {
+            code: 0.into(),
+            task: ErrorTask::Read,
+        };
+        assert!(!error.is_eof());
+
+        let error = CApiError {
+            code: 255.into(),
+            task: ErrorTask::Read,
+        };
+        assert!(!error.is_eof());
+
+        let error = Error::CouldNotOpen {
+            path: PathBuf::from("not/a/file"),
+            mode: FileMode::Read,
+        };
+        assert!(!error.is_eof());
+    }
+
+    #[test]
+    fn test_from_correct_type() {
+        let nul_err: NulError = CString::new(b"foo\0".to_vec()).unwrap_err();
+        let nul_err2: NulError = CString::new(b"foo\0".to_vec()).unwrap_err();
+        let err = Error::from(nul_err);
+        let expected = Error::NullInStr(nul_err2);
+        assert_eq!(expected, err);
+
+        let code = 3.into();
+        let task = ErrorTask::Read;
+        let expected = Error::CApiError { code, task };
+        let err = Error::from((code, task));
+        assert_eq!(expected, err);
+
+        let path = Path::new(".");
+        let mode = FileMode::Read;
+        let expected = Error::CouldNotOpen {
+            path: path.to_path_buf(),
+            mode: mode.to_owned(),
+        };
+        let err = Error::from((path, mode));
+        assert_eq!(expected, err);
+
+        let frame = Frame::with_capacity(0);
+        let expected = Error::WrongSizeFrame {
+            expected: 10,
+            found: 0,
+        };
+        let err = Error::from((&frame, 10));
+        assert_eq!(expected, err);
+    }
+}

src/iterator.rs

@@ -1,20 +1,25 @@
 use crate::*;
 use std::rc::Rc;
 
+fn into_iter_inner<T: Trajectory>(mut traj: T) -> TrajectoryIterator<T> {
+    let num_atoms = traj.get_num_atoms();
+    let frame = match &num_atoms {
+        Ok(num_atoms) => Frame::with_capacity(*num_atoms),
+        Err(_) => Frame::new(),
+    };
+    TrajectoryIterator {
+        trajectory: traj,
+        item: Rc::new(frame),
+        has_error: false,
+    }
+}
+
 impl IntoIterator for XTCTrajectory {
     type Item = Result<Rc<Frame>>;
     type IntoIter = TrajectoryIterator<XTCTrajectory>;
 
-    fn into_iter(mut self) -> Self::IntoIter {
-        let frame = match self.get_num_atoms() {
-            Ok(num_atoms) => Frame::with_capacity(num_atoms),
-            Err(_) => Frame::new(),
-        };
-        TrajectoryIterator {
-            trajectory: self,
-            item: Rc::new(frame),
-            has_error: false,
-        }
+    fn into_iter(self) -> Self::IntoIter {
+        into_iter_inner(self)
     }
 }
 
@@ -22,16 +27,8 @@ impl IntoIterator for TRRTrajectory {
     type Item = Result<Rc<Frame>>;
     type IntoIter = TrajectoryIterator<TRRTrajectory>;
 
-    fn into_iter(mut self) -> Self::IntoIter {
-        let frame = match self.get_num_atoms() {
-            Ok(num_atoms) => Frame::with_capacity(num_atoms),
-            Err(_) => Frame::new(),
-        };
-        TrajectoryIterator {
-            trajectory: self,
-            item: Rc::new(frame),
-            has_error: false,
-        }
+    fn into_iter(self) -> Self::IntoIter {
+        into_iter_inner(self)
     }
 }
 
@@ -46,6 +43,31 @@ pub struct TrajectoryIterator<T> {
     has_error: bool,
 }
 
+impl<T: Trajectory> TrajectoryIterator<T> {
+    /// Inner function for `next()`  to seperate error handling from iteration logic
+    fn next_inner(&mut self) -> <Self as Iterator>::Item {
+        // If we couldn't read the number of frames when we called into_iter, return that error now
+        // It's OK to do this every frame because the result is cached by Trajectory
+        let num_atoms = match &self.trajectory.get_num_atoms() {
+            &Ok(n) => n,
+            Err(e) => Err(Error::CouldNotCheckNAtoms(Box::new(e.clone())))?,
+        };
+
+        // Reuse old frame
+        let item: &mut Frame = match Rc::get_mut(&mut self.item) {
+            Some(item) => item,
+            None => {
+                // caller kept frame. Create new one
+                self.item = Rc::new(Frame::with_capacity(num_atoms));
+                Rc::get_mut(&mut self.item).expect("Could not get mutable access to new Rc")
+            }
+        };
+
+        self.trajectory.read(item)?;
+        Ok(Rc::clone(&self.item))
+    }
+}
+
 impl<T> Iterator for TrajectoryIterator<T>
 where
     T: Trajectory,
@@ -53,21 +75,12 @@ where
     type Item = Result<Rc<Frame>>;
 
     fn next(&mut self) -> Option<Self::Item> {
-        // Reuse old frame
         if self.has_error {
             return None;
         }
 
-        let item: &mut Frame = match Rc::get_mut(&mut self.item) {
-            Some(item) => item,
-            None => {
-                // caller kept frame. Create new one
-                self.item = Rc::new(Frame::with_capacity(self.item.num_atoms));
-                Rc::get_mut(&mut self.item).expect("Could not get mutable access to new Rc")
-            }
-        };
-        match self.trajectory.read(item) {
-            Ok(()) => Some(Ok(Rc::clone(&self.item))),
+        match self.next_inner() {
+            Ok(item) => Some(Ok(item)),
             Err(e) if e.is_eof() => None,
             Err(e) => {
                 self.has_error = true;

src/lib.rs

@@ -62,8 +62,10 @@ extern crate assert_approx_eq;
 extern crate lazy_init;
 
 pub mod c_abi;
+mod errors;
 mod frame;
 mod iterator;
+pub use errors::*;
 pub use frame::Frame;
 pub use iterator::*;
 
@@ -78,75 +80,6 @@ use std::cell::Cell;
 use std::ffi::CString;
 use std::path::{Path, PathBuf};
 
-#[derive(Debug, Clone, PartialEq)]
-pub enum Error {
-    CouldNotOpenFile(std::path::PathBuf, FileMode),
-    CouldNotReadAtomNumber(u32),
-    CouldNotRead(u32),
-    CouldNotWrite(u32),
-    CouldNotFlush(u32),
-    PathInvalidCstring,
-}
-
-impl std::fmt::Display for Error {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        use Error::*;
-        match self {
-            CouldNotOpenFile(path, mode) => write!(
-                f,
-                "Failed to open file at {path:?} with mode {mode:?}",
-                path = path,
-                mode = mode
-            ),
-            CouldNotReadAtomNumber(code) => write!(
-                f,
-                "Failed to read atom number from trajectory: C API returned error code {}",
-                code
-            ),
-            CouldNotRead(code) => write!(
-                f,
-                "Failed to read trajectory: C API returned error code {}",
-                code
-            ),
-            CouldNotWrite(code) => write!(
-                f,
-                "Failed to write trajectory: C API returned error code {}",
-                code
-            ),
-            CouldNotFlush(code) => write!(
-                f,
-                "Failed to flush trajectory: C API returned error code {}",
-                code
-            ),
-            PathInvalidCstring => write!(
-                f,
-                "Path cannot be converted to a C string because it has a 0 byte"
-            ),
-        }
-    }
-}
-
-impl Error {
-    pub fn is_eof(&self) -> bool {
-        use Error::*;
-        match self {
-            &CouldNotReadAtomNumber(code)
-            | &CouldNotRead(code)
-            | &CouldNotWrite(code)
-            | &CouldNotFlush(code)
-                if code == xdrfile::exdrENDOFFILE =>
-            {
-                true
-            }
-            _ => false,
-        }
-    }
-}
-
-impl std::error::Error for Error {}
-
-pub type Result<T, E = Error> = std::result::Result<T, E>;
-
 #[derive(Debug, Clone, PartialEq)]
 pub enum FileMode {
     Write,
@@ -168,8 +101,22 @@ impl FileMode {
 }
 
 fn path_to_cstring(path: impl AsRef<Path>) -> Result<CString> {
-    let s = path.as_ref().to_str().ok_or(Error::PathInvalidCstring)?;
-    CString::new(s).map_err(|_| Error::PathInvalidCstring)
+    let s = path.as_ref().to_str().ok_or(Error::InvalidOsStr)?;
+    Ok(CString::new(s)?)
+}
+
+/// Convert an error code from a C call to an Error
+///
+/// `code` should be an integer return code returned from the C API.
+/// If `code` indicates the function returned successfully, Nothing is returned;
+/// otherwise, the code is converted into the appropriate `Error`.
+pub fn check_code(code: impl Into<ErrorCode>, task: ErrorTask) -> Option<Error> {
+    let code: ErrorCode = code.into();
+    if let ErrorCode::ExdrOk = code {
+        None
+    } else {
+        Some(Error::from((code, task)))
+    }
 }
 
 /// A safe wrapper around the c implementation of an XDRFile
@@ -202,7 +149,7 @@ impl XDRFile {
                 })
             } else {
                 // Something went wrong. But the C api does not tell us what
-                Err(Error::CouldNotOpenFile(path.into(), filemode))
+                Err((path, filemode))?
             }
         }
     }
@@ -267,24 +214,33 @@ impl XTCTrajectory {
 
 impl Trajectory for XTCTrajectory {
     fn read(&mut self, frame: &mut Frame) -> Result<()> {
+        let mut step: i32 = 0;
+
+        let num_atoms =
+            self.get_num_atoms()
+                .map_err(|e| Error::CouldNotCheckNAtoms(Box::new(e)))? as usize;
+        if num_atoms != frame.coords.len() {
+            Err((&*frame, num_atoms))?;
+        }
+
         unsafe {
             // C lib requires an i32 to be passed, but step is exposed it as u32
             // (A step cannot be negative, can it?). So we need to create a step
             // variable to pass to read_xtc and cast it afterwards to u32
-            let mut step: i32 = 0;
             let code = xdrfile_xtc::read_xtc(
                 self.handle.xdrfile,
-                frame.num_atoms as i32,
+                num_atoms as i32,
                 &mut step,
                 &mut frame.time,
                 &mut frame.box_vector,
-                frame.coords.as_ptr() as *mut [f32; 3],
+                frame.coords.as_mut_ptr(),
                 &mut self.precision.get(),
             ) as u32;
             frame.step = step as u32;
-            match code {
-                xdrfile::exdrOK => Ok(()),
-                _ => Err(Error::CouldNotRead(code)),
+            if let Some(err) = check_code(code, ErrorTask::Read) {
+                Err(err)
+            } else {
+                Ok(())
             }
         }
     }
@@ -300,9 +256,10 @@ impl Trajectory for XTCTrajectory {
                 frame.coords[..].as_ptr() as *mut [f32; 3],
                 1000.0,
             ) as u32;
-            match code {
-                xdrfile::exdrOK => Ok(()),
-                _ => Err(Error::CouldNotWrite(code)),
+            if let Some(err) = check_code(code, ErrorTask::Write) {
+                Err(err)
+            } else {
+                Ok(())
             }
         }
     }
@@ -310,9 +267,10 @@ impl Trajectory for XTCTrajectory {
     fn flush(&mut self) -> Result<()> {
         unsafe {
             let code = xdr_seek::xdr_flush(self.handle.xdrfile) as u32;
-            match code {
-                xdrfile::exdrOK => Ok(()),
-                _ => Err(Error::CouldNotFlush(code)),
+            if let Some(err) = check_code(code, ErrorTask::Read) {
+                Err(err)
+            } else {
+                Ok(())
             }
         }
     }
@@ -321,6 +279,7 @@ impl Trajectory for XTCTrajectory {
         self.num_atoms
             .get_or_create(|| {
                 let mut num_atoms: i32 = 0;
+
                 unsafe {
                     let path = path_to_cstring(&self.handle.path)?;
                     let path_p = path.into_raw();
@@ -328,9 +287,11 @@ impl Trajectory for XTCTrajectory {
                         xdrfile_xtc::read_xtc_natoms(path_p, &mut num_atoms as *const i32) as u32;
                     // Reconstitute the CString so it is deallocated correctly
                     let _ = CString::from_raw(path_p);
-                    match code {
-                        xdrfile::exdrOK => Ok(num_atoms as u32),
-                        _ => Err(Error::CouldNotReadAtomNumber(code)),
+
+                    if let Some(err) = check_code(code, ErrorTask::ReadNumAtoms) {
+                        Err(err)
+                    } else {
+                        Ok(num_atoms as u32)
                     }
                 }
             })
@@ -371,28 +332,37 @@ impl TRRTrajectory {
 
 impl Trajectory for TRRTrajectory {
     fn read(&mut self, frame: &mut Frame) -> Result<()> {
+        let mut step: i32 = 0;
+        let mut lambda: f32 = 0.0;
+
+        let num_atoms =
+            self.get_num_atoms()
+                .map_err(|e| Error::CouldNotCheckNAtoms(Box::new(e)))? as usize;
+        if num_atoms != frame.coords.len() {
+            Err((&*frame, num_atoms))?;
+        }
+
         unsafe {
             // C lib requires an i32 to be passed, but step is exposed it as u32
             // (A step cannot be negative, can it?). So we need to create a step
             // variable to pass to read_trr and cast it afterwards to u32.
             // Similar for lambda.
-            let mut step: i32 = 0;
-            let mut lambda: f32 = 0.0;
             let code = xdrfile_trr::read_trr(
                 self.handle.xdrfile,
-                frame.num_atoms as i32,
+                num_atoms as i32,
                 &mut step,
                 &mut frame.time,
                 &mut lambda,
                 &mut frame.box_vector,
-                frame.coords.as_ptr() as *mut [f32; 3],
+                frame.coords.as_mut_ptr(),
                 std::ptr::null_mut(),
                 std::ptr::null_mut(),
             ) as u32;
             frame.step = step as u32;
-            match code {
-                xdrfile::exdrOK => Ok(()),
-                _ => Err(Error::CouldNotRead(code)),
+            if let Some(err) = check_code(code, ErrorTask::Read) {
+                Err(err)
+            } else {
+                Ok(())
             }
         }
     }
@@ -410,9 +380,10 @@ impl Trajectory for TRRTrajectory {
                 std::ptr::null_mut(),
                 std::ptr::null_mut(),
             ) as u32;
-            match code {
-                xdrfile::exdrOK => Ok(()),
-                _ => Err(Error::CouldNotWrite(code)),
+            if let Some(err) = check_code(code, ErrorTask::Write) {
+                Err(err)
+            } else {
+                Ok(())
             }
         }
     }
@@ -420,9 +391,10 @@ impl Trajectory for TRRTrajectory {
     fn flush(&mut self) -> Result<()> {
         unsafe {
             let code = xdr_seek::xdr_flush(self.handle.xdrfile) as u32;
-            match code {
-                xdrfile::exdrOK => Ok(()),
-                _ => Err(Error::CouldNotFlush(code)),
+            if let Some(err) = check_code(code, ErrorTask::Flush) {
+                Err(err)
+            } else {
+                Ok(())
             }
         }
     }
@@ -438,9 +410,11 @@ impl Trajectory for TRRTrajectory {
                         xdrfile_trr::read_trr_natoms(path_p, &mut num_atoms as *const i32) as u32;
                     // Reconstitute the CString so it is deallocated correctly
                     let _ = CString::from_raw(path_p);
-                    match code {
-                        xdrfile::exdrOK => Ok(num_atoms as u32),
-                        _ => Err(Error::CouldNotReadAtomNumber(code)),
+
+                    if let Some(err) = check_code(code, ErrorTask::ReadNumAtoms) {
+                        Err(err)
+                    } else {
+                        Ok(num_atoms as u32)
                     }
                 }
             })
@@ -535,28 +509,85 @@ mod tests {
     }
 
     #[test]
-    fn test_path_to_cstring() -> Result<(), Box<dyn std::error::Error>> {
-        let result_invalid = path_to_cstring("invalid/\0path");
+    pub fn test_manual_loop() -> Result<(), Box<dyn std::error::Error>> {
+        let mut xtc_frames = Vec::new();
+        let mut xtc_traj = XTCTrajectory::open_read("tests/1l2y.xtc")?;
+        let mut frame = Frame::with_capacity(xtc_traj.get_num_atoms()?);
 
-        assert_eq!(result_invalid, Err(Error::PathInvalidCstring));
+        while let Ok(()) = xtc_traj.read(&mut frame) {
+            xtc_frames.push(frame.clone());
+        }
+
+        let mut trr_frames = Vec::new();
+        let mut trr_traj = TRRTrajectory::open_read("tests/1l2y.trr")?;
+
+        while let Ok(()) = trr_traj.read(&mut frame) {
+            trr_frames.push(frame.clone());
+        }
+
+        for (xtc, trr) in xtc_frames.into_iter().zip(trr_frames) {
+            assert_eq!(xtc.num_atoms, trr.num_atoms);
+            assert_eq!(xtc.step, trr.step);
+            assert_eq!(xtc.time, trr.time);
+            assert_eq!(xtc.box_vector, trr.box_vector);
+            for (xtc_xyz, trr_xyz) in xtc.coords.into_iter().zip(trr.coords) {
+                assert!(xtc_xyz[0] - trr_xyz[0] <= 1e-5);
+                assert!(xtc_xyz[1] - trr_xyz[1] <= 1e-5);
+                assert!(xtc_xyz[2] - trr_xyz[2] <= 1e-5);
+            }
+        }
+        Ok(())
+    }
+
+    #[test]
+    pub fn test_wrong_size_frame() -> Result<(), Box<dyn std::error::Error>> {
+        let mut xtc_traj = XTCTrajectory::open_read("tests/1l2y.xtc")?;
+        let mut frame = Frame::new();
+
+        let result = xtc_traj.read(&mut frame);
+        if let Err(e) = result {
+            assert!(matches!(e, Error::WrongSizeFrame { .. }));
+        } else {
+            panic!("A read with an incorrectly sized frame should not succeed")
+        }
+        Ok(())
+    }
+
+    #[test]
+    fn test_path_to_cstring() -> Result<(), Box<dyn std::error::Error>> {
+        let result_invalid = path_to_cstring(PathBuf::from("invalid/\0path"));
+
+        if let Err(err) = result_invalid {
+            match err {
+                Error::NullInStr(_) => (),
+                Error::InvalidOsStr => (),
+                _ => panic!("Improper error type for path_to_cstring"),
+            }
+        } else {
+            panic!("path_to_cstring should return Err if there are null bytes");
+        }
 
         let result_valid = path_to_cstring("valid/path");
-
         assert_eq!(result_valid, Ok(CString::new("valid/path")?));
+
         Ok(())
     }
 
     #[test]
     fn test_err_could_not_open() {
         let file_name = "non-existent.xtc";
-        let expexted_path = Path::new(&file_name);
+
+        let path = Path::new(&file_name);
         if let Err(e) = XDRFile::open(file_name, FileMode::Read) {
-            match e {
-                Error::CouldNotOpenFile(err_path, err_mode) => {
-                    assert_eq!(expexted_path, err_path);
-                    assert!(FileMode::Read == err_mode)
-                }
-                _ => panic!("Wrong Error type"),
+            if let Error::CouldNotOpen {
+                path: err_path,
+                mode: err_mode,
+            } = e
+            {
+                assert_eq!(path, err_path);
+                assert_eq!(FileMode::Read, err_mode)
+            } else {
+                panic!("Wrong Error type")
             }
         }
     }
@@ -566,13 +597,10 @@ mod tests {
         let file_name = "README.md"; // not a trajectory
         let mut trr = TRRTrajectory::open_read(file_name)?;
         if let Err(e) = trr.get_num_atoms() {
-            match e {
-                Error::CouldNotReadAtomNumber(code) => {
-                    assert_eq!(xdrfile::exdrMAGIC, code);
+            assert_eq!(Some(ErrorCode::ExdrMagic), e.code());
+        } else {
+            panic!("Should not be able to read number of atoms from readme");
         }
-                _ => panic!("Wrong Error type"),
-            }
-        };
         Ok(())
     }
 
@@ -582,34 +610,13 @@ mod tests {
         let mut frame = Frame::with_capacity(1);
         let mut trr = TRRTrajectory::open_read(file_name)?;
         if let Err(e) = trr.read(&mut frame) {
-            match e {
-                Error::CouldNotRead(code) => {
-                    assert_eq!(xdrfile::exdrMAGIC, code);
-                }
-                _ => panic!("Wrong Error type"),
-            }
+            assert_eq!(Some(ErrorCode::ExdrMagic), e.code());
+        } else {
+            panic!("Should not be able to read number of atoms from readme");
         }
         Ok(())
     }
 
-    #[test]
-    fn test_err_eof() {
-        let error = Error::CouldNotRead(xdrfile::exdrENDOFFILE);
-        assert!(error.is_eof());
-
-        let error = Error::CouldNotRead(xdrfile::exdrENDOFFILE + 1);
-        assert!(!error.is_eof());
-
-        let error = Error::CouldNotWrite(0);
-        assert!(!error.is_eof());
-
-        let error = Error::CouldNotFlush(255);
-        assert!(!error.is_eof());
-
-        let error = Error::CouldNotOpenFile(PathBuf::from("not/a/file"), FileMode::Read);
-        assert!(!error.is_eof());
-    }
-
     #[test]
     fn test_err_file_eof() -> Result<(), Box<dyn std::error::Error>> {
         let tempfile = NamedTempFile::new()?;
@@ -654,4 +661,15 @@ mod tests {
 
         Ok(())
     }
+
+    #[test]
+    fn test_check_code() {
+        let code: ErrorCode = 0.into();
+        assert!(!check_code(code, ErrorTask::Read).is_some());
+
+        for i in vec![1, 10, 100, 1000] {
+            let code: ErrorCode = i.into();
+            assert!(check_code(code, ErrorTask::Read).is_some());
+        }
+    }
 }