Cannot disable test failure behaviour

[lukeredpath]

The library automatically causes tests to fail when reportIssue is called, which is not always desirable behaviour and there is no easy way to stop this from happening.

I'm in the process of migrating our custom error handling code over to using swift-issue-reporting. Our app uses TCA. I already have a number of tests where I'm explicitly configuring a dependency to throw an error to test the error handling path - even if we have explicit error handling logic we might still use reportIssue to log that issue (to Sentry, using a custom reporter). There is no need for this to trigger a test failure.

I expected that this behaviour was just a custom "test" reporter but it isn't, it is hardcoded into reportIssue. I cannot use withExpectedIssue because the async version does not support XCTest - I cannot change over to Swift testing just to work around this issue. I can use XCTExpectFailure() directly but this is a very broad solution that could hide genuine test failures.

I think this behaviour should be configurable, or refactored into a default issue reporter that can be overridden using the existing API for configuring reporters.

[lukeredpath]

Expanded example of why this isn't always helpful:

We have an effect that performs some work and if an error is thrown, we catch it and send a different action back to the store. However it is still useful for reporting purposes to report this issue. A simplified example of what this might look like:

return .run { send in
  do {
    try await client.somethingImporant()
    await send(.somethingWorked)
  } catch {
    reportIssue(error)
    await send(.somethingFailed)
  }
}

If I'm testing the failure flow, I might override the client dependency to throw an error to test that the error is caught and I receive the .somethingFailed action. I don't particularly care in my test that the error was reported.

let store = TestStore(...)
store.dependencies.client.somethingImportant = { throw SomeError() }

// this action returns the effect above
await store.send(.task)
await store.receive(.somethingFailed)

The above test fails due to the unhandled reportIssue call. This example is a heavily simplified version of an actual test.

1. I don't want to use XCTExpectFailure here because I don't actually think the test is failing and this might mask a real failure.
2. I can't wrap this code with withExpectedIssue because the async version of this only works with Swift Testing.

I just want to disable the exhaustive asserting against reported issues, its just not important to this test.

Edit to say: on Stephen's suggestion I tried using the issue matcher for XCTExpectFailure:

XCTExpectFailure {
   $0.compactDescription.hasPrefix("failed - Caught error:")
}

This sort of works around the issue, but I don't like that I lose my green "test passed" tick and have to settle for lots of grey X status indicators across my test suite for tests that I consider to be a pass.

[RuiAAPeres]

Leaving a comment, since I am facing the exact situation with my tests (with Swift Testing):

The library automatically causes tests to fail when reportIssue is called, which is not always desirable behaviour and there is no easy way to stop this from happening.

[mbrandonw]

We don't really consider this an issue with the library as it stands today because the point of the library is to report issues that are testable. If you are reporting issues that are not programmer error / things to be fixed, then it may be the case that reportIssue is not the correct tool.

We will discuss more today and come back with any feedback or questions.

But, since this isn't an issue with the library I am going to convert it to a discussion. Please feel free to continue the conversation over there!

[mbrandonw]

I'm not sure I understand why this library needs to be treated as a thing that is just there to report errors during development.

This is just the original purpose of the library and current design of it. We are currently prioritizing this library as a means to report programmer errors that are visible in tests over it being a general purpose issue reporting library for reporting to any external service.

There's a PR open (#146) that offers one way of doing this in a fairly unobtrusive way. Its there for people who need it, but doesn't have any impact on those who don't have a use for it.

But as @RuiAAPeres showed above, can't this be if !isTesting { … } instead?

[lukeredpath]

It seems pretty intrusive to have to litter my code with this everywhere. We use reportIssue extensively.

[mbrandonw]

But your PR is proposing to use withEmitsFailureOnReportIssue(false) { … } everywhere, which seems even more intrusive. And you could also define your own reportIssue that bakes the if !isTesting check into it.

[lukeredpath]

It wouldn’t have to be used everywhere, only around the tests that are testing these particular flows. Most of the time I want reportIssue to to cause a test failure. There’s just times when I don’t and unfortunately XCTExpectFailure is clunky and means I lose my green tick, which irritates me as it should be deemed a passing test. Wrapping reportIssue in my own function that checks isTesting is not the right place - I want to make the decision to cause a test failure or not to be on a per test basis.

If this library is intended for a specific use case then I’m not really sure why it even supports the concept of custom reporters. Sending errors to an external service seems like exactly the sort of thing you’d use a custom reporter for, so I feel like I must be missing something.

[mbrandonw]

It wouldn’t have to be used everywhere, only around the tests that are testing these particular flow

That seems dangerous to me, and a pattern I feel should be discouraged if we were to have this tool. It would blanket disable all test failures for issue reporting. It seems best to surgically apply this tool for the exact issues you want to report externally but that you don't care for testing.

If this library is intended for a specific use case then I’m not really sure why it even supports the concept of custom reporters. Sending errors to an external service seems like exactly the sort of thing you’d use a custom reporter for, so I feel like I must be missing something.

While we are glad people are finding uses for custom reporters, the use cases we had in mind were printing to the logs, purple runtime warnings, and breakpoints. I'm not saying that we can't evolve the tool, but I am describing why it is the way it is, and it really does seem to me that even in its current form it suites your needs. It seems best to me that you would want to localize skipping test failures on a per-reportIssue basis rather than doing it globally.

But, if you do want this global behavior, you can always support it outside of the library. You can have a @TaskLocal in your project for disabling test failures and then have a custom reportIssue that checks it under the hood.