-
-
Notifications
You must be signed in to change notification settings - Fork 15
Add source location URL to CI test failures. #311
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,22 +10,29 @@ private[weaver] sealed trait 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 => | ||
| Failures.Failure(ex.message, ex, ex.locations))) | ||
| } | ||
| def fromAssertion( | ||
| sourceLocationUrl: Option[String], | ||
| assertion: Expectations): Result = | ||
| assertion.run match { | ||
| case Valid(_) => Success | ||
| case Invalid(failed) => | ||
| Failures(failed.map(ex => | ||
| Failures.Failure(ex.message, ex, sourceLocationUrl, ex.locations))) | ||
| } | ||
|
|
||
| case object Success extends Result { | ||
| def formatted: Option[String] = None | ||
| } | ||
|
|
||
| final case class Ignored(reason: String, location: SourceLocation) | ||
| final case class Ignored( | ||
| reason: String, | ||
| sourceLocationUrl: Option[String], | ||
| location: SourceLocation) | ||
| extends Result { | ||
|
|
||
| def formatted: Option[String] = { | ||
| Some(formatDescription(reason, | ||
| sourceLocationUrl = sourceLocationUrl, | ||
| List(location), | ||
| Console.YELLOW, | ||
| TAB2.prefix)) | ||
|
|
@@ -40,6 +47,7 @@ private[weaver] object Result { | |
| val failure = failures.head | ||
| val formattedMessage = formatDescription( | ||
| failure.msg, | ||
| failures.head.sourceLocationUrl, | ||
| failure.locations.toList, | ||
| Console.RED, | ||
| TAB2.prefix | ||
|
|
@@ -53,6 +61,7 @@ private[weaver] object Result { | |
|
|
||
| formatDescription( | ||
| msg, | ||
| sourceLocationUrl, | ||
| locations.toList, | ||
| Console.RED, | ||
| s" [$idx] " | ||
|
|
@@ -67,6 +76,7 @@ private[weaver] object Result { | |
| final case class Failure( | ||
| msg: String, | ||
| source: ExpectationFailed, | ||
| sourceLocationUrl: Option[String], | ||
| locations: NonEmptyList[SourceLocation]) | ||
| } | ||
|
|
||
|
|
@@ -77,6 +87,7 @@ private[weaver] object Result { | |
| def formatted: Option[String] = { | ||
| val formattedMessage = formatDescription( | ||
| "'Only' tag is not allowed when `isCI=true`", | ||
| None, | ||
| List(location), | ||
| Console.RED, | ||
| TAB2.prefix | ||
|
|
@@ -102,13 +113,13 @@ private[weaver] object Result { | |
|
|
||
| val success: Result = Success | ||
|
|
||
| def from(error: Throwable): Result = { | ||
| def from(sourceLocationUrl: Option[String], error: Throwable): Result = { | ||
| error match { | ||
| case ex: IgnoredException => | ||
| Ignored(ex.reason, ex.location) | ||
| Ignored(ex.reason, sourceLocationUrl, ex.location) | ||
| case exs: ExpectationsFailed => | ||
| Failures(exs.failures.map { ex => | ||
| Failures.Failure(ex.message, ex, ex.locations) | ||
| Failures.Failure(ex.message, ex, sourceLocationUrl, ex.locations) | ||
| }) | ||
| case other => | ||
| Exception(other) | ||
|
|
@@ -137,6 +148,7 @@ private[weaver] object Result { | |
|
|
||
| if (errorOutputLines.nonEmpty) { | ||
| formatDescription(errorOutputLines.mkString(EOL), | ||
| None, | ||
| Nil, | ||
| Console.RED, | ||
| TAB2.prefix) | ||
|
|
@@ -145,6 +157,7 @@ private[weaver] object Result { | |
|
|
||
| val formattedMessage = formatDescription( | ||
| msg, | ||
| None, | ||
| Nil, | ||
| Console.RED, | ||
| TAB2.prefix | ||
|
|
@@ -159,20 +172,21 @@ private[weaver] object Result { | |
|
|
||
| private def formatDescription( | ||
| message: String, | ||
| sourceLocationUrl: Option[String], | ||
| location: List[SourceLocation], | ||
| color: String, | ||
| prefix: String): String = { | ||
|
|
||
| val prefixIsWhitespace = prefix.trim.isEmpty | ||
| val footer = locationFooter(location) | ||
| val footer = locationFooter(sourceLocationUrl, location) | ||
| val lines = (message.split("\\r?\\n") ++ footer).zipWithIndex.map { | ||
| case (line, index) => | ||
| val linePrefix = | ||
| if (prefixIsWhitespace && line.trim.isEmpty) "" else prefix | ||
| if (index == 0) | ||
| color + linePrefix + line + | ||
| location | ||
| .map(l => s" (${l.fileRelativePath}:${l.line})") | ||
| .map(l => s" (${formatLocationPath(sourceLocationUrl, l)})") | ||
| .mkString("\n") | ||
| else | ||
| color + linePrefix + line | ||
|
|
@@ -181,14 +195,30 @@ private[weaver] object Result { | |
| lines.mkString(EOL) + Console.RESET | ||
| } | ||
|
|
||
| private def locationFooter(locations: List[SourceLocation]): List[String] = { | ||
| private def locationFooter( | ||
| sourceLocationUrl: Option[String], | ||
| locations: List[SourceLocation]): List[String] = { | ||
| val lines = locations.flatMap { l => | ||
| val prefix = s"${l.fileRelativePath}:${l.line}" | ||
| l.sourceCode.fold(List.empty[String]) { sourceCode => | ||
| val pointer = " " * (sourceCode.column - 1) + "^" | ||
| List(prefix, sourceCode.sourceLine, pointer) | ||
| List(formatLocationPath(sourceLocationUrl, l), | ||
| sourceCode.sourceLine, | ||
| pointer) | ||
| } | ||
| } | ||
| if (lines.nonEmpty) "" :: lines else Nil | ||
| } | ||
|
|
||
| private def formatLocationPath( | ||
| sourceLocationUrl: Option[String], | ||
| l: SourceLocation): String = | ||
| sourceLocationUrl match { | ||
| case Some(url) => | ||
| // Display a URL to a source location on a CI host. Line numbers are typically referenced with #L anchors. | ||
| s"${url}${l.fileRelativePath}#L${l.line}" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the user be able to override the anchor with config/env var? |
||
| case None => | ||
| // Display a path to a local file. | ||
| s"${l.fileRelativePath}:${l.line}" | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,21 +8,25 @@ import cats.data.Chain | |
| import cats.effect.Ref | ||
| import cats.effect.Clock | ||
| import cats.effect.Concurrent | ||
| import cats.effect.std.Env | ||
| import cats.syntax.all._ | ||
|
|
||
| import weaver.internals.SourceLocationUrl | ||
| object Test { | ||
|
|
||
| def apply[F[_]](name: String, f: Log[F] => F[Expectations])( | ||
| implicit F: Defer[F], | ||
| G: Concurrent[F], | ||
| C: Clock[F]): F[TestOutcome] = { | ||
| C: Clock[F], | ||
| E: Env[F] | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure it's necessary for the change to impact these layers of the framework. You could centralise the querying of the environment in the formatter (in an unsafe fashion), and apply the github-specific rendering there. I appreciate that reading the environment does not fall strictly in the pure-FP paradigm that Typelevel ougth to strive for, but the sbt-test-interface that weaver implements forces some compromise onto us. I'm happy to deviate a little bit out of the CE idioms if it minimises the impact / the amount of things the information needs to be threaded through. Additionally, the set of environment variables is pretty much immutable during the lifetime of a program. |
||
| ): F[TestOutcome] = { | ||
| for { | ||
| ref <- Ref[F].of(Chain.empty[Log.Entry]) | ||
| start <- C.realTime | ||
| res <- Defer[F] | ||
| ref <- Ref[F].of(Chain.empty[Log.Entry]) | ||
| start <- C.realTime | ||
| sourceUrl <- SourceLocationUrl[F] | ||
| res <- Defer[F] | ||
| .defer(f(Log.collected[F, Chain](ref, C.realTime.map(_.toMillis)))) | ||
| .map(Result.fromAssertion) | ||
| .handleError(ex => Result.from(ex)) | ||
| .map(Result.fromAssertion(sourceUrl, _)) | ||
| .handleError(ex => Result.from(sourceUrl, ex)) | ||
| end <- C.realTime | ||
| logs <- ref.get | ||
| } yield TestOutcome(name, end - start, res, logs) | ||
|
|
@@ -33,8 +37,8 @@ object Test { | |
| val (attempt, duration) = Try(ex()) -> (System.currentTimeMillis() - start) | ||
|
|
||
| val res = attempt match { | ||
| case Success(assertions) => Result.fromAssertion(assertions) | ||
| case Failure(e) => Result.from(e) | ||
| case Success(assertions) => Result.fromAssertion(None, assertions) | ||
| case Failure(e) => Result.from(None, e) | ||
| } | ||
|
|
||
| TestOutcome(name, duration.millis, res, Chain.empty) | ||
|
|
@@ -43,7 +47,7 @@ object Test { | |
| def apply[F[_]](name: String, f: F[Expectations])( | ||
| implicit F: Defer[F], | ||
| G: Concurrent[F], | ||
| C: Clock[F] | ||
| C: Clock[F], | ||
| E: Env[F] | ||
| ): F[TestOutcome] = apply[F](name, (_: Log[F]) => f) | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| package weaver.internals | ||
|
|
||
| import cats.effect.std.Env | ||
| import cats.Monad | ||
| import cats.syntax.all._ | ||
|
|
||
| /** | ||
| * Constructs base URL to use for displaying source locations in failure | ||
| * messages. | ||
| */ | ||
| private[weaver] object SourceLocationUrl { | ||
| def apply[F[_]](implicit E: Env[F], F: Monad[F]): F[Option[String]] = { | ||
| // Users can support non-GitHub CIs by setting WEAVER_SOURCE_URL | ||
| val readWeaverUrl = E.get("WEAVER_SOURCE_URL") | ||
| // If tests are run on GitHub Actions, construct a URL to the test source code. | ||
| // See https://docs.github.com/en/actions/reference/workflows-and-actions/variables#default-environment-variables | ||
| val readGitHubUrl = ( | ||
| E.get("GITHUB_SERVER_URL"), | ||
| E.get("GITHUB_REPOSITORY"), | ||
| E.get("GITHUB_SHA") | ||
| ).tupled.map(_.mapN { case (serverUrl, repo, sha) => | ||
| s"$serverUrl/$repo/tree/$sha/" | ||
| }) | ||
| (readWeaverUrl, readGitHubUrl).mapN { | ||
| case (weaverUrl, gitHubUrl) => weaverUrl.orElse(gitHubUrl) | ||
| } | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: this single named parameter looks odd, can we align it with the rest of the code?