Skip to content

Refactor Result and Exception classes. #192

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.

Sign up for GitHub

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

Merged
zainab-ali merged 10 commits into from
Sep 5, 2025
Merged

Refactor Result and Exception classes. #192

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6cc6c0c
Refactor Result and Exception classes.
zainab-ali Sep 1, 2025
38e3ee1
Bump base version.
zainab-ali Sep 1, 2025
da565c1
Add private modifiers.
zainab-ali Sep 2, 2025
d71998d
Only pass in traceLimit if failure has an underlying exception.
zainab-ali Sep 2, 2025
70fd516
Remove cause from WeaverTestException.
zainab-ali Sep 2, 2025
e9c34a4
Remove reason option from Ignored and Canceled exceptions.
zainab-ali Sep 2, 2025
65a1a2d
AssertionException type should be public.
zainab-ali Sep 3, 2025
699275d
Rename AssertionException to ExpectationFailed.
zainab-ali Sep 3, 2025
6eac8c5
Remove canceled test state.
zainab-ali Sep 4, 2025
204a577
Move ignore to Expectations.Helpers
zainab-ali Sep 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import laika.config.LinkValidation
import org.typelevel.sbt.site.TypelevelSiteSettings
import sbt.librarymanagement.Configurations.ScalaDocTool
// https://typelevel.org/sbt-typelevel/faq.html#what-is-a-base-version-anyway
ThisBuild / tlBaseVersion := "0.10" // your current series x.y
ThisBuild / tlBaseVersion := "0.11" // your current series x.y

ThisBuild / startYear := Some(2019)
ThisBuild / licenses := Seq(License.Apache2)
Expand Down
6 changes: 3 additions & 3 deletions modules/core/shared/src/main/scala/weaver/Comparison.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ import munit.diff.Diffs
* A type class used to compare two instances of the same type and construct an
* informative report.
*
* If the comparison succeeds with [[Result.Success]] then no report is printed.
* If the comparison fails with [[Result.Failure]], then the report is printed
* with the test failure.
* If the comparison succeeds with [[Comparison.Result.Success]] then no report

@zainab-ali zainab-ali Sep 2, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This scaladoc was incorrect.

* is printed. If the comparison fails with [[Comparison.Result.Failure]], then
* the report is printed with the test failure.
*
* The report is generally a diff of the `expected` and `found` values. It may
* use ANSI escape codes to add color.
Expand Down
2 changes: 1 addition & 1 deletion modules/core/shared/src/main/scala/weaver/Expectations.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ case class Expectations(run: ValidatedNel[AssertionException, Unit]) {
*/
def traced(loc: SourceLocation): Expectations =
Expectations(run.leftMap(_.map(e =>
e.copy(locations = e.locations.append(loc)))))
e.withLocation(loc))))

}

Expand Down
10 changes: 6 additions & 4 deletions modules/core/shared/src/main/scala/weaver/Formatter.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,13 @@ object Formatter {
import Result._

result match {
case Success => withPrefix(green("+ "))
case _: Failure | _: Failures | _: Exception => withPrefix(red("- "))
case _: Cancelled =>
case Success => withPrefix(green("+ "))
case OnlyTagNotAllowedInCI(_) | Failures(_) | Exception(_) =>
withPrefix(red("- "))
case Cancelled(_, _) =>
withPrefix(yellow("- ")) + yellow(" !!! CANCELLED !!!")
case _: Ignored => withPrefix(yellow("- ")) + yellow(" !!! IGNORED !!!")
case Ignored(_, _) =>
withPrefix(yellow("- ")) + yellow(" !!! IGNORED !!!")
}
}

Expand Down
68 changes: 36 additions & 32 deletions modules/core/shared/src/main/scala/weaver/Result.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,20 @@
package weaver

import scala.util.Try

import cats.data.NonEmptyList
import cats.data.Validated.{ Invalid, Valid }

sealed trait Result {
private[weaver] sealed trait Result {
def formatted: Option[String]
}

object Result {
private[weaver] object Result {
import Formatter._

def fromAssertion(assertion: Expectations): Result = assertion.run match {
case Valid(_) => Success
case Invalid(failed) =>
Failures(failed.map(ex =>
Result.Failure(ex.message, Some(ex), ex.locations.toList)))
Failures.Failure(ex.message, ex, ex.locations)))
}

case object Success extends Result {
Expand All @@ -39,19 +37,25 @@ object Result {
}
}

final case class Failures(failures: NonEmptyList[Failure]) extends Result {
final case class Failures(failures: NonEmptyList[Failures.Failure])
extends Result {

def formatted: Option[String] =
if (failures.size == 1) failures.head.formatted
else {
if (failures.size == 1) {
val failure = failures.head
Some(formatError(failure.msg,
Some(failure.source),
failure.locations.toList,
Some(0)))
} else {

val descriptions = failures.zipWithIndex.map {
case (failure, idx) =>
import failure._

formatDescription(
if (msg != null && msg.nonEmpty) msg else "Test failed",
location,
locations.toList,
Console.RED,
s" [$idx] "
)
Expand All @@ -61,20 +65,25 @@ object Result {
}
}

final case class Failure(

@Baccata Baccata Sep 3, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

msg: String,
source: Option[Throwable],
location: List[SourceLocation])
object Failures {
final case class Failure(
msg: String,
source: Throwable,
locations: NonEmptyList[SourceLocation])
}

final case class OnlyTagNotAllowedInCI(
location: SourceLocation)
extends Result {

def formatted: Option[String] =
Some(formatError(msg, source, location, Some(0)))

@zainab-ali zainab-ali Sep 3, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Some(formatError("'Only' tag is not allowed when `isCI=true`",
None,
List(location),
Some(0)))
}

final case class Exception(
source: Throwable,
location: Option[SourceLocation])
extends Result {
final case class Exception(source: Throwable) extends Result {

def formatted: Option[String] = {
val description = {
Expand All @@ -85,16 +94,10 @@ object Result {
.fold(className)(m => s"$className: $m")
}

val maxStackFrames = sys.props.get("WEAVER_MAX_STACKFRAMES").flatMap(s =>

@Baccata Baccata Sep 3, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we removing this altogether ?

@zainab-ali zainab-ali Sep 3, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Try(s.trim.toInt).toOption).getOrElse(50)

val stackTraceLimit =
if (location.isDefined) Some(maxStackFrames) else None

Some(formatError(description,
Some(source),
location.toList,
stackTraceLimit))
Nil,
None))
}
}

Expand All @@ -103,15 +106,16 @@ object Result {
def from(error: Throwable): Result = {
error match {
case ex: AssertionException =>
Result.Failure(ex.message, Some(ex), ex.locations.toList)
Failures(NonEmptyList.of(Failures.Failure(
ex.message,
ex,
ex.locations)))
case ex: IgnoredException =>
Result.Ignored(ex.reason, ex.location)
Ignored(ex.reason, ex.location)
case ex: CanceledException =>
Result.Cancelled(ex.reason, ex.location)
case ex: WeaverException =>
Result.Exception(ex, Some(ex.getLocation))
Cancelled(ex.reason, ex.location)
case other =>
Result.Exception(other, None)
Exception(other)
}
}

Expand Down
23 changes: 11 additions & 12 deletions modules/core/shared/src/main/scala/weaver/TestOutcome.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,21 +35,20 @@ object TestOutcome {
extends TestOutcome {

def status: TestStatus = result match {
case Result.Success => TestStatus.Success
case Result.Cancelled(_, _) => TestStatus.Cancelled
case Result.Ignored(_, _) => TestStatus.Ignored
case Result.Failure(_, _, _) | Result.Failures(_) => TestStatus.Failure
case Result.Exception(_, _) => TestStatus.Exception
case Result.Success => TestStatus.Success
case Result.Cancelled(_, _) => TestStatus.Cancelled
case Result.Ignored(_, _) => TestStatus.Ignored
case Result.OnlyTagNotAllowedInCI(_) | Result.Failures(_) =>
TestStatus.Failure
case Result.Exception(_) => TestStatus.Exception
}

def cause: Option[Throwable] = result match {
case Result.Exception(cause, _) => Some(cause)
case Result.Failure(_, maybeCause, _) => maybeCause
case Result.Failures(failures) =>
failures.collectFirst {
case Result.Failure(_, Some(cause), _) => cause
}
case _ => None
case Result.Exception(cause) => Some(cause)
case Result.Failures(failures) => Some(failures.head.source)
case Result.OnlyTagNotAllowedInCI(_) | Result.Cancelled(
_,
_) | Result.Ignored(_, _) | Result.Success => None
}

def formatted(mode: Mode): String =
Expand Down
75 changes: 19 additions & 56 deletions modules/core/shared/src/main/scala/weaver/exceptions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,64 +1,27 @@
package weaver

import scala.util.control.NonFatal

import cats.data.NonEmptyList

abstract class WeaverException(
private[weaver] sealed abstract class WeaverTestException(
message: String,
cause: Option[Throwable],
location: SourceLocation)
extends RuntimeException(message, cause.orNull) {

def getLocation: SourceLocation = location

cause: Option[Throwable]
) extends RuntimeException(message, cause.orNull)

private[weaver] final class AssertionException(
private[weaver] val message: String,
private[weaver] val locations: NonEmptyList[SourceLocation])
extends WeaverTestException(message, None) {
private[weaver] def withLocation(
location: SourceLocation): AssertionException =
new AssertionException(message, locations.append(location))

@zainab-ali zainab-ali Sep 2, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Baccata Sep 3, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 zainab-ali Sep 3, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Baccata Sep 3, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 zainab-ali Sep 3, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Baccata Sep 3, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExpectationFailed sounds good to me

}

sealed abstract class WeaverTestException(
message: String,
cause: Option[Throwable],
location: SourceLocation)
extends WeaverException(message, cause, location)

final case class AssertionException(
message: String,
locations: NonEmptyList[SourceLocation])
extends WeaverTestException(message, None, locations.head)

final class IgnoredException(
val reason: Option[String],
val location: SourceLocation)
extends WeaverTestException(reason.orNull, None, location)
private[weaver] final class IgnoredException(
private[weaver] val reason: Option[String],
private[weaver] val location: SourceLocation)
extends WeaverTestException(reason.orNull, None)

final class CanceledException(
val reason: Option[String],
val location: SourceLocation)
extends WeaverTestException(reason.orNull, None, location)

object OurException {

/**
* Utility for pattern matching.
*/
def unapply(ex: Throwable): Option[WeaverException] = ex match {
case ref: WeaverTestException =>
Some(ref)
case _ =>
None
}
}

object NotOurException {

/**
* Utility for pattern matching.
*/
def unapply(ex: Throwable) = ex match {
case OurException(_) =>
None
case NonFatal(_) =>
Some(ex)
case _ =>
None
}
}
private[weaver] final class CanceledException(
private[weaver] val reason: Option[String],
private[weaver] val location: SourceLocation)
extends WeaverTestException(reason.orNull, None)
6 changes: 1 addition & 5 deletions modules/core/shared/src/main/scala/weaver/suites.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,7 @@ abstract class RunnableSuite[F[_]] extends EffectSuite[F] {


private[this] def onlyNotOnCiFailure(test: TestName): TestOutcome = {
val result = Result.Failure(
msg = "'Only' tag is not allowed when `isCI=true`",
source = None,
location = List(test.location)
)
val result = Result.OnlyTagNotAllowedInCI(location = test.location)
TestOutcome(
name = test.name,
duration = FiniteDuration(0, "ns"),
Expand Down