From 7f312dc1ff1c9cfe342f21776e1f160fd0ac4be2 Mon Sep 17 00:00:00 2001 From: Benjamin Hipple Date: Sun, 3 May 2020 12:05:21 -0400 Subject: [PATCH] Hide section about NixPkgs review when it was skipped We're no longer running this on `staging` PRs, and users doing single-package updates have the option to omit `nixpkgs-review`, so we should only include the PR comment section if it was actually run. This continues to enhance the automated tests for the PR comment formatting, so you can see what the diff looks like just by looking at the test suite! --- .gitignore | 9 +-- src/Update.hs | 22 ++++--- test/UpdateSpec.hs | 48 +++++++++------ test_data/expected_pr_description_1.md | 8 ++- test_data/expected_pr_description_2.md | 82 ++++++++++++++++++++++++++ 5 files changed, 138 insertions(+), 31 deletions(-) create mode 100644 test_data/expected_pr_description_2.md diff --git a/.gitignore b/.gitignore index 1f2f806..0ca305d 100644 --- a/.gitignore +++ b/.gitignore @@ -1,8 +1,9 @@ +.ghc* /github_token.txt +/nixpkgs-update.nix /packages-to-update.txt /result -dist/ dist-newstyle/ -/nixpkgs-update.nix -.ghc* -store-path-pre-build \ No newline at end of file +dist/ +store-path-pre-build +test_data/actual* \ No newline at end of file diff --git a/src/Update.hs b/src/Update.hs index b39a4dc..76e33b0 100644 --- a/src/Update.hs +++ b/src/Update.hs @@ -394,6 +394,19 @@ prMessage updateEnv isBroken metaDescription metaHomepage rewriteMsgs releaseUrl if compareUrl == T.empty then "" else "\n- [Compare changes on GitHub](" <> compareUrl <> ")\n\n" + nixpkgsReviewSection = + if nixpkgsReviewMsg == T.empty + then "NixPkgs review skipped" + else [interpolate| + 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. + + $nixpkgsReviewMsg + |] pat link = [interpolate|This update was made based on information from $link.|] sourceLinkInfo = maybe "" pat $ sourceURL updateEnv in [interpolate| @@ -459,14 +472,9 @@ prMessage updateEnv isBroken metaDescription metaHomepage rewriteMsgs releaseUrl $cveRep - # Pre-merge build results + ### 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. - - $nixpkgsReviewMsg + $nixpkgsReviewSection --- diff --git a/test/UpdateSpec.hs b/test/UpdateSpec.hs index 76c2274..6c0f9a0 100644 --- a/test/UpdateSpec.hs +++ b/test/UpdateSpec.hs @@ -10,27 +10,41 @@ import qualified Utils main :: IO () main = hspec spec +-- Tests here will write their actual results to an actual*.md file next to the +-- expected one. If you are updating the PR description, run the test suite and +-- copy all the actuals over to expected. This can be viewed with `git diff` +-- then to ensure your changes look as expected. spec :: Spec spec = do describe "PR message" do + -- Common mock options + 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" + 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 + T.writeFile "test_data/actual_pr_description_1.md" actual + actual `shouldBe` expected + + it "does not include NixPkgs review section when no review was done" do + expected <- T.readFile "test_data/expected_pr_description_2.md" + let nixpkgsReviewMsg' = "" + let actual = Update.prMessage updateEnv isBroken metaDescription metaHomepage rewriteMsgs releaseUrl compareUrl resultCheckReport commitHash attrPath maintainersCc resultPath opReport cveRep cachixTestInstructions nixpkgsReviewMsg' + T.writeFile "test_data/actual_pr_description_2.md" actual actual `shouldBe` expected diff --git a/test_data/expected_pr_description_1.md b/test_data/expected_pr_description_1.md index 538fc8b..d0d4f82 100644 --- a/test_data/expected_pr_description_1.md +++ b/test_data/expected_pr_description_1.md @@ -69,12 +69,14 @@ ls -la /nix/store/some-hash-path/bin -# Pre-merge build results +### Pre-merge build results -We have automatically built all packages that will get rebuilt due to this change. +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. +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 diff --git a/test_data/expected_pr_description_2.md b/test_data/expected_pr_description_2.md new file mode 100644 index 0000000..d23220f --- /dev/null +++ b/test_data/expected_pr_description_2.md @@ -0,0 +1,82 @@ +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 + +NixPkgs review skipped + +--- + +###### Maintainer pings + + + +cc @maintainer1 for testing.