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

Errors cannot be tested: ErrorKind implements neither Debug nor PartialEq #102

Closed
BenWiederhake opened this issue Feb 20, 2024 · 3 comments · Fixed by #109
Closed

Errors cannot be tested: ErrorKind implements neither Debug nor PartialEq #102

BenWiederhake opened this issue Feb 20, 2024 · 3 comments · Fixed by #109

Comments

@BenWiederhake
Copy link
Contributor

The tests currently do not check negative inputs, and would not detect if the library is buggy by accepting too much, e.g. basically never throwing an error. This is because doing so is not really feasible: The "obvious" way to test error scenarios is to assert that the returned error equals some expected error. However, that cannot work, because ErrorKind implements neither Debug nor PartialEq.

Implementing Debug is an easy fix:

diff --git a/src/error.rs b/src/error.rs
index 0da36e5..18c9d27 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -66,7 +66,7 @@ impl StdError for Error {}
 
 impl Display for Error {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        self.kind.fmt(f)
+        std::fmt::Display::fmt(&self.kind, f)
     }
 }
 
@@ -139,6 +139,12 @@ impl Display for ErrorKind {
     }
 }
 
+impl Debug for ErrorKind {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        std::fmt::Display::fmt(self, f)
+    }
+}
+
 impl From<lexopt::Error> for ErrorKind {
     fn from(other: lexopt::Error) -> ErrorKind {
         match other {

Implementing PartialEq is impossible however, because of ParsingFailed { … dyn StdError } and IoError(std::io::Error). Note that [std::io::ErrorKind](https://doc.rust-lang.org/std/io/enum.ErrorKind.html#impl-PartialEq-for-ErrorKind) and [std::num::IntErrorKind](https://doc.rust-lang.org/std/num/enum.IntErrorKind.html#impl-PartialEq-for-IntErrorKind) already implement PartialEq.

I have no good idea how to approach this, only bad ones:

  • Alternate enum: A new method fn ErrorKind::to_pure(&self) -> PureErrorKind;, which converts the current ErrorKind to a dataless enum PureErrorKind which has PartialEq.
  • Hardcode the error message in the tests: I hate it. It would work though.
  • Somehow moving the data to Error, making it easy for the current ErrorKind to implement PartialEq.
@tertsdiepraam
Copy link
Member

Good observation. I agree that the error should implement Debug, not just for tests, but also to be able to implement std::error::Error. For the tests, errors can still be matched on instead of tested for equality. Something like:

assert!(matches!(parse(...), Err(...)));

@tertsdiepraam
Copy link
Member

tertsdiepraam commented Feb 20, 2024

Or maybe we can impose Box<dyn StdError + Send + Sync + PartialEq + 'static> on ParsingFailed? I'd have to check whether the std parsing error implements that.

@tertsdiepraam
Copy link
Member

The std types do have PartialEq as far as I can tell, but I think it's too restrictive to impose. I'll make a PR for Debug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants