Cleanup func-test suite (#3828)

* Cleanup func-test suite

This test-suite is vastly legacy from Haskell IDE Engine.
It was originally written before we had a plugin-based architecture
where each plugin tests its feature in isolation.
Over time, this test-suite bitrotted, where a lot of testcases were
either redundant or out-of-date and consequentially disabled.

We clean up the test-suite, delete old tests and remove unused testdata.

* Remove unused config change

* Move qualified import tests to hls-refactor-plugin

* Add README for `func-test`

Provides guidance when to add a test to `func-test` and some historical
context.

* Add fendor as CODEOWNER for `func-test` and `hls-test-utils`

---------

Co-authored-by: Michael Peyton Jones <me@michaelpj.com>

Author: fendor

CODEOWNERS

@@ -3,7 +3,8 @@
 /ghcide/session-loader @pepeiborra @fendor
 /hls-graph @pepeiborra
 /hls-plugin-api @berberman
-/hls-test-utils
+/hls-test-utils @fendor
+/test @fendor
 /hie-compat
 
 # Plugins

haskell-language-server.cabal

@@ -529,21 +529,11 @@ test-suite func-test
 
   main-is:            Main.hs
   other-modules:
-    Command
-    Completion
     Config
-    Deferred
-    Definition
-    Diagnostic
     Format
     FunctionalBadProject
-    FunctionalCodeAction
     HieBios
-    Highlight
     Progress
-    Reference
-    Symbol
-    TypeDefinition
     Test.Hls.Command
     Test.Hls.Flags
 

hls-test-utils/src/Test/Hls/Util.hs

@@ -25,6 +25,7 @@ module Test.Hls.Util
     , knownBrokenOnWindows
     , knownBrokenForGhcVersions
     , knownBrokenInEnv
+    , knownBrokenInSpecificEnv
     , onlyWorkForGhcVersions
     -- * Extract code actions
     , fromAction
@@ -123,12 +124,18 @@ hostOS
     | isMac = MacOS
     | otherwise = Linux
 
--- | Mark as broken if /any/ of environmental spec mathces the current environment.
+-- | Mark as broken if /any/ of the environmental specs matches the current environment.
 knownBrokenInEnv :: [EnvSpec] -> String -> TestTree -> TestTree
 knownBrokenInEnv envSpecs reason
     | any matchesCurrentEnv envSpecs = expectFailBecause reason
     | otherwise = id
 
+-- | Mark as broken if /all/ environmental specs match the current environment.
+knownBrokenInSpecificEnv :: [EnvSpec] -> String -> TestTree -> TestTree
+knownBrokenInSpecificEnv envSpecs reason
+    | all matchesCurrentEnv envSpecs = expectFailBecause reason
+    | otherwise = id
+
 knownBrokenOnWindows :: String -> TestTree -> TestTree
 knownBrokenOnWindows = knownBrokenInEnv [HostOS Windows]
 

plugins/hls-refactor-plugin/test/Main.hs

@@ -528,21 +528,43 @@ insertImportTests = testGroup "insert import"
       "ModuleDeclAndImports.hs"
       "ModuleDeclAndImports.expected.hs"
       "import Data.Monoid"
+  , importQualifiedTests
+  ]
+
+importQualifiedTests :: TestTree
+importQualifiedTests = testGroup "import qualified prefix suggestions"
+  [ checkImport'
+      "qualified import works with 3.8 code action kinds"
+      "ImportQualified.hs"
+      "ImportQualified.expected.hs"
+      "import qualified Control.Monad as Control"
+      ["import Control.Monad (when)"]
+  , checkImport'
+      "qualified import in postfix position works with 3.8 code action kinds"
+      "ImportPostQualified.hs"
+      "ImportPostQualified.expected.hs"
+      "import Control.Monad qualified as Control"
+      ["import qualified Control.Monad as Control", "import Control.Monad (when)"]
   ]
 
 checkImport :: String -> FilePath -> FilePath -> T.Text -> TestTree
 checkImport testComment originalPath expectedPath action =
+  checkImport' testComment originalPath expectedPath action []
+
+checkImport' :: String -> FilePath -> FilePath -> T.Text -> [T.Text] -> TestTree
+checkImport' testComment originalPath expectedPath action excludedActions =
   testSessionWithExtraFiles "import-placement" testComment $ \dir ->
     check (dir </> originalPath) (dir </> expectedPath) action
   where
     check :: FilePath -> FilePath -> T.Text -> Session ()
     check originalPath expectedPath action = do
       oSrc <- liftIO $ readFileUtf8 originalPath
