From d7759666e675c813ad6145d74afe0070d5ac541e Mon Sep 17 00:00:00 2001 From: Jordan Martinez Date: Mon, 16 Oct 2023 13:03:00 -0500 Subject: [PATCH 01/11] Deduplicate test deps; report what's actually needed --- src/Spago/Purs/Graph.purs | 142 ++++++++++++---------- test-fixtures/check-pedantic-packages.txt | 41 +++++++ test/Spago/Build.purs | 100 +++++++++++++-- test/Spago/Build/Polyrepo.purs | 14 ++- 4 files changed, 223 insertions(+), 74 deletions(-) create mode 100644 test-fixtures/check-pedantic-packages.txt diff --git a/src/Spago/Purs/Graph.purs b/src/Spago/Purs/Graph.purs index 07243b3f8..0bff6dfeb 100644 --- a/src/Spago/Purs/Graph.purs +++ b/src/Spago/Purs/Graph.purs @@ -183,68 +183,86 @@ checkImports graph = do packageGraph <- runSpago (Record.union { selected: NEA.singleton selected } env) $ getModuleGraphWithPackage graph let - packageName = selected.package.name - testPackageName = unsafeCoerce (PackageName.print packageName <> ":test") - - declaredDependencies = unwrap selected.package.dependencies - declaredTestDependencies = maybe Map.empty (unwrap <<< _.dependencies) selected.package.test - - -- The direct dependencies specified in the config - dependencyPackages = Map.mapMaybe (const (Just Map.empty)) declaredDependencies - dependencyTestPackages = Map.mapMaybe (const (Just Map.empty)) $ Map.union declaredDependencies declaredTestDependencies - - -- Compile the globs for the project, we get the set of source files in the project - projectGlob :: Set FilePath <- map Set.fromFoldable do - map Array.fold $ traverse compileGlob (Config.sourceGlob NoTestGlobs packageName (WorkspacePackage selected)) - - -- Same but for tests - projectTestsGlob :: Set FilePath <- map Set.fromFoldable do - map Array.fold $ traverse compileGlob (Config.sourceGlob OnlyTestGlobs packageName (WorkspacePackage selected)) - - let - -- Filter this improved graph to only have the project modules - projectGraph = Map.filterWithKey (\_ { path } -> Set.member path projectGlob) packageGraph - projectTestsGraph = Map.filterWithKey (\_ { path } -> Set.member path projectTestsGlob) packageGraph - - -- Go through all the modules in the project graph, figure out which packages each module depends on, - -- accumulate all of that in a single place - accumulateImported importedPkgs' (Tuple moduleName { depends }) = - let - accumulateDep importedPkgs importedModule = case Map.lookup importedModule packageGraph of - Nothing -> importedPkgs - -- Skip dependencies on modules in the same package, we are not interested in that - Just { package } | package == packageName -> importedPkgs - Just { package } | package == testPackageName -> importedPkgs - Just { package: importedPackage } -> Map.alter - ( case _ of - Nothing -> Just $ Map.singleton moduleName (Set.singleton importedModule) - Just p -> Just $ Map.alter - ( case _ of - Nothing -> Just $ Set.singleton importedModule - Just set -> Just $ Set.insert importedModule set - ) - moduleName - p - ) - importedPackage - importedPkgs - in - foldl accumulateDep importedPkgs' depends - - importedPackages :: ImportedPackages - importedPackages = foldl accumulateImported Map.empty (Map.toUnfoldable projectGraph :: Array _) - importedTestPackages = foldl accumulateImported Map.empty (Map.toUnfoldable projectTestsGraph :: Array _) - - unused = Map.keys $ Map.difference dependencyPackages importedPackages - transitive = Map.difference importedPackages dependencyPackages - unusedTest = - if Set.isEmpty projectTestsGlob then Set.empty - else Map.keys $ Map.difference (Map.difference dependencyTestPackages dependencyPackages) importedTestPackages - transitiveTest = - if Set.isEmpty projectTestsGlob then Map.empty - else Map.difference importedTestPackages dependencyTestPackages - - pure { unused, transitive, unusedTest, transitiveTest } + dropValues = Map.mapMaybe (const (Just Map.empty)) + srcDeps = unwrap selected.package.dependencies + srcResult <- getUsedUnusedTransitiveFor + { selected + , packageName: selected.package.name + , dependencyPackages: dropValues srcDeps + , isSrc: true + , packageGraph + } + let srcDepsUsed = Map.filterKeys (flip Map.member srcResult.used) srcDeps + testResult <- getUsedUnusedTransitiveFor + { selected + , packageName: selected.package.name + , dependencyPackages: + dropValues + -- add the used source dependencies, not the declared ones. + $ Map.unionWith const srcDepsUsed + -- get test deps + $ maybe Map.empty (unwrap <<< _.dependencies) selected.package.test + , isSrc: false + , packageGraph + } + + pure + { unused: srcResult.unused + , transitive: srcResult.transitive + , unusedTest: Set.difference testResult.unused $ Map.keys srcResult.used + , transitiveTest: differenceAll testResult.transitive [ srcResult.used, srcResult.transitive ] + } + where + differenceAll sourceMap removalsArray = foldl Map.difference sourceMap removalsArray + getUsedUnusedTransitiveFor { selected, packageName, dependencyPackages, isSrc, packageGraph } = do + let + testPackageName = unsafeCoerce (PackageName.print packageName <> ":test") + + testGlobOption + | isSrc = NoTestGlobs + | otherwise = OnlyTestGlobs + + -- Compile the globs for the project, we get the set of source files in the project + glob :: Set FilePath <- map Set.fromFoldable do + map Array.fold $ traverse compileGlob (Config.sourceGlob testGlobOption packageName (WorkspacePackage selected)) + + let + -- Filter this improved graph to only have the project modules + projectGraph = Map.filterWithKey (\_ { path } -> Set.member path glob) packageGraph + + -- Go through all the modules in the project graph, figure out which packages each module depends on, + -- accumulate all of that in a single place + accumulateImported importedPkgs' (Tuple moduleName { depends }) = + let + accumulateDep importedPkgs importedModule = case Map.lookup importedModule packageGraph of + Nothing -> importedPkgs + -- Skip dependencies on modules in the same package, we are not interested in that + Just { package } | package == packageName -> importedPkgs + Just { package } | package == testPackageName -> importedPkgs + Just { package: importedPackage } -> Map.alter + ( case _ of + Nothing -> Just $ Map.singleton moduleName (Set.singleton importedModule) + Just p -> Just $ Map.alter + ( case _ of + Nothing -> Just $ Set.singleton importedModule + Just set -> Just $ Set.insert importedModule set + ) + moduleName + p + ) + importedPackage + importedPkgs + in + foldl accumulateDep importedPkgs' depends + + importedPackages :: ImportedPackages + importedPackages = foldl accumulateImported Map.empty (Map.toUnfoldable projectGraph :: Array _) + + pure + { used: if isSrc then Map.intersection dependencyPackages importedPackages else Map.empty + , unused: Map.keys $ Map.difference dependencyPackages importedPackages + , transitive: Map.difference importedPackages dependencyPackages + } -------------------------------------------------------------------------------- -- Errors diff --git a/test-fixtures/check-pedantic-packages.txt b/test-fixtures/check-pedantic-packages.txt new file mode 100644 index 000000000..6b7619495 --- /dev/null +++ b/test-fixtures/check-pedantic-packages.txt @@ -0,0 +1,41 @@ +Reading Spago workspace configuration... +Read the package set from the registry + +✅ Selecting package to build: foo + +Downloading dependencies... +Building... + Src Lib All +Warnings 0 0 0 +Errors 0 0 0 + +✅ Build succeeded. + +Looking for unused and undeclared transitive dependencies... + +❌ Sources for package 'foo' declares unused dependencies - please remove them from the project config: + - either + + +❌ Sources for package 'foo' import the following transitive dependencies - please add them to the project dependencies, or remove the imports: + newtype + from `Main`, which imports: + Data.Newtype + + +Run the following command to install them all: + spago install -p foo newtype + + +❌ Tests for package 'foo' declares unused dependencies - please remove them from the project config: + - tuples + + +❌ Tests for package 'foo' import the following transitive dependencies - please add them to the project dependencies, or remove the imports: + either + from `Test.Main`, which imports: + Data.Either + + +Run the following command to install them all: + spago install --test-deps -p foo either diff --git a/test/Spago/Build.purs b/test/Spago/Build.purs index 9d9c7b467..3e7775fe9 100644 --- a/test/Spago/Build.purs +++ b/test/Spago/Build.purs @@ -2,6 +2,8 @@ module Test.Spago.Build where import Test.Prelude +import Data.Array as Array +import Data.Map as Map import Node.FS.Aff as FSA import Node.Path as Path import Node.Process as Process @@ -78,7 +80,7 @@ spec = Spec.around withTempDir do FS.exists "output" `Assert.shouldReturn` true FS.exists (Path.concat [ "subpackage", "output" ]) `Assert.shouldReturn` false - Spec.describe "pedantic packages" do + Spec.describeOnly "pedantic packages" do let modifyPackageConfig f = do @@ -102,36 +104,40 @@ spec = Spec.around withTempDir do Spec.describe "fails when there are imports from transitive dependencies and" do let - setupSrcTransitiveTests spago = do + setupSrcTransitiveTests spago installConsoleAndEffect = do spago [ "init", "--name", "7368613235362d34312f4e59746b7869335477336d33414d72" ] >>= shouldBeSuccess spago [ "install", "maybe" ] >>= shouldBeSuccess + when installConsoleAndEffect do + spago [ "install", "--test-deps", "console", "effect" ] >>= shouldBeSuccess FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) "module Main where\nimport Prelude\nimport Data.Maybe\nimport Control.Alt\nmain = unit" -- get rid of "Compiling ..." messages and other compiler warnings spago [ "build" ] >>= shouldBeSuccess Spec.it "passed --pedantic-packages CLI flag" \{ spago, fixture } -> do - setupSrcTransitiveTests spago + setupSrcTransitiveTests spago true spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-direct-import-transitive-dependency.txt") Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do - setupSrcTransitiveTests spago + setupSrcTransitiveTests spago false addPedanticPackagesToSrc spago [ "build" ] >>= shouldBeFailureErr (fixture "check-direct-import-transitive-dependency.txt") Spec.describe "warns about unused dependencies when" do let - setupSrcUnusedDeps spago = do + setupSrcUnusedDeps spago installConsoleAndEffect = do spago [ "init", "--name", "7368613235362d2f444a2b4f56375435646a59726b53586548" ] >>= shouldBeSuccess + when installConsoleAndEffect do + spago [ "install", "--test-deps", "console", "effect" ] >>= shouldBeSuccess FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) "module Main where\nimport Prelude\nmain = unit" -- get rid of "Compiling ..." messages and other compiler warnings spago [ "build" ] >>= shouldBeSuccess Spec.it "passing --pedantic-packages CLI flag" \{ spago, fixture } -> do - setupSrcUnusedDeps spago + setupSrcUnusedDeps spago true spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-unused-dependency.txt") Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do - setupSrcUnusedDeps spago + setupSrcUnusedDeps spago false addPedanticPackagesToSrc spago [ "build" ] >>= shouldBeFailureErr (fixture "check-unused-dependency.txt") @@ -159,6 +165,86 @@ spec = Spec.around withTempDir do modifyPkgTestConfig true spago [ "build" ] >>= shouldBeFailureErr (fixture "check-unused-test-dependency.txt") + {- + Goal: + Src + - remove either - unused + - install newtype - install `newtype` (transitive dep of either) as it will be needed once either is removed + Test + - remove tuples - unused + - install either - install `either` (implicit dep of source package, but need to add it here once removed from source) + -} + Spec.describe "fails to build and reports deduplicated src and test unused/transitive dependenciess when" do + let + addPedanticPackages = do + modifyPackageConfig \config -> + config + { package = config.package <#> \r -> r + { build = Just + { pedantic_packages: Just true + , strict: r.build >>= _.strict + , censor_project_warnings: r.build >>= _.censor_project_warnings + } + , test = Just + { main: "Test.Main" + , pedantic_packages: Just true + , strict: r.test >>= _.strict + , censor_test_warnings: r.test >>= _.censor_test_warnings + , dependencies: maybe (Config.Dependencies Map.empty) _.dependencies r.test + , execArgs: r.test >>= _.execArgs + } + } + } + + setupUnusedAndTransitiveDeps spago = do + -- Since we need to edit `spago.yaml` and other files generated by `spago init`, + -- may as well just create those files by hand. + FS.writeYamlFile configCodec "spago.yaml" $ mkPackageAndWorkspaceConfig + { package: { packageName: "foo", srcDependencies: [ "prelude", "control", "either" ] } + , workspace: { setVersion: Just $ mkVersion "0.0.1" } + } + [ configAddTestMain + , configAddTestDependencies [ "tuples" ] + ] + + FS.mkdirp "src" + FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) $ Array.intercalate "\n" + [ "module Main where " + , "" + , "import Prelude" + , "import Data.Newtype as Newtype" + , "import Control.Alt as Alt" + , "" + , "foo :: Int" + , "foo = 1" + ] + + FS.mkdirp $ Path.concat [ "test", "Test" ] + FS.writeTextFile (Path.concat [ "test", "Test", "Main.purs" ]) $ Array.intercalate "\n" + [ "module Test.Main where " + , "" + , "import Prelude" + , "import Data.Newtype (class Newtype)" + , "import Data.Either (Either(..))" + , "" + , "newtype Bar = Bar Int" + , "derive instance Newtype Bar _" + , "" + , "foo :: Either Bar Int" + , "foo = Right 1" + ] + -- get rid of "Compiling ..." messages and other compiler warnings + spago [ "build" ] >>= shouldBeSuccess + + Spec.it "passing --pedantic-packages" \{ spago, fixture } -> do + setupUnusedAndTransitiveDeps spago + spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-pedantic-packages.txt") + + Spec.it "package config has `pedantic_packages: true` in both `build` and `test`" \{ spago, fixture } -> do + setupUnusedAndTransitiveDeps spago + addPedanticPackages + spago [ "build" ] >>= shouldBeFailureErr (fixture "check-pedantic-packages.txt") + Spec.it "--strict causes build to fail if there are warnings" \{ spago, fixture } -> do spago [ "init" ] >>= shouldBeSuccess let srcMain = Path.concat [ "src", "Main.purs" ] diff --git a/test/Spago/Build/Polyrepo.purs b/test/Spago/Build/Polyrepo.purs index 9d4bc9531..fd74561f1 100644 --- a/test/Spago/Build/Polyrepo.purs +++ b/test/Spago/Build/Polyrepo.purs @@ -364,7 +364,7 @@ spec = Spec.describe "polyrepo" do { modName: mkTestModuleName packageName , imports: [ "import Control.Alt as Control" - , "import Data.Newtype as Newtype" + , "import Safe.Coerce as Coerce" ] , body: [ "" @@ -383,7 +383,11 @@ spec = Spec.describe "polyrepo" do [ toMsgPrefix isSrc <> " for package '" <> package <> "' declares unused dependencies - please remove them from the project config:" , " - " <> (if isSrc then "tuples" else "either") ] - mkTransitiveDepErr isSrc package = + mkTransitiveDepErr isSrc package = do + let + { pkg, mkModName, pkgModName } = + if isSrc then { pkg: "newtype", mkModName: mkSrcModuleName, pkgModName: "Data.Newtype" } + else { pkg: "safe-coerce", mkModName: mkTestModuleName, pkgModName: "Safe.Coerce" } Array.intercalate "\n" [ Array.fold [ toMsgPrefix isSrc @@ -391,9 +395,9 @@ spec = Spec.describe "polyrepo" do , package , "' import the following transitive dependencies - please add them to the project dependencies, or remove the imports:" ] - , " newtype" - , " from `" <> (if isSrc then mkSrcModuleName else mkTestModuleName) package <> "`, which imports:" - , " Data.Newtype" + , " " <> pkg + , " from `" <> mkModName package <> "`, which imports:" + , " " <> pkgModName ] Spec.it "when package config has 'pedantic_packages: true', build fails with expected errors" \{ spago } -> do writeWorkspaceOnlySpagoYamlFile From 43020f429632b236029c8783d467c1e107e9db52 Mon Sep 17 00:00:00 2001 From: Jordan Martinez Date: Mon, 16 Oct 2023 13:09:36 -0500 Subject: [PATCH 02/11] Drop describeOnly --- test/Spago/Build.purs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Spago/Build.purs b/test/Spago/Build.purs index 3e7775fe9..2a4d27a91 100644 --- a/test/Spago/Build.purs +++ b/test/Spago/Build.purs @@ -80,7 +80,7 @@ spec = Spec.around withTempDir do FS.exists "output" `Assert.shouldReturn` true FS.exists (Path.concat [ "subpackage", "output" ]) `Assert.shouldReturn` false - Spec.describeOnly "pedantic packages" do + Spec.describe "pedantic packages" do let modifyPackageConfig f = do From 97b3e5b60558a8b1192c97565499bc65f15fc5c9 Mon Sep 17 00:00:00 2001 From: Jordan Martinez Date: Mon, 16 Oct 2023 13:10:29 -0500 Subject: [PATCH 03/11] Update comments --- test/Spago/Build.purs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/Spago/Build.purs b/test/Spago/Build.purs index 2a4d27a91..c05435ceb 100644 --- a/test/Spago/Build.purs +++ b/test/Spago/Build.purs @@ -169,10 +169,10 @@ spec = Spec.around withTempDir do Goal: Src - remove either - unused - - install newtype - install `newtype` (transitive dep of either) as it will be needed once either is removed + - install newtype - (transitive dep of either) as it will be needed once either is removed Test - remove tuples - unused - - install either - install `either` (implicit dep of source package, but need to add it here once removed from source) + - install either - (implicit dep of source package, but need to add it here once removed from source) -} Spec.describe "fails to build and reports deduplicated src and test unused/transitive dependenciess when" do let From 4588d353e83134bbc94a37af9e527299fe450774 Mon Sep 17 00:00:00 2001 From: Jordan Martinez Date: Tue, 17 Oct 2023 15:06:16 -0500 Subject: [PATCH 04/11] Add more individual tests --- ...rect-import-transitive-dependency-test.txt | 23 +++ ...ck-direct-import-transitive-dependency.txt | 5 - test/Spago/Build.purs | 173 ++++++++++++------ 3 files changed, 141 insertions(+), 60 deletions(-) create mode 100644 test-fixtures/check-direct-import-transitive-dependency-test.txt diff --git a/test-fixtures/check-direct-import-transitive-dependency-test.txt b/test-fixtures/check-direct-import-transitive-dependency-test.txt new file mode 100644 index 000000000..97c52ddab --- /dev/null +++ b/test-fixtures/check-direct-import-transitive-dependency-test.txt @@ -0,0 +1,23 @@ +Reading Spago workspace configuration... +Read the package set from the registry + +✅ Selecting package to build: 7368613235362d34312f4e59746b7869335477336d33414d72 + +Downloading dependencies... +Building... + Src Lib All +Warnings 0 0 0 +Errors 0 0 0 + +✅ Build succeeded. + +Looking for unused and undeclared transitive dependencies... + +❌ Tests for package '7368613235362d34312f4e59746b7869335477336d33414d72' import the following transitive dependencies - please add them to the project dependencies, or remove the imports: + control + from `Test.Main`, which imports: + Control.Alt + + +Run the following command to install them all: + spago install --test-deps -p 7368613235362d34312f4e59746b7869335477336d33414d72 control diff --git a/test-fixtures/check-direct-import-transitive-dependency.txt b/test-fixtures/check-direct-import-transitive-dependency.txt index e77ac94da..638330861 100644 --- a/test-fixtures/check-direct-import-transitive-dependency.txt +++ b/test-fixtures/check-direct-import-transitive-dependency.txt @@ -13,11 +13,6 @@ Errors 0 0 0 Looking for unused and undeclared transitive dependencies... -❌ Sources for package '7368613235362d34312f4e59746b7869335477336d33414d72' declares unused dependencies - please remove them from the project config: - - console - - effect - - ❌ Sources for package '7368613235362d34312f4e59746b7869335477336d33414d72' import the following transitive dependencies - please add them to the project dependencies, or remove the imports: control from `Main`, which imports: diff --git a/test/Spago/Build.purs b/test/Spago/Build.purs index c05435ceb..a7ebf4883 100644 --- a/test/Spago/Build.purs +++ b/test/Spago/Build.purs @@ -101,69 +101,132 @@ spec = Spec.around withTempDir do } } } + addPedanticPackagesToTest = modifyPackageConfig \config -> + config + { package = config.package <#> \r -> r + { test = Just + { main: "Test.Main" + , pedantic_packages: Just true + , strict: Nothing + , censor_test_warnings: Nothing + , dependencies: maybe (Config.Dependencies Map.empty) _.dependencies r.test + , execArgs: r.test >>= _.execArgs + } + } + } - Spec.describe "fails when there are imports from transitive dependencies and" do - let - setupSrcTransitiveTests spago installConsoleAndEffect = do - spago [ "init", "--name", "7368613235362d34312f4e59746b7869335477336d33414d72" ] >>= shouldBeSuccess - spago [ "install", "maybe" ] >>= shouldBeSuccess - when installConsoleAndEffect do - spago [ "install", "--test-deps", "console", "effect" ] >>= shouldBeSuccess - FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) "module Main where\nimport Prelude\nimport Data.Maybe\nimport Control.Alt\nmain = unit" - -- get rid of "Compiling ..." messages and other compiler warnings - spago [ "build" ] >>= shouldBeSuccess + Spec.describe "fails when imports from transitive dependencies" do - Spec.it "passed --pedantic-packages CLI flag" \{ spago, fixture } -> do - setupSrcTransitiveTests spago true - spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-direct-import-transitive-dependency.txt") + Spec.describe "appear in the source package and" do - Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do - setupSrcTransitiveTests spago false - addPedanticPackagesToSrc - spago [ "build" ] >>= shouldBeFailureErr (fixture "check-direct-import-transitive-dependency.txt") + let + setupSrcTransitiveTests spago installConsoleAndEffect = do + spago [ "init", "--name", "7368613235362d34312f4e59746b7869335477336d33414d72" ] >>= shouldBeSuccess + spago [ "install", "maybe" ] >>= shouldBeSuccess + when installConsoleAndEffect do + spago [ "install", "--test-deps", "console", "effect" ] >>= shouldBeSuccess + FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) $ Array.intercalate "\n" + [ "module Main where" + , "import Prelude" + , "import Data.Maybe as Maybe" + , "import Effect as Effect" + , "import Effect.Console as Console" + , "import Control.Alt as Alt" + , "main = unit" + ] + -- get rid of "Compiling ..." messages and other compiler warnings + spago [ "build" ] >>= shouldBeSuccess + + Spec.it "passed --pedantic-packages CLI flag" \{ spago, fixture } -> do + setupSrcTransitiveTests spago true + spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-direct-import-transitive-dependency.txt") + + Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do + setupSrcTransitiveTests spago false + addPedanticPackagesToSrc + spago [ "build" ] >>= shouldBeFailureErr (fixture "check-direct-import-transitive-dependency.txt") + + Spec.describe "appear in the test package and" do - Spec.describe "warns about unused dependencies when" do - let - setupSrcUnusedDeps spago installConsoleAndEffect = do - spago [ "init", "--name", "7368613235362d2f444a2b4f56375435646a59726b53586548" ] >>= shouldBeSuccess - when installConsoleAndEffect do - spago [ "install", "--test-deps", "console", "effect" ] >>= shouldBeSuccess - FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) "module Main where\nimport Prelude\nmain = unit" - -- get rid of "Compiling ..." messages and other compiler warnings - spago [ "build" ] >>= shouldBeSuccess + let + setupTestTransitiveTests spago = do + spago [ "init", "--name", "7368613235362d34312f4e59746b7869335477336d33414d72" ] >>= shouldBeSuccess + spago [ "install", "maybe" ] >>= shouldBeSuccess + FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) $ Array.intercalate "\n" + [ "module Main where" + , "import Prelude" + , "import Data.Maybe as Maybe" + , "import Effect as Effect" + , "import Effect.Console as Console" + , "main = unit" + ] + FS.writeTextFile (Path.concat [ "test", "Test", "Main.purs" ]) $ Array.intercalate "\n" + [ "module Test.Main where" + , "import Prelude" + , "import Data.Maybe as Maybe" + , "import Effect as Effect" + , "import Effect.Console as Console" + , "import Control.Alt as Alt" + , "main = unit" + ] + -- get rid of "Compiling ..." messages and other compiler warnings + spago [ "build" ] >>= shouldBeSuccess + + Spec.it "passed --pedantic-packages CLI flag" \{ spago, fixture } -> do + setupTestTransitiveTests spago + spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-direct-import-transitive-dependency-test.txt") + + Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do + setupTestTransitiveTests spago + addPedanticPackagesToTest + spago [ "build" ] >>= shouldBeFailureErr (fixture "check-direct-import-transitive-dependency-test.txt") + + Spec.describe "fails with warnings about unused dependencies" do + + Spec.describe "in a source package when" do + + let + setupSrcUnusedDeps spago installConsoleAndEffect = do + spago [ "init", "--name", "7368613235362d2f444a2b4f56375435646a59726b53586548" ] >>= shouldBeSuccess + when installConsoleAndEffect do + spago [ "install", "--test-deps", "console", "effect" ] >>= shouldBeSuccess + FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) "module Main where\nimport Prelude\nmain = unit" + -- get rid of "Compiling ..." messages and other compiler warnings + spago [ "build" ] >>= shouldBeSuccess + + Spec.it "passing --pedantic-packages CLI flag" \{ spago, fixture } -> do + setupSrcUnusedDeps spago true + spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-unused-dependency.txt") - Spec.it "passing --pedantic-packages CLI flag" \{ spago, fixture } -> do - setupSrcUnusedDeps spago true - spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-unused-dependency.txt") + Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do + setupSrcUnusedDeps spago false + addPedanticPackagesToSrc + spago [ "build" ] >>= shouldBeFailureErr (fixture "check-unused-dependency.txt") - Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do - setupSrcUnusedDeps spago false - addPedanticPackagesToSrc - spago [ "build" ] >>= shouldBeFailureErr (fixture "check-unused-dependency.txt") + Spec.describe "in a test package when" do - Spec.it "package's test config has 'pedantic_packages: true' and test code has unused dependencies" \{ spago, fixture } -> do - spago [ "init", "--name", "7368613235362d2f444a2b4f56375435646a59726b53586548" ] >>= shouldBeSuccess - FS.writeTextFile (Path.concat [ "test", "Test", "Main.purs" ]) "module Test.Main where\nimport Prelude\nmain = unit" let - modifyPkgTestConfig pedantic = modifyPackageConfig \config -> - config - { package = config.package <#> \r -> r - { test = Just - { main: "Test.Main" - , pedantic_packages: Just pedantic - , strict: Nothing - , censor_test_warnings: Nothing - , dependencies: mkDependencies [ "newtype" ] - , execArgs: Nothing - } - } - } - modifyPkgTestConfig false - -- get rid of "Compiling ..." messages and other compiler warnings - spago [ "build" ] >>= shouldBeSuccess - -- now add pedantic - modifyPkgTestConfig true - spago [ "build" ] >>= shouldBeFailureErr (fixture "check-unused-test-dependency.txt") + setupTestUnusedDeps spago = do + spago [ "init", "--name", "7368613235362d2f444a2b4f56375435646a59726b53586548" ] >>= shouldBeSuccess + spago [ "install", "--test-deps", "newtype" ] >>= shouldBeSuccess + FS.writeTextFile (Path.concat [ "test", "Test", "Main.purs" ]) $ Array.intercalate "\n" + [ "module Test.Main where" + , "import Prelude" + , "import Effect as Effect" + , "import Effect.Console as Console" + , "main = unit" + ] + -- get rid of "Compiling ..." messages and other compiler warnings + spago [ "build" ] >>= shouldBeSuccess + + Spec.it "passing --pedantic-packages CLI flag" \{ spago, fixture } -> do + setupTestUnusedDeps spago + spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-unused-test-dependency.txt") + + Spec.it "package's test config has 'pedantic_packages: true'" \{ spago, fixture } -> do + setupTestUnusedDeps spago + addPedanticPackagesToTest + spago [ "build" ] >>= shouldBeFailureErr (fixture "check-unused-test-dependency.txt") {- Goal: From 12dee7eff78905bd5abeb3a5f91a4cdb5b9a4d34 Mon Sep 17 00:00:00 2001 From: Jordan Martinez Date: Tue, 17 Oct 2023 15:37:41 -0500 Subject: [PATCH 05/11] Add unused in src and test packages test --- ...heck-unused-source-and-test-dependency.txt | 23 +++++++++++++++++++ test/Spago/Build.purs | 22 ++++++++++++++++++ 2 files changed, 45 insertions(+) create mode 100644 test-fixtures/check-unused-source-and-test-dependency.txt diff --git a/test-fixtures/check-unused-source-and-test-dependency.txt b/test-fixtures/check-unused-source-and-test-dependency.txt new file mode 100644 index 000000000..0de0f2d63 --- /dev/null +++ b/test-fixtures/check-unused-source-and-test-dependency.txt @@ -0,0 +1,23 @@ +Reading Spago workspace configuration... +Read the package set from the registry + +✅ Selecting package to build: 7368613235362d2f444a2b4f56375435646a59726b53586548 + +Downloading dependencies... +Building... + Src Lib All +Warnings 0 0 0 +Errors 0 0 0 + +✅ Build succeeded. + +Looking for unused and undeclared transitive dependencies... + +❌ Sources for package '7368613235362d2f444a2b4f56375435646a59726b53586548' declares unused dependencies - please remove them from the project config: + - console + - effect + + +❌ Tests for package '7368613235362d2f444a2b4f56375435646a59726b53586548' declares unused dependencies - please remove them from the project config: + - console + - effect diff --git a/test/Spago/Build.purs b/test/Spago/Build.purs index a7ebf4883..219ce3612 100644 --- a/test/Spago/Build.purs +++ b/test/Spago/Build.purs @@ -228,6 +228,28 @@ spec = Spec.around withTempDir do addPedanticPackagesToTest spago [ "build" ] >>= shouldBeFailureErr (fixture "check-unused-test-dependency.txt") + Spec.describe "in both the source and test packages when" do + + let + setupUnusedDeps spago = do + spago [ "init", "--name", "7368613235362d2f444a2b4f56375435646a59726b53586548" ] >>= shouldBeSuccess + spago [ "install", "prelude", "effect", "console" ] >>= shouldBeSuccess + spago [ "install", "--test-deps", "prelude", "effect", "console" ] >>= shouldBeSuccess + FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) "module Main where\nimport Prelude\nmain = unit" + FS.writeTextFile (Path.concat [ "test", "Test", "Main.purs" ]) "module Test.Main where\nimport Prelude\nmain = unit" + -- get rid of "Compiling ..." messages and other compiler warnings + spago [ "build" ] >>= shouldBeSuccess + + Spec.it "passing --pedantic-packages CLI flag" \{ spago, fixture } -> do + setupUnusedDeps spago + spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-unused-source-and-test-dependency.txt") + + Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do + setupUnusedDeps spago + addPedanticPackagesToSrc + addPedanticPackagesToTest + spago [ "build" ] >>= shouldBeFailureErr (fixture "check-unused-source-and-test-dependency.txt") + {- Goal: Src From 0276cd82a7fbd40e0bf62c88119e395c6cbdcb87 Mon Sep 17 00:00:00 2001 From: Jordan Martinez Date: Wed, 18 Oct 2023 07:16:16 -0500 Subject: [PATCH 06/11] Use record args; add comments --- test/Spago/Build.purs | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/test/Spago/Build.purs b/test/Spago/Build.purs index 219ce3612..ce60cd259 100644 --- a/test/Spago/Build.purs +++ b/test/Spago/Build.purs @@ -120,10 +120,10 @@ spec = Spec.around withTempDir do Spec.describe "appear in the source package and" do let - setupSrcTransitiveTests spago installConsoleAndEffect = do + setupSrcTransitiveTests spago { installConsoleAndEffectInTests } = do spago [ "init", "--name", "7368613235362d34312f4e59746b7869335477336d33414d72" ] >>= shouldBeSuccess spago [ "install", "maybe" ] >>= shouldBeSuccess - when installConsoleAndEffect do + when installConsoleAndEffectInTests do spago [ "install", "--test-deps", "console", "effect" ] >>= shouldBeSuccess FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) $ Array.intercalate "\n" [ "module Main where" @@ -138,11 +138,13 @@ spec = Spec.around withTempDir do spago [ "build" ] >>= shouldBeSuccess Spec.it "passed --pedantic-packages CLI flag" \{ spago, fixture } -> do - setupSrcTransitiveTests spago true + -- Install deps in test so that overriding the test's config with pedantic packages CLI flag + -- doesn't find issues in test code. This code is only testing source package. + setupSrcTransitiveTests spago { installConsoleAndEffectInTests: true } spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-direct-import-transitive-dependency.txt") Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do - setupSrcTransitiveTests spago false + setupSrcTransitiveTests spago { installConsoleAndEffectInTests: false } addPedanticPackagesToSrc spago [ "build" ] >>= shouldBeFailureErr (fixture "check-direct-import-transitive-dependency.txt") @@ -186,20 +188,22 @@ spec = Spec.around withTempDir do Spec.describe "in a source package when" do let - setupSrcUnusedDeps spago installConsoleAndEffect = do + setupSrcUnusedDeps spago { installConsoleAndEffectInTests } = do spago [ "init", "--name", "7368613235362d2f444a2b4f56375435646a59726b53586548" ] >>= shouldBeSuccess - when installConsoleAndEffect do + when installConsoleAndEffectInTests do spago [ "install", "--test-deps", "console", "effect" ] >>= shouldBeSuccess FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) "module Main where\nimport Prelude\nmain = unit" -- get rid of "Compiling ..." messages and other compiler warnings spago [ "build" ] >>= shouldBeSuccess Spec.it "passing --pedantic-packages CLI flag" \{ spago, fixture } -> do - setupSrcUnusedDeps spago true + -- Install deps in test so that overriding the test's config with pedantic packages CLI flag + -- doesn't find issues in test code. This code is only testing source package. + setupSrcUnusedDeps spago { installConsoleAndEffectInTests: true } spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-unused-dependency.txt") Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do - setupSrcUnusedDeps spago false + setupSrcUnusedDeps spago { installConsoleAndEffectInTests: false } addPedanticPackagesToSrc spago [ "build" ] >>= shouldBeFailureErr (fixture "check-unused-dependency.txt") From 4bfca519fea80f0df7bd51cce402292fe4b04179 Mon Sep 17 00:00:00 2001 From: Jordan Martinez Date: Wed, 18 Oct 2023 07:24:57 -0500 Subject: [PATCH 07/11] Add writeMain/writeTestMain --- test/Prelude.purs | 12 ++++++++++++ test/Spago/Build.purs | 42 +++++++++++++++++++----------------------- 2 files changed, 31 insertions(+), 23 deletions(-) diff --git a/test/Prelude.purs b/test/Prelude.purs index 4744523a9..9c1d90d58 100644 --- a/test/Prelude.purs +++ b/test/Prelude.purs @@ -185,6 +185,18 @@ mkPackageName = unsafeFromRight <<< PackageName.parse mkVersion :: String -> Version mkVersion = unsafeFromRight <<< Version.parse +writeMain :: Array String -> String +writeMain rest = writePursFile { moduleName: "Main", rest } + +writeTestMain :: Array String -> String +writeTestMain rest = writePursFile { moduleName: "Test.Main", rest } + +writePursFile :: { moduleName :: String, rest :: Array String } -> String +writePursFile { moduleName, rest } = + Array.intercalate "\n" $ Array.cons modNameLine $ rest + where + modNameLine = "module " <> moduleName <> " where" + mkDependencies :: Array String -> Config.Dependencies mkDependencies = Config.Dependencies <<< Map.fromFoldable <<< map (flip Tuple Nothing <<< mkPackageName) diff --git a/test/Spago/Build.purs b/test/Spago/Build.purs index ce60cd259..49e8031eb 100644 --- a/test/Spago/Build.purs +++ b/test/Spago/Build.purs @@ -2,7 +2,6 @@ module Test.Spago.Build where import Test.Prelude -import Data.Array as Array import Data.Map as Map import Node.FS.Aff as FSA import Node.Path as Path @@ -125,9 +124,8 @@ spec = Spec.around withTempDir do spago [ "install", "maybe" ] >>= shouldBeSuccess when installConsoleAndEffectInTests do spago [ "install", "--test-deps", "console", "effect" ] >>= shouldBeSuccess - FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) $ Array.intercalate "\n" - [ "module Main where" - , "import Prelude" + FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) $ writeMain + [ "import Prelude" , "import Data.Maybe as Maybe" , "import Effect as Effect" , "import Effect.Console as Console" @@ -154,17 +152,15 @@ spec = Spec.around withTempDir do setupTestTransitiveTests spago = do spago [ "init", "--name", "7368613235362d34312f4e59746b7869335477336d33414d72" ] >>= shouldBeSuccess spago [ "install", "maybe" ] >>= shouldBeSuccess - FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) $ Array.intercalate "\n" - [ "module Main where" - , "import Prelude" + FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) $ writeMain + [ "import Prelude" , "import Data.Maybe as Maybe" , "import Effect as Effect" , "import Effect.Console as Console" , "main = unit" ] - FS.writeTextFile (Path.concat [ "test", "Test", "Main.purs" ]) $ Array.intercalate "\n" - [ "module Test.Main where" - , "import Prelude" + FS.writeTextFile (Path.concat [ "test", "Test", "Main.purs" ]) $ writeTestMain + [ "import Prelude" , "import Data.Maybe as Maybe" , "import Effect as Effect" , "import Effect.Console as Console" @@ -213,9 +209,8 @@ spec = Spec.around withTempDir do setupTestUnusedDeps spago = do spago [ "init", "--name", "7368613235362d2f444a2b4f56375435646a59726b53586548" ] >>= shouldBeSuccess spago [ "install", "--test-deps", "newtype" ] >>= shouldBeSuccess - FS.writeTextFile (Path.concat [ "test", "Test", "Main.purs" ]) $ Array.intercalate "\n" - [ "module Test.Main where" - , "import Prelude" + FS.writeTextFile (Path.concat [ "test", "Test", "Main.purs" ]) $ writeTestMain + [ "import Prelude" , "import Effect as Effect" , "import Effect.Console as Console" , "main = unit" @@ -239,8 +234,13 @@ spec = Spec.around withTempDir do spago [ "init", "--name", "7368613235362d2f444a2b4f56375435646a59726b53586548" ] >>= shouldBeSuccess spago [ "install", "prelude", "effect", "console" ] >>= shouldBeSuccess spago [ "install", "--test-deps", "prelude", "effect", "console" ] >>= shouldBeSuccess - FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) "module Main where\nimport Prelude\nmain = unit" - FS.writeTextFile (Path.concat [ "test", "Test", "Main.purs" ]) "module Test.Main where\nimport Prelude\nmain = unit" + let + sameFileContent = + [ "import Prelude" + , "main = unit" + ] + FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) $ writeMain sameFileContent + FS.writeTextFile (Path.concat [ "test", "Test", "Main.purs" ]) $ writeTestMain sameFileContent -- get rid of "Compiling ..." messages and other compiler warnings spago [ "build" ] >>= shouldBeSuccess @@ -297,10 +297,8 @@ spec = Spec.around withTempDir do ] FS.mkdirp "src" - FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) $ Array.intercalate "\n" - [ "module Main where " - , "" - , "import Prelude" + FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) $ writeMain + [ "import Prelude" , "import Data.Newtype as Newtype" , "import Control.Alt as Alt" , "" @@ -309,10 +307,8 @@ spec = Spec.around withTempDir do ] FS.mkdirp $ Path.concat [ "test", "Test" ] - FS.writeTextFile (Path.concat [ "test", "Test", "Main.purs" ]) $ Array.intercalate "\n" - [ "module Test.Main where " - , "" - , "import Prelude" + FS.writeTextFile (Path.concat [ "test", "Test", "Main.purs" ]) $ writeTestMain + [ "import Prelude" , "import Data.Newtype (class Newtype)" , "import Data.Either (Either(..))" , "" From 2a89ebafc332b75a3ed65d93297fbbe6e19624af Mon Sep 17 00:00:00 2001 From: Jordan Martinez Date: Wed, 18 Oct 2023 07:27:25 -0500 Subject: [PATCH 08/11] Rename let bindings --- test/Spago/Build.purs | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/test/Spago/Build.purs b/test/Spago/Build.purs index 49e8031eb..17e2387f8 100644 --- a/test/Spago/Build.purs +++ b/test/Spago/Build.purs @@ -119,7 +119,7 @@ spec = Spec.around withTempDir do Spec.describe "appear in the source package and" do let - setupSrcTransitiveTests spago { installConsoleAndEffectInTests } = do + setupTransitiveTestsInSource spago { installConsoleAndEffectInTests } = do spago [ "init", "--name", "7368613235362d34312f4e59746b7869335477336d33414d72" ] >>= shouldBeSuccess spago [ "install", "maybe" ] >>= shouldBeSuccess when installConsoleAndEffectInTests do @@ -138,18 +138,18 @@ spec = Spec.around withTempDir do Spec.it "passed --pedantic-packages CLI flag" \{ spago, fixture } -> do -- Install deps in test so that overriding the test's config with pedantic packages CLI flag -- doesn't find issues in test code. This code is only testing source package. - setupSrcTransitiveTests spago { installConsoleAndEffectInTests: true } + setupTransitiveTestsInSource spago { installConsoleAndEffectInTests: true } spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-direct-import-transitive-dependency.txt") Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do - setupSrcTransitiveTests spago { installConsoleAndEffectInTests: false } + setupTransitiveTestsInSource spago { installConsoleAndEffectInTests: false } addPedanticPackagesToSrc spago [ "build" ] >>= shouldBeFailureErr (fixture "check-direct-import-transitive-dependency.txt") Spec.describe "appear in the test package and" do let - setupTestTransitiveTests spago = do + setupTransitiveTestsInTests spago = do spago [ "init", "--name", "7368613235362d34312f4e59746b7869335477336d33414d72" ] >>= shouldBeSuccess spago [ "install", "maybe" ] >>= shouldBeSuccess FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) $ writeMain @@ -171,11 +171,11 @@ spec = Spec.around withTempDir do spago [ "build" ] >>= shouldBeSuccess Spec.it "passed --pedantic-packages CLI flag" \{ spago, fixture } -> do - setupTestTransitiveTests spago + setupTransitiveTestsInTests spago spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-direct-import-transitive-dependency-test.txt") Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do - setupTestTransitiveTests spago + setupTransitiveTestsInTests spago addPedanticPackagesToTest spago [ "build" ] >>= shouldBeFailureErr (fixture "check-direct-import-transitive-dependency-test.txt") @@ -184,7 +184,7 @@ spec = Spec.around withTempDir do Spec.describe "in a source package when" do let - setupSrcUnusedDeps spago { installConsoleAndEffectInTests } = do + setupUnusedDepsInSource spago { installConsoleAndEffectInTests } = do spago [ "init", "--name", "7368613235362d2f444a2b4f56375435646a59726b53586548" ] >>= shouldBeSuccess when installConsoleAndEffectInTests do spago [ "install", "--test-deps", "console", "effect" ] >>= shouldBeSuccess @@ -195,18 +195,18 @@ spec = Spec.around withTempDir do Spec.it "passing --pedantic-packages CLI flag" \{ spago, fixture } -> do -- Install deps in test so that overriding the test's config with pedantic packages CLI flag -- doesn't find issues in test code. This code is only testing source package. - setupSrcUnusedDeps spago { installConsoleAndEffectInTests: true } + setupUnusedDepsInSource spago { installConsoleAndEffectInTests: true } spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-unused-dependency.txt") Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do - setupSrcUnusedDeps spago { installConsoleAndEffectInTests: false } + setupUnusedDepsInSource spago { installConsoleAndEffectInTests: false } addPedanticPackagesToSrc spago [ "build" ] >>= shouldBeFailureErr (fixture "check-unused-dependency.txt") Spec.describe "in a test package when" do let - setupTestUnusedDeps spago = do + setupUnusedDepsInTest spago = do spago [ "init", "--name", "7368613235362d2f444a2b4f56375435646a59726b53586548" ] >>= shouldBeSuccess spago [ "install", "--test-deps", "newtype" ] >>= shouldBeSuccess FS.writeTextFile (Path.concat [ "test", "Test", "Main.purs" ]) $ writeTestMain @@ -219,18 +219,18 @@ spec = Spec.around withTempDir do spago [ "build" ] >>= shouldBeSuccess Spec.it "passing --pedantic-packages CLI flag" \{ spago, fixture } -> do - setupTestUnusedDeps spago + setupUnusedDepsInTest spago spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-unused-test-dependency.txt") Spec.it "package's test config has 'pedantic_packages: true'" \{ spago, fixture } -> do - setupTestUnusedDeps spago + setupUnusedDepsInTest spago addPedanticPackagesToTest spago [ "build" ] >>= shouldBeFailureErr (fixture "check-unused-test-dependency.txt") Spec.describe "in both the source and test packages when" do let - setupUnusedDeps spago = do + setupUnusedDepsInBoth spago = do spago [ "init", "--name", "7368613235362d2f444a2b4f56375435646a59726b53586548" ] >>= shouldBeSuccess spago [ "install", "prelude", "effect", "console" ] >>= shouldBeSuccess spago [ "install", "--test-deps", "prelude", "effect", "console" ] >>= shouldBeSuccess @@ -245,11 +245,11 @@ spec = Spec.around withTempDir do spago [ "build" ] >>= shouldBeSuccess Spec.it "passing --pedantic-packages CLI flag" \{ spago, fixture } -> do - setupUnusedDeps spago + setupUnusedDepsInBoth spago spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-unused-source-and-test-dependency.txt") Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do - setupUnusedDeps spago + setupUnusedDepsInBoth spago addPedanticPackagesToSrc addPedanticPackagesToTest spago [ "build" ] >>= shouldBeFailureErr (fixture "check-unused-source-and-test-dependency.txt") @@ -285,7 +285,7 @@ spec = Spec.around withTempDir do } } - setupUnusedAndTransitiveDeps spago = do + setupUnusedAndTransitiveDepsInBoth spago = do -- Since we need to edit `spago.yaml` and other files generated by `spago init`, -- may as well just create those files by hand. FS.writeYamlFile configCodec "spago.yaml" $ mkPackageAndWorkspaceConfig @@ -322,11 +322,11 @@ spec = Spec.around withTempDir do spago [ "build" ] >>= shouldBeSuccess Spec.it "passing --pedantic-packages" \{ spago, fixture } -> do - setupUnusedAndTransitiveDeps spago + setupUnusedAndTransitiveDepsInBoth spago spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-pedantic-packages.txt") Spec.it "package config has `pedantic_packages: true` in both `build` and `test`" \{ spago, fixture } -> do - setupUnusedAndTransitiveDeps spago + setupUnusedAndTransitiveDepsInBoth spago addPedanticPackages spago [ "build" ] >>= shouldBeFailureErr (fixture "check-pedantic-packages.txt") From 6733b50a0efe00eef3766ed0c92ebd7b4b046edd Mon Sep 17 00:00:00 2001 From: Fabrizio Ferrai Date: Tue, 31 Oct 2023 13:33:39 +0100 Subject: [PATCH 09/11] Refactor the test setup for the pedantic flag --- test-fixtures/check-pedantic-packages.txt | 41 --- ...heck-unused-source-and-test-dependency.txt | 23 -- ...rect-import-transitive-dependency-both.txt | 33 +++ ...rect-import-transitive-dependency-test.txt | 6 +- ...ck-direct-import-transitive-dependency.txt | 6 +- .../pedantic/check-pedantic-packages.txt | 43 +++ .../check-unused-dependency-in-source.txt | 30 ++ .../check-unused-dependency.txt | 4 +- ...heck-unused-source-and-test-dependency.txt | 23 ++ .../check-unused-test-dependency.txt | 4 +- test/Prelude.purs | 4 +- test/Spago/Build.purs | 256 +---------------- test/Spago/Build/Pedantic.purs | 259 ++++++++++++++++++ 13 files changed, 404 insertions(+), 328 deletions(-) delete mode 100644 test-fixtures/check-pedantic-packages.txt delete mode 100644 test-fixtures/check-unused-source-and-test-dependency.txt create mode 100644 test-fixtures/pedantic/check-direct-import-transitive-dependency-both.txt rename test-fixtures/{ => pedantic}/check-direct-import-transitive-dependency-test.txt (52%) rename test-fixtures/{ => pedantic}/check-direct-import-transitive-dependency.txt (52%) create mode 100644 test-fixtures/pedantic/check-pedantic-packages.txt create mode 100644 test-fixtures/pedantic/check-unused-dependency-in-source.txt rename test-fixtures/{ => pedantic}/check-unused-dependency.txt (56%) create mode 100644 test-fixtures/pedantic/check-unused-source-and-test-dependency.txt rename test-fixtures/{ => pedantic}/check-unused-test-dependency.txt (56%) create mode 100644 test/Spago/Build/Pedantic.purs diff --git a/test-fixtures/check-pedantic-packages.txt b/test-fixtures/check-pedantic-packages.txt deleted file mode 100644 index 6b7619495..000000000 --- a/test-fixtures/check-pedantic-packages.txt +++ /dev/null @@ -1,41 +0,0 @@ -Reading Spago workspace configuration... -Read the package set from the registry - -✅ Selecting package to build: foo - -Downloading dependencies... -Building... - Src Lib All -Warnings 0 0 0 -Errors 0 0 0 - -✅ Build succeeded. - -Looking for unused and undeclared transitive dependencies... - -❌ Sources for package 'foo' declares unused dependencies - please remove them from the project config: - - either - - -❌ Sources for package 'foo' import the following transitive dependencies - please add them to the project dependencies, or remove the imports: - newtype - from `Main`, which imports: - Data.Newtype - - -Run the following command to install them all: - spago install -p foo newtype - - -❌ Tests for package 'foo' declares unused dependencies - please remove them from the project config: - - tuples - - -❌ Tests for package 'foo' import the following transitive dependencies - please add them to the project dependencies, or remove the imports: - either - from `Test.Main`, which imports: - Data.Either - - -Run the following command to install them all: - spago install --test-deps -p foo either diff --git a/test-fixtures/check-unused-source-and-test-dependency.txt b/test-fixtures/check-unused-source-and-test-dependency.txt deleted file mode 100644 index 0de0f2d63..000000000 --- a/test-fixtures/check-unused-source-and-test-dependency.txt +++ /dev/null @@ -1,23 +0,0 @@ -Reading Spago workspace configuration... -Read the package set from the registry - -✅ Selecting package to build: 7368613235362d2f444a2b4f56375435646a59726b53586548 - -Downloading dependencies... -Building... - Src Lib All -Warnings 0 0 0 -Errors 0 0 0 - -✅ Build succeeded. - -Looking for unused and undeclared transitive dependencies... - -❌ Sources for package '7368613235362d2f444a2b4f56375435646a59726b53586548' declares unused dependencies - please remove them from the project config: - - console - - effect - - -❌ Tests for package '7368613235362d2f444a2b4f56375435646a59726b53586548' declares unused dependencies - please remove them from the project config: - - console - - effect diff --git a/test-fixtures/pedantic/check-direct-import-transitive-dependency-both.txt b/test-fixtures/pedantic/check-direct-import-transitive-dependency-both.txt new file mode 100644 index 000000000..23001f4f7 --- /dev/null +++ b/test-fixtures/pedantic/check-direct-import-transitive-dependency-both.txt @@ -0,0 +1,33 @@ +Reading Spago workspace configuration... +Read the package set from the registry + +✅ Selecting package to build: pedantic + +Downloading dependencies... +Building... + Src Lib All +Warnings 0 0 0 +Errors 0 0 0 + +✅ Build succeeded. + +Looking for unused and undeclared transitive dependencies... + +❌ Sources for package 'pedantic' import the following transitive dependencies - please add them to the project dependencies, or remove the imports: + maybe + from `Main`, which imports: + Data.Maybe + + +Run the following command to install them all: + spago install -p pedantic maybe + + +❌ Tests for package 'pedantic' import the following transitive dependencies - please add them to the project dependencies, or remove the imports: + control + from `Test.Main`, which imports: + Control.Alt + + +Run the following command to install them all: + spago install --test-deps -p pedantic control diff --git a/test-fixtures/check-direct-import-transitive-dependency-test.txt b/test-fixtures/pedantic/check-direct-import-transitive-dependency-test.txt similarity index 52% rename from test-fixtures/check-direct-import-transitive-dependency-test.txt rename to test-fixtures/pedantic/check-direct-import-transitive-dependency-test.txt index 97c52ddab..1c3c96cfd 100644 --- a/test-fixtures/check-direct-import-transitive-dependency-test.txt +++ b/test-fixtures/pedantic/check-direct-import-transitive-dependency-test.txt @@ -1,7 +1,7 @@ Reading Spago workspace configuration... Read the package set from the registry -✅ Selecting package to build: 7368613235362d34312f4e59746b7869335477336d33414d72 +✅ Selecting package to build: pedantic Downloading dependencies... Building... @@ -13,11 +13,11 @@ Errors 0 0 0 Looking for unused and undeclared transitive dependencies... -❌ Tests for package '7368613235362d34312f4e59746b7869335477336d33414d72' import the following transitive dependencies - please add them to the project dependencies, or remove the imports: +❌ Tests for package 'pedantic' import the following transitive dependencies - please add them to the project dependencies, or remove the imports: control from `Test.Main`, which imports: Control.Alt Run the following command to install them all: - spago install --test-deps -p 7368613235362d34312f4e59746b7869335477336d33414d72 control + spago install --test-deps -p pedantic control diff --git a/test-fixtures/check-direct-import-transitive-dependency.txt b/test-fixtures/pedantic/check-direct-import-transitive-dependency.txt similarity index 52% rename from test-fixtures/check-direct-import-transitive-dependency.txt rename to test-fixtures/pedantic/check-direct-import-transitive-dependency.txt index 638330861..8131ae6c4 100644 --- a/test-fixtures/check-direct-import-transitive-dependency.txt +++ b/test-fixtures/pedantic/check-direct-import-transitive-dependency.txt @@ -1,7 +1,7 @@ Reading Spago workspace configuration... Read the package set from the registry -✅ Selecting package to build: 7368613235362d34312f4e59746b7869335477336d33414d72 +✅ Selecting package to build: pedantic Downloading dependencies... Building... @@ -13,11 +13,11 @@ Errors 0 0 0 Looking for unused and undeclared transitive dependencies... -❌ Sources for package '7368613235362d34312f4e59746b7869335477336d33414d72' import the following transitive dependencies - please add them to the project dependencies, or remove the imports: +❌ Sources for package 'pedantic' import the following transitive dependencies - please add them to the project dependencies, or remove the imports: control from `Main`, which imports: Control.Alt Run the following command to install them all: - spago install -p 7368613235362d34312f4e59746b7869335477336d33414d72 control + spago install -p pedantic control diff --git a/test-fixtures/pedantic/check-pedantic-packages.txt b/test-fixtures/pedantic/check-pedantic-packages.txt new file mode 100644 index 000000000..7af052601 --- /dev/null +++ b/test-fixtures/pedantic/check-pedantic-packages.txt @@ -0,0 +1,43 @@ +Reading Spago workspace configuration... +Read the package set from the registry + +✅ Selecting package to build: pedantic + +Downloading dependencies... +Building... + Src Lib All +Warnings 0 0 0 +Errors 0 0 0 + +✅ Build succeeded. + +Looking for unused and undeclared transitive dependencies... + +❌ Sources for package 'pedantic' declares unused dependencies - please remove them from the project config: + - console + - effect + - either + + +❌ Sources for package 'pedantic' import the following transitive dependencies - please add them to the project dependencies, or remove the imports: + newtype + from `Main`, which imports: + Data.Newtype + + +Run the following command to install them all: + spago install -p pedantic newtype + + +❌ Tests for package 'pedantic' declares unused dependencies - please remove them from the project config: + - tuples + + +❌ Tests for package 'pedantic' import the following transitive dependencies - please add them to the project dependencies, or remove the imports: + either + from `Test.Main`, which imports: + Data.Either + + +Run the following command to install them all: + spago install --test-deps -p pedantic either diff --git a/test-fixtures/pedantic/check-unused-dependency-in-source.txt b/test-fixtures/pedantic/check-unused-dependency-in-source.txt new file mode 100644 index 000000000..44935f0bc --- /dev/null +++ b/test-fixtures/pedantic/check-unused-dependency-in-source.txt @@ -0,0 +1,30 @@ +Reading Spago workspace configuration... +Read the package set from the registry + +✅ Selecting package to build: pedantic + +Downloading dependencies... +Building... + Src Lib All +Warnings 0 0 0 +Errors 0 0 0 + +✅ Build succeeded. + +Looking for unused and undeclared transitive dependencies... + +❌ Sources for package 'pedantic' declares unused dependencies - please remove them from the project config: + - console + - effect + + +❌ Tests for package 'pedantic' import the following transitive dependencies - please add them to the project dependencies, or remove the imports: + console + from `Test.Main`, which imports: + Effect.Console + effect + from `Test.Main`, which imports: + Effect + +Run the following command to install them all: + spago install --test-deps -p pedantic console effect diff --git a/test-fixtures/check-unused-dependency.txt b/test-fixtures/pedantic/check-unused-dependency.txt similarity index 56% rename from test-fixtures/check-unused-dependency.txt rename to test-fixtures/pedantic/check-unused-dependency.txt index 042aa88ce..83f7d1b1c 100644 --- a/test-fixtures/check-unused-dependency.txt +++ b/test-fixtures/pedantic/check-unused-dependency.txt @@ -1,7 +1,7 @@ Reading Spago workspace configuration... Read the package set from the registry -✅ Selecting package to build: 7368613235362d2f444a2b4f56375435646a59726b53586548 +✅ Selecting package to build: pedantic Downloading dependencies... Building... @@ -13,6 +13,6 @@ Errors 0 0 0 Looking for unused and undeclared transitive dependencies... -❌ Sources for package '7368613235362d2f444a2b4f56375435646a59726b53586548' declares unused dependencies - please remove them from the project config: +❌ Sources for package 'pedantic' declares unused dependencies - please remove them from the project config: - console - effect diff --git a/test-fixtures/pedantic/check-unused-source-and-test-dependency.txt b/test-fixtures/pedantic/check-unused-source-and-test-dependency.txt new file mode 100644 index 000000000..c762edab8 --- /dev/null +++ b/test-fixtures/pedantic/check-unused-source-and-test-dependency.txt @@ -0,0 +1,23 @@ +Reading Spago workspace configuration... +Read the package set from the registry + +✅ Selecting package to build: pedantic + +Downloading dependencies... +Building... + Src Lib All +Warnings 0 0 0 +Errors 0 0 0 + +✅ Build succeeded. + +Looking for unused and undeclared transitive dependencies... + +❌ Sources for package 'pedantic' declares unused dependencies - please remove them from the project config: + - console + - effect + + +❌ Tests for package 'pedantic' declares unused dependencies - please remove them from the project config: + - console + - effect diff --git a/test-fixtures/check-unused-test-dependency.txt b/test-fixtures/pedantic/check-unused-test-dependency.txt similarity index 56% rename from test-fixtures/check-unused-test-dependency.txt rename to test-fixtures/pedantic/check-unused-test-dependency.txt index 706b06844..136eefc3b 100644 --- a/test-fixtures/check-unused-test-dependency.txt +++ b/test-fixtures/pedantic/check-unused-test-dependency.txt @@ -1,7 +1,7 @@ Reading Spago workspace configuration... Read the package set from the registry -✅ Selecting package to build: 7368613235362d2f444a2b4f56375435646a59726b53586548 +✅ Selecting package to build: pedantic Downloading dependencies... Building... @@ -13,5 +13,5 @@ Errors 0 0 0 Looking for unused and undeclared transitive dependencies... -❌ Tests for package '7368613235362d2f444a2b4f56375435646a59726b53586548' declares unused dependencies - please remove them from the project config: +❌ Tests for package 'pedantic' declares unused dependencies - please remove them from the project config: - newtype diff --git a/test/Prelude.purs b/test/Prelude.purs index 9c1d90d58..7fde51cc1 100644 --- a/test/Prelude.purs +++ b/test/Prelude.purs @@ -42,7 +42,9 @@ withTempDir = Aff.bracket createTempDir cleanupTempDir temp <- mkTemp' $ Just "spago-test-" FS.mkdirp temp liftEffect $ Process.chdir temp - log $ "Running test in " <> temp + isDebug <- liftEffect $ map isJust $ Process.lookupEnv "SPAGO_TEST_DEBUG" + when isDebug do + log $ "Running test in " <> temp let fixturesPath = oldCwd <> Path.sep <> "test-fixtures" diff --git a/test/Spago/Build.purs b/test/Spago/Build.purs index 17e2387f8..2bc2b105c 100644 --- a/test/Spago/Build.purs +++ b/test/Spago/Build.purs @@ -2,14 +2,13 @@ module Test.Spago.Build where import Test.Prelude -import Data.Map as Map import Node.FS.Aff as FSA import Node.Path as Path import Node.Process as Process import Spago.Command.Init as Init -import Spago.Core.Config (configCodec) import Spago.Core.Config as Config import Spago.FS as FS +import Test.Spago.Build.Pedantic as Pedantic import Test.Spago.Build.Polyrepo as BuildPolyrepo import Test.Spec (Spec) import Test.Spec as Spec @@ -79,257 +78,6 @@ spec = Spec.around withTempDir do FS.exists "output" `Assert.shouldReturn` true FS.exists (Path.concat [ "subpackage", "output" ]) `Assert.shouldReturn` false - Spec.describe "pedantic packages" do - - let - modifyPackageConfig f = do - content <- FS.readYamlDocFile configCodec "spago.yaml" - case content of - Left err -> - Assert.fail $ "Failed to decode spago.yaml file\n" <> err - Right { yaml: config } -> - FS.writeYamlFile configCodec "spago.yaml" $ f config - - addPedanticPackagesToSrc = modifyPackageConfig \config -> - config - { package = config.package <#> \r -> r - { build = Just - { pedantic_packages: Just true - , strict: Nothing - , censor_project_warnings: Nothing - } - } - } - addPedanticPackagesToTest = modifyPackageConfig \config -> - config - { package = config.package <#> \r -> r - { test = Just - { main: "Test.Main" - , pedantic_packages: Just true - , strict: Nothing - , censor_test_warnings: Nothing - , dependencies: maybe (Config.Dependencies Map.empty) _.dependencies r.test - , execArgs: r.test >>= _.execArgs - } - } - } - - Spec.describe "fails when imports from transitive dependencies" do - - Spec.describe "appear in the source package and" do - - let - setupTransitiveTestsInSource spago { installConsoleAndEffectInTests } = do - spago [ "init", "--name", "7368613235362d34312f4e59746b7869335477336d33414d72" ] >>= shouldBeSuccess - spago [ "install", "maybe" ] >>= shouldBeSuccess - when installConsoleAndEffectInTests do - spago [ "install", "--test-deps", "console", "effect" ] >>= shouldBeSuccess - FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) $ writeMain - [ "import Prelude" - , "import Data.Maybe as Maybe" - , "import Effect as Effect" - , "import Effect.Console as Console" - , "import Control.Alt as Alt" - , "main = unit" - ] - -- get rid of "Compiling ..." messages and other compiler warnings - spago [ "build" ] >>= shouldBeSuccess - - Spec.it "passed --pedantic-packages CLI flag" \{ spago, fixture } -> do - -- Install deps in test so that overriding the test's config with pedantic packages CLI flag - -- doesn't find issues in test code. This code is only testing source package. - setupTransitiveTestsInSource spago { installConsoleAndEffectInTests: true } - spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-direct-import-transitive-dependency.txt") - - Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do - setupTransitiveTestsInSource spago { installConsoleAndEffectInTests: false } - addPedanticPackagesToSrc - spago [ "build" ] >>= shouldBeFailureErr (fixture "check-direct-import-transitive-dependency.txt") - - Spec.describe "appear in the test package and" do - - let - setupTransitiveTestsInTests spago = do - spago [ "init", "--name", "7368613235362d34312f4e59746b7869335477336d33414d72" ] >>= shouldBeSuccess - spago [ "install", "maybe" ] >>= shouldBeSuccess - FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) $ writeMain - [ "import Prelude" - , "import Data.Maybe as Maybe" - , "import Effect as Effect" - , "import Effect.Console as Console" - , "main = unit" - ] - FS.writeTextFile (Path.concat [ "test", "Test", "Main.purs" ]) $ writeTestMain - [ "import Prelude" - , "import Data.Maybe as Maybe" - , "import Effect as Effect" - , "import Effect.Console as Console" - , "import Control.Alt as Alt" - , "main = unit" - ] - -- get rid of "Compiling ..." messages and other compiler warnings - spago [ "build" ] >>= shouldBeSuccess - - Spec.it "passed --pedantic-packages CLI flag" \{ spago, fixture } -> do - setupTransitiveTestsInTests spago - spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-direct-import-transitive-dependency-test.txt") - - Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do - setupTransitiveTestsInTests spago - addPedanticPackagesToTest - spago [ "build" ] >>= shouldBeFailureErr (fixture "check-direct-import-transitive-dependency-test.txt") - - Spec.describe "fails with warnings about unused dependencies" do - - Spec.describe "in a source package when" do - - let - setupUnusedDepsInSource spago { installConsoleAndEffectInTests } = do - spago [ "init", "--name", "7368613235362d2f444a2b4f56375435646a59726b53586548" ] >>= shouldBeSuccess - when installConsoleAndEffectInTests do - spago [ "install", "--test-deps", "console", "effect" ] >>= shouldBeSuccess - FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) "module Main where\nimport Prelude\nmain = unit" - -- get rid of "Compiling ..." messages and other compiler warnings - spago [ "build" ] >>= shouldBeSuccess - - Spec.it "passing --pedantic-packages CLI flag" \{ spago, fixture } -> do - -- Install deps in test so that overriding the test's config with pedantic packages CLI flag - -- doesn't find issues in test code. This code is only testing source package. - setupUnusedDepsInSource spago { installConsoleAndEffectInTests: true } - spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-unused-dependency.txt") - - Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do - setupUnusedDepsInSource spago { installConsoleAndEffectInTests: false } - addPedanticPackagesToSrc - spago [ "build" ] >>= shouldBeFailureErr (fixture "check-unused-dependency.txt") - - Spec.describe "in a test package when" do - - let - setupUnusedDepsInTest spago = do - spago [ "init", "--name", "7368613235362d2f444a2b4f56375435646a59726b53586548" ] >>= shouldBeSuccess - spago [ "install", "--test-deps", "newtype" ] >>= shouldBeSuccess - FS.writeTextFile (Path.concat [ "test", "Test", "Main.purs" ]) $ writeTestMain - [ "import Prelude" - , "import Effect as Effect" - , "import Effect.Console as Console" - , "main = unit" - ] - -- get rid of "Compiling ..." messages and other compiler warnings - spago [ "build" ] >>= shouldBeSuccess - - Spec.it "passing --pedantic-packages CLI flag" \{ spago, fixture } -> do - setupUnusedDepsInTest spago - spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-unused-test-dependency.txt") - - Spec.it "package's test config has 'pedantic_packages: true'" \{ spago, fixture } -> do - setupUnusedDepsInTest spago - addPedanticPackagesToTest - spago [ "build" ] >>= shouldBeFailureErr (fixture "check-unused-test-dependency.txt") - - Spec.describe "in both the source and test packages when" do - - let - setupUnusedDepsInBoth spago = do - spago [ "init", "--name", "7368613235362d2f444a2b4f56375435646a59726b53586548" ] >>= shouldBeSuccess - spago [ "install", "prelude", "effect", "console" ] >>= shouldBeSuccess - spago [ "install", "--test-deps", "prelude", "effect", "console" ] >>= shouldBeSuccess - let - sameFileContent = - [ "import Prelude" - , "main = unit" - ] - FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) $ writeMain sameFileContent - FS.writeTextFile (Path.concat [ "test", "Test", "Main.purs" ]) $ writeTestMain sameFileContent - -- get rid of "Compiling ..." messages and other compiler warnings - spago [ "build" ] >>= shouldBeSuccess - - Spec.it "passing --pedantic-packages CLI flag" \{ spago, fixture } -> do - setupUnusedDepsInBoth spago - spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-unused-source-and-test-dependency.txt") - - Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do - setupUnusedDepsInBoth spago - addPedanticPackagesToSrc - addPedanticPackagesToTest - spago [ "build" ] >>= shouldBeFailureErr (fixture "check-unused-source-and-test-dependency.txt") - - {- - Goal: - Src - - remove either - unused - - install newtype - (transitive dep of either) as it will be needed once either is removed - Test - - remove tuples - unused - - install either - (implicit dep of source package, but need to add it here once removed from source) - -} - Spec.describe "fails to build and reports deduplicated src and test unused/transitive dependenciess when" do - let - addPedanticPackages = do - modifyPackageConfig \config -> - config - { package = config.package <#> \r -> r - { build = Just - { pedantic_packages: Just true - , strict: r.build >>= _.strict - , censor_project_warnings: r.build >>= _.censor_project_warnings - } - , test = Just - { main: "Test.Main" - , pedantic_packages: Just true - , strict: r.test >>= _.strict - , censor_test_warnings: r.test >>= _.censor_test_warnings - , dependencies: maybe (Config.Dependencies Map.empty) _.dependencies r.test - , execArgs: r.test >>= _.execArgs - } - } - } - - setupUnusedAndTransitiveDepsInBoth spago = do - -- Since we need to edit `spago.yaml` and other files generated by `spago init`, - -- may as well just create those files by hand. - FS.writeYamlFile configCodec "spago.yaml" $ mkPackageAndWorkspaceConfig - { package: { packageName: "foo", srcDependencies: [ "prelude", "control", "either" ] } - , workspace: { setVersion: Just $ mkVersion "0.0.1" } - } - [ configAddTestMain - , configAddTestDependencies [ "tuples" ] - ] - - FS.mkdirp "src" - FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) $ writeMain - [ "import Prelude" - , "import Data.Newtype as Newtype" - , "import Control.Alt as Alt" - , "" - , "foo :: Int" - , "foo = 1" - ] - - FS.mkdirp $ Path.concat [ "test", "Test" ] - FS.writeTextFile (Path.concat [ "test", "Test", "Main.purs" ]) $ writeTestMain - [ "import Prelude" - , "import Data.Newtype (class Newtype)" - , "import Data.Either (Either(..))" - , "" - , "newtype Bar = Bar Int" - , "derive instance Newtype Bar _" - , "" - , "foo :: Either Bar Int" - , "foo = Right 1" - ] - -- get rid of "Compiling ..." messages and other compiler warnings - spago [ "build" ] >>= shouldBeSuccess - - Spec.it "passing --pedantic-packages" \{ spago, fixture } -> do - setupUnusedAndTransitiveDepsInBoth spago - spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-pedantic-packages.txt") - - Spec.it "package config has `pedantic_packages: true` in both `build` and `test`" \{ spago, fixture } -> do - setupUnusedAndTransitiveDepsInBoth spago - addPedanticPackages - spago [ "build" ] >>= shouldBeFailureErr (fixture "check-pedantic-packages.txt") - Spec.it "--strict causes build to fail if there are warnings" \{ spago, fixture } -> do spago [ "init" ] >>= shouldBeSuccess let srcMain = Path.concat [ "src", "Main.purs" ] @@ -388,6 +136,8 @@ spec = Spec.around withTempDir do spagoYaml <- FS.readTextFile "spago.yaml" spagoYaml `shouldContain` "- prelude: \">=6.0.1 <7.0.0\"" + Pedantic.spec + BuildPolyrepo.spec -- Spec.it "runs a --before command" \{ spago } -> do diff --git a/test/Spago/Build/Pedantic.purs b/test/Spago/Build/Pedantic.purs new file mode 100644 index 000000000..b843770c5 --- /dev/null +++ b/test/Spago/Build/Pedantic.purs @@ -0,0 +1,259 @@ +module Test.Spago.Build.Pedantic (spec) where + +import Test.Prelude + +import Data.Array as Array +import Data.Map as Map +import Node.Path as Path +import Spago.Core.Config (Dependencies(..), Config) +import Spago.Core.Config as Core +import Spago.FS as FS +import Test.Spec (SpecT) +import Test.Spec as Spec +import Test.Spec.Assertions as Assert + +spec :: SpecT Aff TestDirs Identity Unit +spec = + Spec.describe "pedantic packages" do + + Spec.describe "fails when imports from transitive dependencies" do + + -- Here we are importing the `Control.Alt` module, which is in the `control` + -- package, which comes through `maybe` but we are not importing directly + Spec.it "appear in the source package" \{ spago, fixture } -> do + setup spago + ( defaultSetupConfig + { installSourcePackages = [ "maybe" ] + -- Install deps in test so that overriding the test's config with pedantic packages CLI flag + -- doesn't find issues in test code. This code is only testing source package. + -- TODO: this should not be necessary: we should recognise that we are + -- using these packages in the source code, so they should not come through + -- as unnecessary into the test deps + -- , installTestPackages = [ "console", "effect" ] + , main = Just + [ "import Prelude" + , "import Data.Maybe as Maybe" + , "import Effect as Effect" + , "import Effect.Console as Console" + , "import Control.Alt as Alt" + , "main = unit" + ] + } + ) + spago [ "build", "--pedantic-packages" ] + >>= shouldBeFailureErr (fixture "pedantic/check-direct-import-transitive-dependency.txt") + editSpagoYaml addPedanticFlagToSrc + spago [ "build" ] + >>= shouldBeFailureErr (fixture "pedantic/check-direct-import-transitive-dependency.txt") + + -- We are importing `Control.Alt` in the test package, which is in the `control` + -- package, which comes through `maybe` but we are not importing directly, so we + -- should get a transitivity warning about that + Spec.it "appear in the test package" \{ spago, fixture } -> do + setup spago + ( defaultSetupConfig + { installTestPackages = [ "maybe" ] + , main = Just + [ "import Prelude" + , "import Data.Maybe as Maybe" + , "import Effect as Effect" + , "import Effect.Console as Console" + , "main = unit" + ] + , testMain = Just + [ "import Prelude" + , "import Data.Maybe as Maybe" + , "import Effect as Effect" + , "import Effect.Console as Console" + , "import Control.Alt as Alt" + , "main = unit" + ] + } + ) + -- Here we install `maybe` only in test, so there should be a transitive + -- warning to install `maybe` in source, since we use it there and the flag + -- will turn it on for both source and test + spago [ "build", "--pedantic-packages" ] + >>= shouldBeFailureErr (fixture "pedantic/check-direct-import-transitive-dependency-both.txt") + editSpagoYaml addPedanticFlagToTest + -- Otherwise we can just turn it on for the test, and it will complain only about `control` + spago [ "build" ] + >>= shouldBeFailureErr (fixture "pedantic/check-direct-import-transitive-dependency-test.txt") + + Spec.describe "fails with warnings about unused dependencies" do + + -- Here we install `effect` and `console` in the test package, and we don't use them + -- in the source, so we should get an "unused" warning about them + Spec.it "in a source package" \{ spago, fixture } -> do + setup spago + ( defaultSetupConfig + { installTestPackages = [ "effect", "console" ] + , main = Just + [ "import Prelude" + , "main = unit" + ] + } + ) + spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "pedantic/check-unused-dependency.txt") + editSpagoYaml addPedanticFlagToSrc + spago [ "build" ] >>= shouldBeFailureErr (fixture "pedantic/check-unused-dependency.txt") + + -- Here we do not install `effect` and `console` in the test package, and we don't use them + -- in the source, so we should get an "unused" warning about them for the source, and a prompt + -- to install them in test + Spec.it "in a source package, but they are used in test" \{ spago, fixture } -> do + setup spago + ( defaultSetupConfig + { main = Just + [ "import Prelude" + , "main = unit" + ] + } + ) + spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "pedantic/check-unused-dependency-in-source.txt") + editSpagoYaml addPedanticFlagToSrc + spago [ "build" ] >>= shouldBeFailureErr (fixture "pedantic/check-unused-dependency.txt") + + -- Complain about the unused `newtype` dependency in the test package + Spec.it "in a test package" \{ spago, fixture } -> do + setup spago + ( defaultSetupConfig + { installTestPackages = [ "newtype" ] + , testMain = Just + [ "import Prelude" + , "main = unit" + ] + } + ) + -- first just add the flag + spago [ "build", "--pedantic-packages" ] + >>= shouldBeFailureErr (fixture "pedantic/check-unused-test-dependency.txt") + -- then prove that it also works when using it from the config + editSpagoYaml addPedanticFlagToTest + spago [ "build" ] + >>= shouldBeFailureErr (fixture "pedantic/check-unused-test-dependency.txt") + + -- `console` and `effect` are going to be unused for both source and test packages + Spec.it "in both the source and test packages" \{ spago, fixture } -> do + setup spago + ( defaultSetupConfig + { installSourcePackages = [ "prelude", "effect", "console" ] + , installTestPackages = [ "prelude", "effect", "console" ] + , main = Just + [ "import Prelude" + , "main = unit" + ] + , testMain = Just + [ "import Prelude" + , "main = unit" + ] + } + ) + spago [ "build", "--pedantic-packages" ] + >>= shouldBeFailureErr (fixture "pedantic/check-unused-source-and-test-dependency.txt") + editSpagoYaml (addPedanticFlagToSrc >>> addPedanticFlagToTest) + spago [ "build" ] + >>= shouldBeFailureErr (fixture "pedantic/check-unused-source-and-test-dependency.txt") + + -- The source package adds `control` and `either`, but: + -- * `either` is unused + -- * `control` is used, and that's fine + -- * `newtype` is used but not declare, so we get a "transitive" warning + -- The test package adds `tuples`, but: + -- * `tuples` is unused + -- * `newtype` is transitively imported, but we already warned about that in the source + -- so consider that fixed + -- * `either` is transitively imported, and it's going to be removed from the source + -- dependencies, so we get a "transitive" warning to install it in test + Spec.it "fails to build and reports deduplicated src and test unused/transitive dependencies" \{ spago, fixture } -> do + setup spago + ( defaultSetupConfig + { installSourcePackages = [ "prelude", "control", "either" ] + , installTestPackages = [ "tuples" ] + , main = Just + [ "import Prelude" + , "import Data.Newtype as Newtype" + , "import Control.Alt as Alt" + , "" + , "foo :: Int" + , "foo = 1" + ] + , testMain = Just + [ "import Prelude" + , "import Data.Newtype (class Newtype)" + , "import Data.Either (Either(..))" + , "" + , "newtype Bar = Bar Int" + , "derive instance Newtype Bar _" + , "" + , "foo :: Either Bar Int" + , "foo = Right 1" + ] + } + ) + spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "pedantic/check-pedantic-packages.txt") + editSpagoYaml (addPedanticFlagToSrc >>> addPedanticFlagToTest) + spago [ "build" ] >>= shouldBeFailureErr (fixture "pedantic/check-pedantic-packages.txt") + +editSpagoYaml :: (Config -> Config) -> Aff Unit +editSpagoYaml f = do + content <- FS.readYamlDocFile Core.configCodec "spago.yaml" + case content of + Left err -> + Assert.fail $ "Failed to decode spago.yaml file\n" <> err + Right { yaml: config } -> + FS.writeYamlFile Core.configCodec "spago.yaml" $ f config + +addPedanticFlagToSrc :: Config -> Config +addPedanticFlagToSrc config = config + { package = config.package <#> \r -> r + { build = Just + { pedantic_packages: Just true + , strict: Nothing + , censor_project_warnings: Nothing + } + } + } + +addPedanticFlagToTest :: Config -> Config +addPedanticFlagToTest config = config + { package = config.package <#> \r -> r + { test = Just + { main: "Test.Main" + , pedantic_packages: Just true + , strict: Nothing + , censor_test_warnings: Nothing + , dependencies: maybe (Dependencies Map.empty) _.dependencies r.test + , execArgs: r.test >>= _.execArgs + } + } + } + +type SetupConfig = + { installSourcePackages :: Array String + , installTestPackages :: Array String + , main :: Maybe (Array String) + , testMain :: Maybe (Array String) + } + +defaultSetupConfig :: SetupConfig +defaultSetupConfig = + { installSourcePackages: [] + , installTestPackages: [] + , main: Nothing + , testMain: Nothing + } + +setup :: (Array String -> Aff (Either ExecError ExecResult)) -> SetupConfig -> Aff Unit +setup spago config = do + spago [ "init", "--name", "pedantic" ] >>= shouldBeSuccess + unless (Array.null config.installSourcePackages) do + spago ([ "install" ] <> config.installSourcePackages) >>= shouldBeSuccess + unless (Array.null config.installTestPackages) do + spago ([ "install", "--test-deps" ] <> config.installTestPackages) >>= shouldBeSuccess + for_ config.main \main -> + FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) $ writeMain main + for_ config.testMain \testMain -> + FS.writeTextFile (Path.concat [ "test", "Test", "Main.purs" ]) $ writeTestMain testMain + -- get rid of "Compiling ..." messages and other compiler warnings + spago [ "build" ] >>= shouldBeSuccess From 62cc73d089a57705238aee8e2008ab5f557e16ea Mon Sep 17 00:00:00 2001 From: Fabrizio Ferrai Date: Mon, 6 Nov 2023 16:02:33 +0100 Subject: [PATCH 10/11] Fix fixture --- .../pedantic/check-unused-dependency-in-source.txt | 3 ++- test/Spago/Build/Pedantic.purs | 6 ------ 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/test-fixtures/pedantic/check-unused-dependency-in-source.txt b/test-fixtures/pedantic/check-unused-dependency-in-source.txt index 44935f0bc..99365f26d 100644 --- a/test-fixtures/pedantic/check-unused-dependency-in-source.txt +++ b/test-fixtures/pedantic/check-unused-dependency-in-source.txt @@ -21,10 +21,11 @@ Looking for unused and undeclared transitive dependencies... ❌ Tests for package 'pedantic' import the following transitive dependencies - please add them to the project dependencies, or remove the imports: console from `Test.Main`, which imports: - Effect.Console + Effect.Class.Console effect from `Test.Main`, which imports: Effect + Run the following command to install them all: spago install --test-deps -p pedantic console effect diff --git a/test/Spago/Build/Pedantic.purs b/test/Spago/Build/Pedantic.purs index b843770c5..7c7458b89 100644 --- a/test/Spago/Build/Pedantic.purs +++ b/test/Spago/Build/Pedantic.purs @@ -24,12 +24,6 @@ spec = setup spago ( defaultSetupConfig { installSourcePackages = [ "maybe" ] - -- Install deps in test so that overriding the test's config with pedantic packages CLI flag - -- doesn't find issues in test code. This code is only testing source package. - -- TODO: this should not be necessary: we should recognise that we are - -- using these packages in the source code, so they should not come through - -- as unnecessary into the test deps - -- , installTestPackages = [ "console", "effect" ] , main = Just [ "import Prelude" , "import Data.Maybe as Maybe" From 2032c40241c9390af3e110ea433a0676a581becd Mon Sep 17 00:00:00 2001 From: Fabrizio Ferrai Date: Mon, 6 Nov 2023 16:07:29 +0100 Subject: [PATCH 11/11] Fix merge --- test/Spago/Build/Pedantic.purs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Spago/Build/Pedantic.purs b/test/Spago/Build/Pedantic.purs index 7c7458b89..a832f84a1 100644 --- a/test/Spago/Build/Pedantic.purs +++ b/test/Spago/Build/Pedantic.purs @@ -218,7 +218,7 @@ addPedanticFlagToTest config = config , strict: Nothing , censor_test_warnings: Nothing , dependencies: maybe (Dependencies Map.empty) _.dependencies r.test - , execArgs: r.test >>= _.execArgs + , exec_args: r.test >>= _.exec_args } } }