[Feat] Make Throwable Equatable

[jubishop]

Problem Statement

If they're equatable I can use the #expect(throws macro in testing.

Suggested Solution

Define equatable as just having matching userFriendlyStrings ?

Additional Considerations

Obviously this isn't perfect equatability but it'd be a reasonable default.

[Jeehut]

@jubishop Thank you for this suggestion. Before I can make a decision, can you please provide a full real-world example of how you would like to use this? I don't quite understand yet what exactly you're testing, the function itself or the return of a specific error type? Also, since you have a specific use case in mind, I'd like to have that as unit tests, so an example would help a lot.

Having that said, I'm also thinking about if maybe the the ErrorKit.errorChainDescription(for:) function isn't a better fit for the value to be returned? It would provide more detail about the error type and case than a userFriendlyMessage contains.

Maybe we should even strip away the userFriendlyMessage as that could potentially contain dynamic data. While localization probably doesn't matter for unit testing, I can imagine some errors having some fluctuating data, such as when users are supposed to see one of multiple fun error descriptions for some common issue (like no network). Generally, user-facing API should not be used as part of unit tests if we have some more structural data, which we have.

So, for example, a description like the following:

DatabaseError
└─ FileError.notFound(path: "/Users/jane/backup.db")
   └─ userFriendlyMessage: "Database file missing."

Would be stripped to something like:

DatabaseError
└─ FileError.notFound(path: "/Users/jane/backup.db")

And that would be the value we compare for Equatable. What do you think? Does that make sense?

One thing I'm not 100% sure about is if we should keep the parameters, as they could be dynamic, too. Potentially, we could even strip that away, but then two errors stating "file not found" with different paths would be considered the same error, which they aren't in most cases. I tend towards keeping the parameters but stripping the user-friendly message.

[jubishop]

using the error chain description is a good idea and I just migrated my use case to that. so here's the protocol I have all my errors conform to:

protocol KittedError: Throwable, Catching, Equatable {}

extension KittedError {
  static func == (_ lhs: Self, _ rhs: Self) -> Bool {
    ErrorKit.errorChainDescription(for: lhs) == ErrorKit.errorChainDescription(for: lhs)
  }
}

I don't have any strong opinion on your other questions, I can see how this isn't super straightforward with an obviously correct answer: I just think it'd be nice to get a reasonable Equatable built-in that works for most scenarios. Here's some exact code from one of my own tests, wherein DownloadError looks like this:

enum DownloadError: KittedError {
  case cancelled(URL)
  case loadFailure(URL)
  case caught(Error)

  var userFriendlyMessage: String {
    switch self {
    case .cancelled(let url):
      return "Cancelled load of \(url)"
    case .loadFailure(let url):
      return "Failed to load \(url)"
    case .caught(let error):
      return ErrorKit.userFriendlyMessage(for: error)
    }
  }
}

and the test is:

  @Test("that you can cancel a mid-flight download")
  func cancelActiveDownload() async throws {
    let downloadManager = DownloadManager(session: session)

    let url = URL.valid()
    await session.set(url, .delay(.milliseconds(50)))
    let task = await downloadManager.addURL(url)
    Task {
      try await Task.sleep(for: .milliseconds(10))
      await task.cancel()
    }

    await #expect(throws: DownloadError.cancelled(url).self) {
      try await task.downloadFinished()
    }

    // Even after the url data has returned, the result remains cancelled.
    try await Task.sleep(for: .milliseconds(100))
    await #expect(throws: DownloadError.cancelled(url).self) {
      try await task.downloadFinished()
    }
  }

a more isolated example for a test, once your library was updated to inherently support Equatable, might be (untested code just written into comment here):

enum MathError: Throwable, Catching {
  case negative(Int)
  case caught

  var userFriendlyMessage: String {
    switch self {
    case .negative(let number):
      return "\(number) must be positive"
    case .caught(let error):
      return ErrorKit.userFriendlyMessage(for: error)
    }
  }
}

func mySquareRoot(_ number: Int) throws(MathError) -> Double {
  if number < 0 {
    throw .negative(number)
  }

  return sqrt(Double(number))
}

@Test("that negative numbers in a square root calculation throws an error")
func testNegativeSquareRoot() async throws {
  #expect(throws: MathError.negative(-1).self) {
    try mySquareRoot(-1)
  }
}

fundamentally these #expect calls only work if the error type is Equatable.

ps thanks for this library its great!

[Jeehut]

@jubishop Thank you for the additional details and sample code! I'll give this a little more thought to consider potential consequences and side effects of adding automatic Equatable conformance. Like what if a developer decides that they want to make their error type conform to Equatable for other reasons? What reasons could those even be?

In the meantime, you can just conform your errors to Equatable manually like you've demonstrated above.

[jubishop]

A user might want to use == in a catch block to check for exactly what was thrown, beyond the type. Perhaps it is best to leave it out of the library. Cheers.

[jubishop]

small side note: I just realized the .self isn't needed on the error instances in the testing checks, duh.

[Jeehut]

@jubishop Thank you for your new inputs. I agree that for the time being, it's probably better to not make Throwable give all error types a magical Equatable conformance. It might actually confuse some people and I feel like your use case is best solved by adding Equatable conformance yourself. You could even define yourself a protocol like ThrowableEquatable (or whatever you want to name it) that automatically adds Equatable support and use that instead of Throwable.

I'm closing this for now as you agreed yourself that it's perhaps best to leave it out of the lib. But if people come across this in the future and have other use cases, feel free to open a new issue so we can discuss again.