From 286f040859b1f4cbbe0a243bcb60a4553a254ec7 Mon Sep 17 00:00:00 2001 From: Benjamin Hipple Date: Wed, 22 Apr 2020 23:32:07 -0400 Subject: [PATCH] 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 | 3 +- src/Update.hs | 94 +++++++++++++------------- test/RewriteSpec.hs | 2 +- test/UpdateSpec.hs | 36 ++++++++++ test_data/expected_pr_description_1.md | 89 ++++++++++++++++++++++++ 5 files changed, 176 insertions(+), 48 deletions(-) create mode 100644 test/UpdateSpec.hs create mode 100644 test_data/expected_pr_description_1.md diff --git a/nixpkgs-update.cabal b/nixpkgs-update.cabal index cbc7551..40a9498 100644 --- a/nixpkgs-update.cabal +++ b/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 diff --git a/src/Update.hs b/src/Update.hs index dd5e87e..b6ae6c1 100644 --- a/src/Update.hs +++ b/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 diff --git a/test/RewriteSpec.hs b/test/RewriteSpec.hs index b3d4a12..cea1c31 100644 --- a/test/RewriteSpec.hs +++ b/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)") diff --git a/test/UpdateSpec.hs b/test/UpdateSpec.hs new file mode 100644 index 0000000..76c2274 --- /dev/null +++ b/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 diff --git a/test_data/expected_pr_description_1.md b/test_data/expected_pr_description_1.md new file mode 100644 index 0000000..f2a51f5 --- /dev/null +++ b/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 + +
+ +Checks done (click to expand) + + +--- + +- built on NixOS +- Some other check + +--- + +
+
+ +Rebuild report (if merged into master) (click to expand) + + +``` +123 total rebuild path(s) +``` + +
+ +
+ +Instructions to test this update (click to expand) + + +--- + + +``` +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 +``` + +--- + +
+
+ + + +# 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.