Refactor Result and Exception classes.

[zainab-ali]

This PR refactors the Result datatype to make it easier to correlate it to different test scenarios.

The Result datatype is now one of:

• Success, meaning the test passed.
• Failures, corresponding to invalid expect statements.
• Exception, meaning an exception was thrown by user code.
• Ignored, meaning the test was ignored.
• OnlyTagNotAllowedInCI, meaning the .only tag was used in CI.

Failures vs Failure

Invalid expectations are equivalent to Failures. Each individual Failure corresponds to an expect, expect.same, or expect.eql statement. For example, expect(1 == 2) and expect.same(3, 4) would result in a non-empty list of two Failure values.

The Failure itself isn't a subclass of Result, since it only ever appears as part of a collection of Failures.

OnlyTagNotAllowedInCI

There is a specific class for OnlyTagNotAllowedInCI. It was previously the only Failure created from something other than expectations, so deserves its own class. Unlike failures originating from expectations, it doesn't have an underlying exception.

Removal of WeaverException

I assume that noone is using the abstract WeaverException class.

This has been removed, along with the OurException extractors and pattern match for creating Result.Exception. Result.Exception now only corresponds to an exception thrown by the user.

IgnoredException made private

The IgnoredException is raised by ignore. It shouldn't be raised by users. By making it private, we can also make the reason field non-optional and remove the cause field.

cancel function removed

The cancel function has been removed in favour of ignore. They both achieve the same effect of skipping the test without marking it as a success or failure.

Binary compatibility

This PR will break binary compatibility. Before merging, we should make the Result type private such that this won't happen again.

[zainab-ali]

This scaladoc was incorrect.

[zainab-ali]

I assume that AssertionException isn't meant to be part of the public API.

If users want to create their own AssertionException, they can do so using the failure expectation and fail using failFast.

[Baccata]

Actually, I think we need to put more thought into it : userland should have the ability to recover from throwables resulting from failed assertions to create some combinators for things like "I want this test to eventually pass".

[zainab-ali]

It does need more thought. The AssertionException type is also part of the public API in Expectations:

case class Expectations(run: ValidatedNel[AssertionException, Unit])

What do you think about making the type public, but the message and locations fields private?

Here's an example of a userland retry function using a public AssertionException type:

  def retryThreeTimes(expectationIO: IO[Unit]): IO[Expectations] = {
    fs2.Stream
      .repeatEval(expectationIO.attemptNarrow[AssertionException])
      .takeWhile({
                   case Left(_) => true
                   case Right(_) => false
                 },
                 takeFailure = true
      )        // Repeat until the assertion is successful
      .take(3) // Repeat at most three times
      .scan(success)({
        case (_, Right(_)) =>
          success
          // If an expectation succeeds, the retry operation is successful
        case (failedExpectations, Left(assertionException)) =>
          failedExpectations.and(Expectations(Validated.invalidNel(assertionException)))
        // Accumulate the failures into a single expectation
      })
      .compile
      .lastOrError
  }

The gist of it is the ability to pattern match on AssertionException as a type, but not using unapply, and reconstruct the failed expectations with Expectations(Validated.invalidNel(assertionException)).

We could expose message and locations too, but I'm not convinced they would be helpful for users.

[Baccata]

What do you think about making the type public, but the message and locations fields private?

Agreed. While we're at it making breaking changes, we may as well rename it, I was never really happy with AssertionException.

We could expose message and locations too, but I'm not convinced they would be helpful for users.

Neither am I.

[zainab-ali]

Do you have any suggestions for the new name?

We're using the term assertion and expectations interchangably in the docs and internals, however the public API now only refers to Expectations and expect. We could prefer the term expectation instead:

• ExpectationFailedException
• ExpectationFailed
• ExpectationException

[Baccata]

ExpectationFailed sounds good to me

[Baccata]

are we removing this altogether ?

[zainab-ali]

WEAVER_MAX_STACKFRAMES is alse used in the Formatter, although I haven't checked if the code path using it is actually hit.

We're removing maxStackFrames because the Exception class never has a defined location, and so the stackTraceLimit would always be None.

If you're reviewing on GitHub, this is easier to see in the split diff view.

[Baccata]

👍

[zainab-ali]

The indent function has been removed, and its usages have been replaced with formatDescription.

The functions were practically identical. The only difference was the treatment of the prefix.

• indent would assume the prefix was whitespace, and so not add it for empty lines.
• formatDescription would always add the prefix

This change rewrites formatDescription to test if the prefix is whitespace and line is empty, and if so not add the prefix.

[zainab-ali]

I walked through the code for formatError for this case and found it was exactly the same as formatDescription.

I think we can reserve formatError for the Exceptioncase, where were have a stack trace we want to render.

[zainab-ali]

I'm itching to rename this to Canceled to align with cats-effect, but will leave that to a follow-up PR. There are several other datatypes which have Cancelled vs Canceled.

[Baccata]

In all frankness this is just mirroring the SBT test interface, but I don't think there's a meaningful difference beween ignored and canceled, so you could just remove it and dodge the question of which spelling should be applied 😄

[zainab-ali]

Reading the test interface docs it seems that each test framework has their own interpretation. I think we could remove the cancel function, and the idea of cancelled tests, but would rather leave that to a separate PR.

EDIT: On second thought, it may be best to do it in this PR. That way, we'd ensure it was in the same minor version bump.

[zainab-ali]

What do you think about moving this to Expectations.Helpers, or some other object?

Currently, users can only call ignore from within a suite, but they may want to do so in a helper function. For example:

object EnvironmentHelpers {
  import Expectations.Helpers.* // or some other object
  def ignoreInDev: IO[Unit] = ignore("Skipping test in DEV environment").whenA(isDev)
}

[Baccata]

yup I'm fine with it

[Baccata]

Good job 👍

Feel free to move the ignore after your suggestion and consider this approval still valid