Improve coverage of the github PR review module. (#316)

src/main/scala/com/mwz/sonar/scala/pr/GithubPrReviewJob.scala

@@ -120,7 +120,7 @@ final class GithubPrReviewJob(
       // Group patches by file names - `filename` is the full path relative to the root
       // of the project, so it should be unique. Raise an error when no files are present.
       prPatches <- Sync[F].fromEither(Either.fromOption(NonEmptyList.fromList(files), NoFilesInPR))
-      allPatches = prPatches.groupByNem(_.filename).map(_.head).toSortedMap.toMap
+      allPatches = prPatches.groupByNem(_.filename).map(_.head).toSortedMap
       // Filter out issues which aren't related to any files in the PR.
       issues = globalIssues.allIssues.filterKeys(f => allPatches.keySet.contains(f.toString))
       // Get new comments and post them.
@@ -132,7 +132,9 @@ final class GithubPrReviewJob(
       _ <- commentsToPost
         .sortBy(c => (c.path, c.position))
         .traverse { comment =>
-          Logger[F].debug(s"Posting a new comment $comment.") >>
+          Logger[F].debug(
+            s"Posting a new comment for ${comment.path}:${comment.position} - ${comment.body}"
+          ) >>
           github.createComment(comment)
         }
     } yield reviewStatus(issues)

src/main/scala/com/mwz/sonar/scala/pr/model.scala

@@ -20,7 +20,6 @@ package pr
 
 import org.http4s.Uri
 
-// TODO: Capture more custom errors.
 @SuppressWarnings(Array("IncorrectlyNamedExceptions"))
 sealed trait ReviewError extends Exception
 case object NoFilesInPR extends ReviewError

src/main/scala/com/mwz/sonar/scala/pr/patch.scala

@@ -74,4 +74,5 @@ object Patch {
           }
       }
       .map(_.fileToPatch)
+      .filterOrElse(_.nonEmpty, PatchError(patch))
 }

src/test/resources/patches/add-del-mod.patch

@@ -0,0 +1,27 @@
+@@ -43,6 +43,8 @@ final case class ConfigError(error: String) extends Exception
+ final class GlobalConfig(config: Configuration) {
+   private[this] val logger = Log(classOf[Scala], "config")
+
++  // TODO: test
++
+   val baseUrl: ConfigErrorOr[Uri] =
+     config
+       .get(CoreProperties.SERVER_BASE_URL)
+@@ -58,8 +60,6 @@ final class GlobalConfig(config: Configuration) {
+       )(Uri.fromString)
+       .leftMap(f => ConfigError(f.sanitized))
+
+-  val pullRequest: EitherT[Option, ConfigError, PullRequest] = getPullRequest
+-
+   /**
+    * Pull request mode which enables PR decoration
+    * (for both issues and coverage).
+@@ -77,7 +77,7 @@ final class GlobalConfig(config: Configuration) {
+    * Post coverage data as PR comments.
+    */
+   def coverageDecoration: Boolean =
+-    pullRequest.exists(!_.disableCoverage).getOrElse(false)
++    false
+
+   private[this] def getPullRequest: EitherT[Option, ConfigError, PullRequest] =
+     for {

src/test/resources/patches/del.patch

@@ -0,0 +1,23 @@
+@@ -26,8 +26,6 @@ sealed trait ReviewError extends Exception
+ case object NoFilesInPR extends ReviewError
+
+ sealed trait PrStatus extends Product with Serializable
+-case object Pending extends PrStatus
+-case object Success extends PrStatus
+ final case class Error(reviewStatus: ReviewStatus) extends PrStatus
+ final case class Failure(error: Throwable) extends PrStatus
+
+@@ -45,13 +43,8 @@ object ReviewStatus {
+         s"$blockers blocker${form(blockers)}"
+       case ReviewStatus(_, critical) if critical > 0 =>
+         s"$critical critical issue${form(critical)}"
+-      case _ =>
+-        "no critical or blocker issues"
+     }
+   }
+-
+-  private[pr] def form(i: Int): String =
+-    if (i > 1) "s" else ""
+ }
+
+ final case class Markdown(text: String) extends AnyVal

src/test/scala/com/mwz/sonar/scala/WithFiles.scala → src/test/scala/com/mwz/sonar/scala/fixtures.scala