-      eSrc <- liftIO $  readFileUtf8 expectedPath
+      eSrc <- liftIO $ readFileUtf8 expectedPath
       originalDoc <- createDoc originalPath "haskell" oSrc
       _ <- waitForDiagnostics
       shouldBeDoc <- createDoc expectedPath "haskell" eSrc
       actionsOrCommands <- getAllCodeActions originalDoc
+      for_ excludedActions (\a -> liftIO $ assertNoActionWithTitle a actionsOrCommands)
       chosenAction <- liftIO $ pickActionWithTitle action actionsOrCommands
       executeCodeAction chosenAction
       originalDocAfterAction <- documentContents originalDoc
@@ -3734,6 +3756,21 @@ pickActionWithTitle title actions = do
         , title == actionTitle
         ]
 
+assertNoActionWithTitle :: T.Text -> [Command |? CodeAction] -> IO ()
+assertNoActionWithTitle title actions = do
+  assertBool ("Unexpected code action " <> show title <> " in " <> show titles) (null matches)
+  pure ()
+  where
+    titles =
+        [ actionTitle
+        | InR CodeAction { _title = actionTitle } <- actions
+        ]
+    matches =
+        [ action
+        | InR action@CodeAction { _title = actionTitle } <- actions
+        , title == actionTitle
+        ]
+
 findCodeActions :: TextDocumentIdentifier -> Range -> [T.Text] -> Session [CodeAction]
 findCodeActions = findCodeActions' (==) "is not a superset of"
 

plugins/hls-refactor-plugin/test/data/import-placement/ImportPostQualified.expected.hs

@@ -0,0 +1,5 @@
+{-# LANGUAGE ImportQualifiedPost #-}
+{-# OPTIONS_GHC -Wprepositive-qualified-module #-}
+import Control.Monad qualified as Control
+main :: IO ()
+main = Control.when True $ putStrLn "hello"

test/testdata/CodeActionImportPostQualified.hs renamed to plugins/hls-refactor-plugin/test/data/import-placement/ImportPostQualified.hs

@@ -0,0 +1,3 @@
+import qualified Control.Monad as Control
+main :: IO ()
+main = Control.when True $ putStrLn "hello"

plugins/hls-refactor-plugin/test/data/import-placement/ImportQualified.expected.hs

@@ -0,0 +1,27 @@
+# The `func-test` test suite.
+
+This is the integration test suite for cross-plugin and cross-package features.
+
+Add integration tests to `func-test` only if they satisfy one or more of the following conditions:
+
+* It tests the interaction between more than one plugin.
+  * For example, plugin A provides a Diagnostic that plugin B requires to provide a CodeAction.
+  * However, it is also valid, and often preferable, to depend on the required plugin directly in plugin B's test suite.
+* It tests HLS specific LSP code.
+  * For example, we test that config changes are appropriately propagated.
+    * Note, this is slightly debatable, since the test could also be part of `ghcide`.
+  * Non HLS specific LSP code may exist in HLS temporarily, but any LSP extensions should be upstreamed to `lsp`.
+* It tests features of the `haskell-language-server-wrapper` executable.
+  * For example, argument parsing.
+* It tests features of the `haskell-language-server` executable.
+  * For example, argument parsing.
+* It tests features provided by `hls-plugin-api` that require an integration test (i.e. a unit test doesn't suffice).
+  * Example: Testing the Logger setup.
+
+If you think that a test that currently lives in `func-test` does not meet the conditions above, open a ticket for discussion or try to move the test to a better location.
+
+Note: `func-test` is a historical test suite. It was originally written for Haskell IDE Engine, which was merged with the `ghcide` project.
+The integration test-suite `func-test` (back then `unit-test` existed as well) was used to test all kinds of features provided by Haskell IDE Engine (HIE).
+When `ghcide` and HIE merged together, the integration test suite was vastly copied.
+HLS moved to a plugin-based architecture, which mainly entails that plugin tests are isolated in the respective plugin's test suite.
+Over time, `func-test` started to bit rot and wasn't maintained properly any more, since all new tests were added to the plugin or `ghcide` test suites.