Refactoring PR description helper and starting to add tests

Having the tests makes it easier to visualize what's going on, because we can
just look at the test output data. This is not foolproof, since one of the IO
helper functions could generate bad data to pass to the pure helper, but it's
better than nothing.

This refactors as much as possible to keep the PR description logic in the pure
`prComment` function, rather than computing it in multiple places, which also
increases out unit test coverage.

nixpkgs-update.cabal

@@ -4,7 +4,7 @@ cabal-version: 2.2
 --
 -- see: https://github.com/sol/hpack
 --
--- hash: 7492d0fc2af5df377764d358317e0892c0bda2961943218f8dcdeef179700cd9
+-- hash: 0401b3234ff350b39169d38415559f3fe383a70cd04480c173c8219d5177c8ca
 
 name:           nixpkgs-update
 version:        0.2.0
@@ -152,6 +152,7 @@ test-suite spec
   other-modules:
       DoctestSpec
       RewriteSpec
+      UpdateSpec
       Paths_nixpkgs_update
   hs-source-dirs:
       test

src/Update.hs

@@ -6,12 +6,13 @@
 {-# OPTIONS_GHC -fno-warn-type-defaults #-}
 
 module Update
-  ( updateAll,
-    updatePackage,
-    cveReport,
+  ( addPatched,
     cveAll,
+    cveReport,
+    prMessage,
     sourceGithubAll,
-    addPatched,
+    updateAll,
+    updatePackage,
   )
 where
 
@@ -236,7 +237,7 @@ updatePackageBatch log updateEnv mergeBaseOutpathsContext =
     -- that we actually should be touching this file. Get to work processing the
     -- various rewrite functions!
     let rwArgs = Rewrite.Args updateEnv attrPath derivationFile derivationContents
-    msgs <- Rewrite.runAll log rwArgs
+    rewriteMsgs <- Rewrite.runAll log rwArgs
     ----------------------------------------------------------------------------
     --
     -- Compute the diff and get updated values
@@ -259,7 +260,7 @@ updatePackageBatch log updateEnv mergeBaseOutpathsContext =
     -- Publish the result
     lift . log $ "Successfully finished processing"
     result <- Nix.resultLink
-    publishPackage log updateEnv oldSrcUrl newSrcUrl attrPath result (Just opDiff) msgs
+    publishPackage log updateEnv oldSrcUrl newSrcUrl attrPath result (Just opDiff) rewriteMsgs
     Git.cleanAndResetTo "master"
 
 publishPackage ::
@@ -272,41 +273,18 @@ publishPackage ::
   Maybe (Set ResultLine) ->
   [Text] ->
   ExceptT Text IO ()
-publishPackage log updateEnv oldSrcUrl newSrcUrl attrPath result opDiff msgs = do
+publishPackage log updateEnv oldSrcUrl newSrcUrl attrPath result opDiff rewriteMsgs = do
   cachixTestInstructions <- doCachix log updateEnv result
   resultCheckReport <-
     case Blacklist.checkResult (packageName updateEnv) of
       Right () -> lift $ Check.result updateEnv (T.unpack result)
       Left msg -> pure msg
-  d <- Nix.getDescription attrPath <|> return T.empty
-  u <- Nix.getHomepageET attrPath <|> return T.empty
+  metaDescription <- Nix.getDescription attrPath <|> return T.empty
+  metaHomepage <- Nix.getHomepageET attrPath <|> return T.empty
   cveRep <- liftIO $ cveReport updateEnv
-  let metaDescription =
-        if d == T.empty
-          then ""
-          else "\n\nmeta.description for " <> attrPath <> " is: " <> d
-  let metaHomepage =
-        if u == T.empty
-          then ""
-          else "\nmeta.homepage for " <> attrPath <> " is: " <> u
-  let rewriteMessages = foldl (\ms m -> ms <> T.pack "\n- " <> m) "\n###### Updates performed" msgs
-  releaseUrlMessage <-
-    ( do
-        msg <- GH.releaseUrl updateEnv newSrcUrl
-        return ("\n- [Release on GitHub](" <> msg <> ")")
-      )
-      <|> return ""
-  compareUrlMessage <-
-    ( do
-        msg <- GH.compareUrl oldSrcUrl newSrcUrl
-        return ("\n- [Compare changes on GitHub](" <> msg <> ")\n\n")
-      )
-      <|> return "\n"
+  releaseUrl <- GH.releaseUrl updateEnv newSrcUrl <|> return ""
+  compareUrl <- GH.compareUrl oldSrcUrl newSrcUrl <|> return ""
   maintainers <- Nix.getMaintainers attrPath
-  let maintainersCc =
-        if not (T.null maintainers)
-          then "\n\ncc " <> maintainers <> " for testing."
-          else ""
   let commitMsg = commitMessage updateEnv attrPath
   Git.commit commitMsg
   commitHash <- Git.headHash
@@ -328,13 +306,13 @@ publishPackage log updateEnv oldSrcUrl newSrcUrl attrPath result opDiff msgs = d
           isBroken
           metaDescription
           metaHomepage
-          rewriteMessages
-          releaseUrlMessage
-          compareUrlMessage
+          rewriteMsgs
+          releaseUrl
+          compareUrl
           resultCheckReport
           commitHash
           attrPath
-          maintainersCc
+          maintainers
           result
           (fromMaybe "" (outpathReport <$> opDiff))
           cveRep
@@ -364,7 +342,7 @@ prMessage ::
   Bool ->
   Text ->
   Text ->
-  Text ->
+  [Text] ->
   Text ->
   Text ->
   Text ->
@@ -377,20 +355,44 @@ prMessage ::
   Text ->
   Text ->
   Text
-prMessage updateEnv isBroken metaDescription metaHomepage rewriteMessages releaseUrlMessage compareUrlMessage resultCheckReport commitHash attrPath maintainersCc resultPath opReport cveRep cachixTestInstructions nixpkgsReviewMsg =
+prMessage updateEnv isBroken metaDescription metaHomepage rewriteMsgs releaseUrl compareUrl resultCheckReport commitHash attrPath maintainers resultPath opReport cveRep cachixTestInstructions nixpkgsReviewMsg =
+  -- Some components of the PR description are pre-generated prior to calling
+  -- because they require IO, but in general try to put as much as possible for
+  -- the formatting into the pure function so that we can control the body
+  -- formatting in one place and unit test it.
   let brokenMsg = brokenWarning isBroken
       title = prTitle updateEnv attrPath
-      sourceLinkInfo = maybe "" pattern $ sourceURL updateEnv
-        where
-          pattern link = [interpolate|This update was made based on information from $link.|]
+      metaHomepageLine =
+        if metaHomepage == T.empty
+          then ""
+          else "\nmeta.homepage for " <> attrPath <> " is: " <> metaHomepage
+      metaDescriptionLine =
+        if metaDescription == T.empty
+          then ""
+          else "\n\nmeta.description for " <> attrPath <> " is: " <> metaDescription
+      rewriteMsgsLine = foldl (\ms m -> ms <> T.pack "\n- " <> m) "\n###### Updates performed" rewriteMsgs
+      maintainersCc =
+        if not (T.null maintainers)
+          then "\n\ncc " <> maintainers <> " for testing."
+          else ""
+      releaseUrlMessage =
+        if releaseUrl == T.empty
+          then ""
+          else "\n- [Release on GitHub](" <> releaseUrl <> ")"
+      compareUrlMessage =
+        if compareUrl == T.empty
+          then ""
+          else "\n- [Compare changes on GitHub](" <> compareUrl <> ")\n\n"
+      pat link = [interpolate|This update was made based on information from $link.|]
+      sourceLinkInfo = maybe "" pat $ sourceURL updateEnv
    in [interpolate|
        $title
 
        Semi-automatic update generated by [nixpkgs-update](https://github.com/ryantm/nixpkgs-update) tools. $sourceLinkInfo
        $brokenMsg
-       $metaDescription
-       $metaHomepage
-       $rewriteMessages
+       $metaDescriptionLine
+       $metaHomepageLine
+       $rewriteMsgsLine
 
        ###### To inspect upstream changes
 

test/RewriteSpec.hs

@@ -35,7 +35,7 @@ spec = do
             . Process.runPure ["\"http://troglobit.com/project/inadyn/\""]
             . Error.errorToIOFinal
             $ Rewrite.quotedUrls rwArgs
-        )
+          )
       T.putStrLn $ T.unlines logs
       head logs `shouldBe` "[quotedUrls]"
       result `shouldBe` Right (Just "Quoted meta.homepage for [RFC 45](https://github.com/NixOS/rfcs/pull/45)")

test/UpdateSpec.hs

@@ -0,0 +1,36 @@
+{-# LANGUAGE OverloadedStrings #-}
+
+module UpdateSpec where
+
+import qualified Data.Text.IO as T
+import Test.Hspec
+import qualified Update
+import qualified Utils
+
+main :: IO ()
+main = hspec spec
+
+spec :: Spec
+spec = do
+  describe "PR message" do
+    it "matches a simple mock example" do
+      expected <- T.readFile "test_data/expected_pr_description_1.md"
+      let options = Utils.Options False False "" False False False False
+      let updateEnv = Utils.UpdateEnv "foobar" "1.0" "1.1" (Just "https://update-site.com") options
+      let isBroken = False
+      let metaDescription = "\"Foobar package description\""
+      let metaHomepage = "\"https://foobar-homepage.com\""
+      let rewriteMsgs = ["Version Update", "Other Update"]
+      let releaseUrl = "https://github.com/foobar/releases"
+      let compareUrl = "https://github.com/foobar/compare"
+      let resultCheckReport = "- Some other check"
+      let commitHash = "af39cf77a0d42a4f6771043ec54221ed"
+      let attrPath = "foobar"
+      let maintainersCc = "@maintainer1"
+      let resultPath = "/nix/store/some-hash-path"
+      let opReport = "123 total rebuild path(s)"
+      let cveRep = ""
+      let cachixTestInstructions = ""
+      let nixpkgsReviewMsg = "nixpkgs-review comment body"
+      let actual = Update.prMessage updateEnv isBroken metaDescription metaHomepage rewriteMsgs releaseUrl compareUrl resultCheckReport commitHash attrPath maintainersCc resultPath opReport cveRep cachixTestInstructions nixpkgsReviewMsg
+      actual `shouldBe` expected

test_data/expected_pr_description_1.md

@@ -0,0 +1,89 @@
+foobar: 1.0 -> 1.1
+
+Semi-automatic update generated by [nixpkgs-update](https://github.com/ryantm/nixpkgs-update) tools. This update was made based on information from https://update-site.com.
+
+
+
+meta.description for foobar is: "Foobar package description"
+
+meta.homepage for foobar is: "https://foobar-homepage.com"
+
+###### Updates performed
+- Version Update
+- Other Update
+
+###### To inspect upstream changes
+
+
+- [Release on GitHub](https://github.com/foobar/releases)
+
+- [Compare changes on GitHub](https://github.com/foobar/compare)
+
+
+###### Impact
+
+<details>
+<summary>
+<b>Checks done</b> (click to expand)
+</summary>
+
+---
+
+- built on NixOS
+- Some other check
+
+---
+
+</details>
+<details>
+<summary>
+<b>Rebuild report</b> (if merged into master) (click to expand)
+</summary>
+
+```
+123 total rebuild path(s)
+```
+
+</details>
+
+<details>
+<summary>
+<b>Instructions to test this update</b> (click to expand)
+</summary>
+
+---
+
+
+```
+nix-build -A foobar https://github.com/r-ryantm/nixpkgs/archive/af39cf77a0d42a4f6771043ec54221ed.tar.gz
+```
+
+After you've downloaded or built it, look at the files and if there are any, run the binaries:
+```
+ls -la /nix/store/some-hash-path
+ls -la /nix/store/some-hash-path/bin
+```
+
+---
+
+</details>
+<br/>
+
+
+
+# Pre-merge build results
+
+We have automatically built all packages that will get rebuilt due to this change.
+
+This gives evidence on whether the upgrade will break dependent packages.
+Note sometimes packages show up as _failed to build_ independent of the change, simply because they are already broken on the target branch.
+
+nixpkgs-review comment body
+
+---
+
+###### Maintainer pings
+
+
+
+cc @maintainer1 for testing.