Merge pull request #3102 from guardian/fix-issue-3096-dependency-frequency-was-ignored-for-grouped-prs

Fix #3096: Don't ignore dependency-frequency-limits for *grouped* PRs

Author: alejandrohdezma

modules/core/src/main/scala/org/scalasteward/core/data/Update.scala

@@ -35,6 +35,7 @@ sealed trait Update {
 
   def show: String
 
+  val asSingleUpdates: List[Update.Single]
 }
 
 object Update {
@@ -46,16 +47,18 @@ object Update {
   ) extends Update {
 
     override def show: String = name
-
+    override val asSingleUpdates: List[Update.Single] = updates
   }
 
   sealed trait Single extends Product with Serializable with Update {
+    override val asSingleUpdates: List[Update.Single] = List(this)
     def forArtifactIds: Nel[ForArtifactId]
     def crossDependencies: Nel[CrossDependency]
     def dependencies: Nel[Dependency]
     def groupId: GroupId
     def artifactIds: Nel[ArtifactId]
     def mainArtifactId: String
+    def groupAndMainArtifactId: (GroupId, String) = (groupId, mainArtifactId)
     def currentVersion: Version
     def newerVersions: Nel[Version]
 

modules/core/src/main/scala/org/scalasteward/core/nurture/PullRequestRepository.scala

@@ -112,13 +112,10 @@ final class PullRequestRepository[F[_]](kvStore: KeyValueStore[F, Repo, Map[Uri,
       case None => Map.empty
       case Some(pullRequests) =>
         pullRequests.values
-          .collect { case Entry(_, u: Update.Single, _, entryCreatedAt, _, _) =>
-            (u.groupId, u.mainArtifactId, entryCreatedAt)
+          .flatMap { entry =>
+            entry.update.asSingleUpdates.map(_.groupAndMainArtifactId -> entry.entryCreatedAt)
           }
-          .groupBy { case (groupId, mainArtifactId, _) => (groupId, mainArtifactId) }
-          .view
-          .mapValues(_.map { case (_, _, createdAt) => createdAt }.max)
-          .toMap
+          .groupMapReduce(_._1)(_._2)(_ max _)
     }
 }
 

modules/core/src/test/scala/org/scalasteward/core/nurture/PullRequestRepositoryTest.scala

@@ -11,11 +11,13 @@ import org.scalasteward.core.forge.data.{PullRequestNumber, PullRequestState}
 import org.scalasteward.core.git.{Branch, Sha1}
 import org.scalasteward.core.mock.MockConfig.config
 import org.scalasteward.core.mock.MockContext.context.pullRequestRepository
-import org.scalasteward.core.mock.MockState
 import org.scalasteward.core.mock.MockState.TraceEntry
 import org.scalasteward.core.mock.MockState.TraceEntry.Cmd
+import org.scalasteward.core.mock.{MockEff, MockState}
 import org.scalasteward.core.util.Nel
 
+import java.util.concurrent.atomic.AtomicInteger
+
 class PullRequestRepositoryTest extends FunSuite {
   private def checkTrace(state: MockState, trace: Vector[TraceEntry]): Unit =
     assertEquals(state.copy(files = Map.empty), MockState.empty.copy(trace = trace))
@@ -29,166 +31,119 @@ class PullRequestRepositoryTest extends FunSuite {
   private val url = uri"https://github.com/typelevel/cats/pull/3291"
   private val sha1 = Sha1.unsafeFrom("a2ced5793c2832ada8c14ba5c77e51c4bc9656a8")
   private val number = PullRequestNumber(3291)
-  private val branch = Branch("update")
+  private def groupedUpdate(updates: Update.ForArtifactId*) =
+    Update.Grouped("group", None, updates.toList)
+  private def openPRFor(update: Update): PullRequestData[Id] =
+    PullRequestData[Id](url, sha1, update, Open, number, Branch("update"))
 
-  test("createOrUpdate >> findPullRequest >> lastPullRequestCreatedAt") {
-    val repo = Repo("pr-repo-test", "repo1")
-    val update = portableScala
-    val data = PullRequestData[Id](url, sha1, update, Open, number, branch)
+  private val repoCounter = new AtomicInteger()
 
-    val p = for {
-      _ <- pullRequestRepository.createOrUpdate(repo, data)
-      result <- pullRequestRepository.findLatestPullRequest(repo, update.crossDependency, "1.0.0".v)
-      createdAt <- pullRequestRepository.lastPullRequestCreatedAt(repo)
-    } yield (result, createdAt)
-    val (state, (result, createdAt)) = p.runSA(MockState.empty).unsafeRunSync()
+  private def executeOnTestRepo[T](expectedStoreOps: Seq[String])(p: Repo => MockEff[T]): T = {
+    val repo = Repo("pr-repo-test", s"repo${repoCounter.getAndIncrement()}")
+    val (state, output) = p(repo).runSA(MockState.empty).unsafeRunSync()
 
     val store =
       config.workspace / s"store/pull_requests/v2/github/${repo.toPath}/pull_requests.json"
-    assertEquals(result.map(d => (d.url, d.baseSha1, d.state)), Some((url, sha1, Open)))
-    assert(createdAt.isDefined)
-
     checkTrace(
       state,
-      Vector(
-        Cmd("read", store.toString),
-        Cmd("write", store.toString)
-      )
+      expectedStoreOps.map(op => Cmd(op, store.toString)).toVector
     )
+
+    output
   }
 
-  test("getObsoleteOpenPullRequests for single update") {
-    val repo = Repo("pr-repo-test", "repo2")
+  private def beforeAndAfterPRCreation[T](update: Update)(func: Repo => MockEff[T]): (T, T) =
+    executeOnTestRepo(expectedStoreOps = Seq("read", "write")) { repo =>
+      for {
+        before <- func(repo)
+        _ <- pullRequestRepository.createOrUpdate(repo, openPRFor(update))
+        after <- func(repo)
+      } yield (before, after)
+    }
+
+  test("createOrUpdate >> findPullRequest >> lastPullRequestCreatedAt") {
     val update = portableScala
+    val data = openPRFor(update)
+
+    val (result, createdAt) = executeOnTestRepo(expectedStoreOps = Seq("read", "write")) { repo =>
+      for {
+        _ <- pullRequestRepository.createOrUpdate(repo, data)
+        result <-
+          pullRequestRepository.findLatestPullRequest(repo, update.crossDependency, "1.0.0".v)
+        createdAt <- pullRequestRepository.lastPullRequestCreatedAt(repo)
+      } yield (result, createdAt)
+    }
+
+    assertEquals(result.map(d => (d.url, d.baseSha1, d.state)), Some((url, sha1, Open)))
+    assert(createdAt.isDefined)
+  }
+
+  test("getObsoleteOpenPullRequests for single update") {
+    val data = openPRFor(portableScala)
     val nextUpdate = portableScala.copy(newerVersions = Nel.of("1.0.1".v))
-    val data = PullRequestData[Id](url, sha1, update, Open, number, branch)
-
-    val p = for {
-      emptyResult <- pullRequestRepository.getObsoleteOpenPullRequests(repo, nextUpdate)
-      _ <- pullRequestRepository.createOrUpdate(repo, data)
-      result <- pullRequestRepository.getObsoleteOpenPullRequests(repo, nextUpdate)
-      _ <- pullRequestRepository.changeState(repo, url, PullRequestState.Closed)
-      closedResult <- pullRequestRepository.getObsoleteOpenPullRequests(repo, nextUpdate)
-    } yield (emptyResult, result, closedResult)
-    val (state, (emptyResult, result, closedResult)) = p.runSA(MockState.empty).unsafeRunSync()
-    val store =
-      config.workspace / s"store/pull_requests/v2/github/${repo.toPath}/pull_requests.json"
+
+    val (emptyResult, result, closedResult) =
+      executeOnTestRepo(expectedStoreOps = Seq("read", "write", "write")) { repo =>
+        val getObsoleteOpenPRs = pullRequestRepository.getObsoleteOpenPullRequests(repo, nextUpdate)
+        for {
+          emptyResult <- getObsoleteOpenPRs
+          _ <- pullRequestRepository.createOrUpdate(repo, data)
+          result <- getObsoleteOpenPRs
+          _ <- pullRequestRepository.changeState(repo, data.url, PullRequestState.Closed)
+          closedResult <- getObsoleteOpenPRs
+        } yield (emptyResult, result, closedResult)
+      }
+
     assertEquals(emptyResult, List.empty)
     assertEquals(closedResult, List.empty)
     assertEquals(result, List(data))
-
-    checkTrace(
-      state,
-      Vector(
-        Cmd("read", store.toString),
-        Cmd("write", store.toString),
-        Cmd("write", store.toString)
-      )
-    )
   }
 
   test("getObsoleteOpenPullRequests for the same single update") {
-    val repo = Repo("pr-repo-test", "repo3")
-    val update = portableScala
-    val data = PullRequestData[Id](url, sha1, update, Open, number, branch)
-
-    val p = for {
-      emptyResult <- pullRequestRepository.getObsoleteOpenPullRequests(repo, update)
-      _ <- pullRequestRepository.createOrUpdate(repo, data)
-      result <- pullRequestRepository.getObsoleteOpenPullRequests(repo, update)
-    } yield (emptyResult, result)
-    val (state, (emptyResult, result)) = p.runSA(MockState.empty).unsafeRunSync()
-    val store =
-      config.workspace / s"store/pull_requests/v2/github/${repo.toPath}/pull_requests.json"
-    assertEquals(emptyResult, List.empty)
-    assertEquals(result, List.empty)
+    val (before, after) = beforeAndAfterPRCreation(portableScala) { repo =>
+      pullRequestRepository.getObsoleteOpenPullRequests(repo, portableScala)
+    }
 
-    checkTrace(
-      state,
-      Vector(
-        Cmd("read", store.toString),
-        Cmd("write", store.toString)
-      )
-    )
+    assertEquals(before, List.empty)
+    assertEquals(after, List.empty)
   }
 
   test("getObsoleteOpenPullRequests for the another single update and ignore closed") {
-    val repo = Repo("pr-repo-test", "repo4")
-    val updateInStore = portableScala
-    val newUpdate = catsCore
-    val data = PullRequestData[Id](url, sha1, updateInStore, Open, number, branch)
-
-    val p = for {
-      emptyResult <- pullRequestRepository.getObsoleteOpenPullRequests(repo, updateInStore)
-      _ <- pullRequestRepository.createOrUpdate(repo, data)
-      result <- pullRequestRepository.getObsoleteOpenPullRequests(repo, newUpdate)
-    } yield (emptyResult, result)
-    val (state, (emptyResult, result)) = p.runSA(MockState.empty).unsafeRunSync()
-    val store =
-      config.workspace / s"store/pull_requests/v2/github/${repo.toPath}/pull_requests.json"
+    val (emptyResult, result) =
+      executeOnTestRepo(expectedStoreOps = Seq("read", "write")) { repo =>
+        for {
+          emptyResult <- pullRequestRepository.getObsoleteOpenPullRequests(repo, portableScala)
+          _ <- pullRequestRepository.createOrUpdate(repo, openPRFor(portableScala))
+          result <- pullRequestRepository.getObsoleteOpenPullRequests(repo, catsCore)
+        } yield (emptyResult, result)
+      }
+
     assertEquals(emptyResult, List.empty)
     assertEquals(result, List.empty)
-
-    checkTrace(
-      state,
-      Vector(
-        Cmd("read", store.toString),
-        Cmd("write", store.toString)
-      )
-    )
   }
 
   test("findLatestPullRequest ignores grouped updates") {
-    val repo = Repo("pr-repo-test", "repo5")
-    val update = portableScala
-    val grouped = Update.Grouped("group", None, List(update))
-    val data = PullRequestData[Id](url, sha1, grouped, Open, number, branch)
-
-    val p = for {
-      _ <- pullRequestRepository.createOrUpdate(repo, data)
-      result <- pullRequestRepository.findLatestPullRequest(repo, update.crossDependency, "1.0.0".v)
-    } yield result
-
-    val (state, result) = p.runSA(MockState.empty).unsafeRunSync()
-
-    val store =
-      config.workspace / s"store/pull_requests/v2/github/${repo.toPath}/pull_requests.json"
+    val (_, result) = beforeAndAfterPRCreation(groupedUpdate(portableScala)) { repo =>
+      pullRequestRepository.findLatestPullRequest(repo, portableScala.crossDependency, "1.0.0".v)
+    }
     assert(result.isEmpty)
-
-    checkTrace(
-      state,
-      Vector(
-        Cmd("read", store.toString),
-        Cmd("write", store.toString)
-      )
-    )
   }
 
   test("lastPullRequestCreatedAt returns timestamp for grouped updates") {
-    val repo = Repo("pr-repo-test", "repo7")
-    val update = catsCore
-    val grouped = Update.Grouped("group", None, List(update))
-    val data = PullRequestData[Id](url, sha1, grouped, Open, number, branch)
-
-    val p = for {
-      emptyCreatedAt <- pullRequestRepository.lastPullRequestCreatedAt(repo)
-      _ <- pullRequestRepository.createOrUpdate(repo, data)
-      createdAt <- pullRequestRepository.lastPullRequestCreatedAt(repo)
-    } yield (emptyCreatedAt, createdAt)
-    val (state, (emptyCreatedAt, createdAt)) = p.runSA(MockState.empty).unsafeRunSync()
-
-    val store =
-      config.workspace / s"store/pull_requests/v2/github/${repo.toPath}/pull_requests.json"
-    assert(emptyCreatedAt.isEmpty)
-    assert(createdAt.isDefined)
-
-    checkTrace(
-      state,
-      Vector(
-        Cmd("read", store.toString),
-        Cmd("write", store.toString)
+    val (before, after) =
+      beforeAndAfterPRCreation(groupedUpdate(catsCore))(
+        pullRequestRepository.lastPullRequestCreatedAt
       )
-    )
+    assert(before.isEmpty)
+    assert(after.isDefined)
   }
 
+  test("lastPullRequestCreatedAtByArtifact for grouped updates") {
+    val (before, after) =
+      beforeAndAfterPRCreation(groupedUpdate(catsCore))(
+        pullRequestRepository.lastPullRequestCreatedAtByArtifact
+      )
+    assert(before.isEmpty)
+    assert(after.contains(catsCore.groupAndMainArtifactId))
+  }
 }