@@ -20,6 +20,10 @@ package com.mwz.sonar.scala
 import java.io.File
 import java.nio.file.{Files, Path}
 
+import cats.effect.IO
+import cats.effect.concurrent.Ref
+import com.mwz.sonar.scala.util.Logger
+
 trait WithFiles {
   def withFiles(paths: String*)(test: Seq[File] => Any): Unit = {
     val tmpDir: Path = Files.createTempDirectory("")
@@ -33,3 +37,30 @@ trait WithFiles {
     }
   }
 }
+
+trait WithTracing {
+  def withTracing(test: Ref[IO, List[String]] => Any): Unit =
+    test(Ref.unsafe[IO, List[String]](List.empty))
+}
+
+trait WithLogging {
+  object LogLevel {
+    sealed trait Level
+    case object Debug extends Level
+    case object Info extends Level
+    case object Warn extends Level
+    case object Error extends Level
+  }
+
+  def withLogging(test: (Ref[IO, List[(LogLevel.Level, String)]], Logger[IO]) => Any): Unit = {
+    val logs = Ref.unsafe[IO, List[(LogLevel.Level, String)]](List.empty)
+    val logger: Logger[IO] = new Logger[IO] {
+      def debug(s: String): IO[Unit] = logs.update((LogLevel.Debug, s) :: _)
+      def info(s: String): IO[Unit] = logs.update((LogLevel.Info, s) :: _)
+      def warn(s: String): IO[Unit] = logs.update((LogLevel.Warn, s) :: _)
+      def error(s: String): IO[Unit] = logs.update((LogLevel.Error, s) :: _)
+      def error(s: String, e: Throwable): IO[Unit] = logs.update((LogLevel.Error, s) :: _)
+    }
+    test(logs, logger)
+  }
+}

src/test/scala/com/mwz/sonar/scala/pr/Generators.scala

@@ -17,6 +17,7 @@
 
 package com.mwz.sonar.scala.pr
 
+import com.mwz.sonar.scala.pr.github.File
 import org.http4s.Uri
 import org.scalacheck._
 import org.sonar.api.batch.fs.InputFile
@@ -35,6 +36,16 @@ object Generators {
         }
     )
 
+  implicit val arbFile: Arbitrary[File] = {
+    Arbitrary(
+      for {
+        fileName <- Gen.nonEmptyListOf(Gen.alphaNumChar)
+        status   <- Gen.alphaLowerStr
+        patch    <- Gen.alphaNumStr
+      } yield File(fileName.mkString, status, patch)
+    )
+  }
+
   implicit val arbRuleKey: Arbitrary[RuleKey] =
     Arbitrary(
       for {

src/test/scala/com/mwz/sonar/scala/pr/GithubPrReviewJobSpec.scala

@@ -17,18 +17,30 @@
 
 package com.mwz.sonar.scala.pr
 
+import scala.concurrent.ExecutionContext
+import scala.language.higherKinds
+
+import cats.data.NonEmptyList
+import cats.effect.ContextShift
 import cats.effect.IO
+import cats.syntax.flatMap._
 import com.mwz.sonar.scala.EmptyLogger
 import com.mwz.sonar.scala.GlobalConfig
+import com.mwz.sonar.scala.WithLogging
+import com.mwz.sonar.scala.WithTracing
 import com.mwz.sonar.scala.pr.Generators._
 import com.mwz.sonar.scala.pr.github.File
+import com.mwz.sonar.scala.pr.github.Github
 import com.mwz.sonar.scala.pr.github.PullRequest
+import com.mwz.sonar.scala.pr.github.Status
 import com.mwz.sonar.scala.pr.github.{Comment, NewComment, NewStatus, User}
+import com.mwz.sonar.scala.util.Logger
 import org.http4s.Uri
 import org.scalacheck.ScalacheckShapeless._
 import org.scalatest.{FlatSpec, LoneElement, Matchers}
 import org.scalatestplus.scalacheck.ScalaCheckDrivenPropertyChecks
 import org.sonar.api.batch.fs.InputFile
+import org.sonar.api.batch.fs.internal.TestInputFileBuilder
 import org.sonar.api.batch.postjob.internal.DefaultPostJobDescriptor
 import org.sonar.api.batch.rule.Severity
 import org.sonar.api.config.internal.MapSettings
@@ -38,11 +50,31 @@ class GithubPrReviewJobSpec
     extends FlatSpec
     with Matchers
     with LoneElement
-    with ScalaCheckDrivenPropertyChecks {
+    with ScalaCheckDrivenPropertyChecks
+    with WithTracing
+    with WithLogging {
+
   trait Ctx {
     val globalConfig = new GlobalConfig(new MapSettings().asConfig)
     val globalIssues = new GlobalIssues
-    val githubPrReviewJob = new GithubPrReviewJob(globalConfig, globalIssues)
+    def githubPrReviewJob(
+      globalConfig: GlobalConfig = globalConfig,
+      globalIssues: GlobalIssues = globalIssues
+    ) = new GithubPrReviewJob(globalConfig, globalIssues)
+  }
+
+  trait IOCtx {
+    implicit val ec: ExecutionContext = ExecutionContext.global
+    implicit val cs: ContextShift[IO] = IO.contextShift(ec)
+  }
+
+  trait GithubNotImpl[F[_]] extends Github[F] {
+    def authenticatedUser: F[User] = ???
+    def pullRequest: F[PullRequest] = ???
+    def comments: F[List[Comment]] = ???
+    def createComment(comment: NewComment): F[Comment] = ???
+    def files: F[List[File]] = ???
+    def createStatus(sha: String, status: NewStatus): F[Status] = ???
   }
 
   val patch =
@@ -57,6 +89,105 @@ class GithubPrReviewJobSpec
     descriptor.name shouldBe "Github PR review job"
   }
 
+  it should "not create a review if no files exist in a PR" in new Ctx with IOCtx with EmptyLogger {
+    forAll { (baseUrl: Uri, user: User, pr: PullRequest) =>
+      withTracing { trace =>
+        val github = new GithubNotImpl[IO] {
+          override def comments = trace.update("comments" :: _) >> IO.pure(List.empty)
+          override def files = trace.update("files" :: _) >> IO.pure(List.empty)
+        }
+        val result = githubPrReviewJob().review[IO](baseUrl, github, user, pr)
+
+        result.attempt.unsafeRunSync() shouldBe Left(NoFilesInPR)
+        trace.get.unsafeRunSync() should contain theSameElementsAs List("files", "comments")
+      }
+    }
+  }
+
+  it should "not post any comments for no issues" in new Ctx with IOCtx with EmptyLogger {
+    forAll { (baseUrl: Uri, user: User, pr: PullRequest, prFiles: NonEmptyList[File]) =>
+      withTracing { trace =>
+        val github = new GithubNotImpl[IO] {
+          override def comments = trace.update("comments" :: _) >> IO.pure(List.empty)
+          override def files = trace.update("files" :: _) >> IO.pure(prFiles.toList)
+        }
+        val result = githubPrReviewJob().review[IO](baseUrl, github, user, pr)
+
+        result.attempt.unsafeRunSync() shouldBe Right(ReviewStatus(blocker = 0, critical = 0))
+        trace.get.unsafeRunSync() should contain theSameElementsAs List("files", "comments")
+      }
+    }
+  }
+
+  it should "log and skip patches which can't be parsed" in new Ctx with IOCtx {
+    forAll { (baseUrl: Uri, user: User, pr: PullRequest, prFiles: NonEmptyList[File]) =>
+      withTracing { trace =>
+        withLogging {
+          case (logs, implicit0(logger: Logger[IO])) =>
+            val github = new GithubNotImpl[IO] {
+              override def comments = trace.update("comments" :: _) >> IO.pure(List.empty)
+              override def files = trace.update("files" :: _) >> IO.pure(prFiles.toList)
+            }
+            val file: InputFile = TestInputFileBuilder.create("", prFiles.head.filename).build()
+            val issue = Issue(RuleKey.of("repo", "rule"), file, 1, Severity.CRITICAL, "msg")
+
+            val issues = new GlobalIssues
+            issues.add(issue)
+            val result = githubPrReviewJob(globalIssues = issues).review[IO](baseUrl, github, user, pr)
+
+            result.attempt.unsafeRunSync() shouldBe Right(ReviewStatus(blocker = 0, critical = 1))
+            trace.get.unsafeRunSync() should contain theSameElementsAs List("files", "comments")
+            logs.get.unsafeRunSync().collect {
+              case (level, msg) if level === LogLevel.Error => msg
+            } should contain theSameElementsAs List(
+              s"Error parsing patch for ${prFiles.head.filename}."
+            )
+        }
+      }
+    }
+  }
+
+  it should "post comments for pr issues" in new Ctx with IOCtx with EmptyLogger {
+    forAll { (baseUrl: Uri, user: User, pr: PullRequest, prFile: File) =>
+      withTracing { trace =>
+        withLogging {
+          case (logs, implicit0(logger: Logger[IO])) =>
+            val fileWithPatch = prFile.copy(patch = patch)
+            val github = new GithubNotImpl[IO] {
+              override def comments = trace.update("comments" :: _) >> IO.pure(List.empty)
+              override def createComment(comment: NewComment) =
+                trace.update("createComment" :: _) >>
+                IO.pure(Comment(1, comment.path, Some(comment.position), user, comment.body))
+              override def files = trace.update("files" :: _) >> IO.pure(List(fileWithPatch))
+            }
+            val file: InputFile = TestInputFileBuilder.create("", fileWithPatch.filename).build()
+            val issue = Issue(RuleKey.of("repo", "rule"), file, 5, Severity.BLOCKER, "msg")
+            val markdown: Markdown = Markdown.inline(baseUrl, issue)
+
+            val issues = new GlobalIssues
+            issues.add(issue)
+            val result = githubPrReviewJob(globalIssues = issues).review[IO](baseUrl, github, user, pr)
+
+            result.attempt.unsafeRunSync() shouldBe Right(ReviewStatus(blocker = 1, critical = 0))
+            trace.get.unsafeRunSync() should contain theSameElementsAs List(
+              "files",
+              "comments",
+              "createComment"
+            )
+            logs.get.unsafeRunSync().collect {
+              case (level, msg)
+                  if level === LogLevel.Info ||
+                  (level === LogLevel.Debug && msg.contains("Posting a new comment for")) =>
+                msg
+            } should contain theSameElementsAs List(
+              "Posting new comments to Github.",
+              s"Posting a new comment for ${prFile.filename}:${issue.line} - ${markdown.text}"
+            )
+        }
+      }
+    }
+  }
+
   it should "not create new comments if there are no issues" in new Ctx with EmptyLogger {
     forAll {
       (
@@ -68,7 +199,7 @@ class GithubPrReviewJobSpec
         patches: Map[String, File]
       ) =>
         val issues: Map[InputFile, List[Issue]] = Map.empty
-        githubPrReviewJob
+        githubPrReviewJob()
           .newComments[IO](baseUrl, user, pr, comments, files, patches, issues)
           .unsafeRunSync() shouldBe empty
     }
@@ -94,7 +225,7 @@ class GithubPrReviewJobSpec
           Comment(1, issue.file.toString, Some(5), user, markdown.text)
         )
 
-        githubPrReviewJob
+        githubPrReviewJob()
           .newComments[IO](baseUrl, user, pr, comments, files, patches, issues)
           .unsafeRunSync() shouldBe empty
     }
@@ -119,7 +250,7 @@ class GithubPrReviewJobSpec
 
         val expected = NewComment(markdown.text, pr.head.sha, issue.file.toString, 5)
 
-        githubPrReviewJob
+        githubPrReviewJob()
           .newComments[IO](baseUrl, user, pr, List.empty, files, patches, issues)
           .unsafeRunSync()
           .loneElement shouldBe expected
@@ -148,7 +279,7 @@ class GithubPrReviewJobSpec
 
         val expected = NewComment(markdown.text, pr.head.sha, issue.file.toString, 5)
 
-        githubPrReviewJob
+        githubPrReviewJob()
           .newComments[IO](baseUrl, user, pr, comments, files, patches, issues)
           .unsafeRunSync()
           .loneElement shouldBe expected
@@ -251,7 +382,7 @@ class GithubPrReviewJobSpec
   }
 
   it should "determine a review status based on found issues" in {
-    forAll { (i: List[Issue]) =>
+    forAll { i: List[Issue] =>
       val blockers = i.filter(_.severity === Severity.BLOCKER).size
       val critical = i.filter(_.severity === Severity.CRITICAL).size
       GithubPrReviewJob.reviewStatus(i.groupBy(_.file)) shouldBe ReviewStatus(blockers, critical)