Skip to content

[Migrate BootTests] part of #4173 Migrate ghcide tests to hls test utils #4227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions ghcide/test/exe/BootTests.hs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
module BootTests (tests) where

import Config (checkDefs, mkR)
import Config (checkDefs, mkR, runInDir,
runWithExtraFiles)
import Control.Applicative.Combinators
import Control.Monad
import Control.Monad.IO.Class (liftIO)
Expand All @@ -15,16 +16,15 @@ import Language.LSP.Protocol.Types hiding
SemanticTokensEdit (..),
mkRange)
import Language.LSP.Test
import System.FilePath
import Test.Hls.FileSystem (toAbsFp)
import Test.Tasty
import Test.Tasty.HUnit
import TestUtils


tests :: TestTree
tests = testGroup "boot"
[ testCase "boot-def-test" $ runWithExtraFiles "boot" $ \dir -> do
let cPath = dir </> "C.hs"
let cPath = dir `toAbsFp` "C.hs"
cSource <- liftIO $ readFileUtf8 cPath
-- Dirty the cache
liftIO $ runInDir dir $ do
Expand All @@ -51,6 +51,6 @@ tests = testGroup "boot"
let floc = mkR 9 0 9 1
checkDefs locs (pure [floc])
, testCase "graph with boot modules" $ runWithExtraFiles "boot2" $ \dir -> do
_ <- openDoc (dir </> "A.hs") "haskell"
_ <- openDoc (dir `toAbsFp` "A.hs") "haskell"
expectNoMoreDiagnostics 2
]
6 changes: 5 additions & 1 deletion ghcide/test/exe/Config.hs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ module Config(
, testWithDummyPluginEmpty'
, testWithDummyPluginAndCap'
, runWithExtraFiles
, runInDir
, testWithExtraFiles

-- * utilities for testing definition and hover
Expand All @@ -36,7 +37,7 @@ import Language.LSP.Protocol.Types (Null (..))
import System.FilePath ((</>))
import Test.Hls
import qualified Test.Hls.FileSystem as FS
import Test.Hls.FileSystem (FileSystem)
import Test.Hls.FileSystem (FileSystem, fsRoot)

testDataDir :: FilePath
testDataDir = "ghcide" </> "test" </> "data"
Expand Down Expand Up @@ -80,6 +81,9 @@ runWithExtraFiles dirName action = do
testWithExtraFiles :: String -> String -> (FileSystem -> Session ()) -> TestTree
testWithExtraFiles testName dirName action = testCase testName $ runWithExtraFiles dirName action

runInDir :: FileSystem -> Session a -> IO a
runInDir fs = runSessionWithServerNoRootLock False dummyPlugin def def def (fsRoot fs)

pattern R :: UInt -> UInt -> UInt -> UInt -> Range
pattern R x y x' y' = Range (Position x y) (Position x' y')

Expand Down
27 changes: 25 additions & 2 deletions hls-test-utils/src/Test/Hls.hs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ module Test.Hls
runSessionWithServerAndCaps,
runSessionWithServerInTmpDir,
runSessionWithServerAndCapsInTmpDir,
runSessionWithServerNoRootLock,
runSessionWithServer',
runSessionWithServerInTmpDir',
-- continuation version that take a FileSystem
Expand Down Expand Up @@ -618,7 +619,10 @@ lockForTempDirs = unsafePerformIO newLock

-- | Host a server, and run a test session on it
-- Note: cwd will be shifted into @root@ in @Session a@
runSessionWithServer' ::
-- notice this function should only be used in tests that
-- require to be nested in the same temporary directory
-- use 'runSessionWithServerInTmpDir' for other cases
runSessionWithServerNoRootLock ::
(Pretty b) =>
-- | whether we disable the kick action or not
Bool ->
Expand All @@ -632,7 +636,7 @@ runSessionWithServer' ::
FilePath ->
Session a ->
IO a
runSessionWithServer' disableKick pluginsDp conf sconf caps root s = withLock lock $ keepCurrentDirectory $ do
runSessionWithServerNoRootLock disableKick pluginsDp conf sconf caps root s = do
(inR, inW) <- createPipe
(outR, outW) <- createPipe

Expand Down Expand Up @@ -676,6 +680,25 @@ runSessionWithServer' disableKick pluginsDp conf sconf caps root s = withLock l
putStrLn $ "Finishing canceling (took " <> showDuration t <> "s)"
pure x

-- | Host a server, and run a test session on it
-- Note: cwd will be shifted into @root@ in @Session a@
Copy link
Collaborator Author

@soulomoon soulomoon May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think the original is right?

Copy link
Collaborator

@fendor fendor May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? Looks correct to me. If you don't lock, I don't think the tests will work though, as shifting the cwd will affect all tests.

Copy link
Collaborator Author

@soulomoon soulomoon May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, the shift happens in the code of lsp package, right?
We might want one with no lock too, so that it can be run inside the one with the lock. Some test in ghcide need to nest the runs.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the shift happens in the executable https://github.com/haskell/haskell-language-server/blob/master/src/Ide/Main.hs#L126

Ok, I understand that nesting might be necessary sometimes. However, the comment should make it clear that all integration tests need to be locked, since we launch a thread, not a new process, for the tests, which all share the same CWD variable.

Copy link
Collaborator Author

@soulomoon soulomoon May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the classification, it probably be a good idea to eliminate the need for getCurrentDirectory inside IDEMain.defaultMain now. I'll try to do it. Also makeAbsolute.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

@soulomoon soulomoon May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Attempt of getting rid of cwd dependent moved to #4231
Focusing on BootTests migration now.

runSessionWithServer' ::
(Pretty b) =>
-- | whether we disable the kick action or not
Bool ->
-- | Plugin to load on the server.
PluginTestDescriptor b ->
-- | lsp config for the server
Config ->
-- | config for the test session
SessionConfig ->
ClientCapabilities ->
FilePath ->
Session a ->
IO a
runSessionWithServer' disableKick pluginsDp conf sconf caps root s =
withLock lock $ keepCurrentDirectory $ runSessionWithServerNoRootLock disableKick pluginsDp conf sconf caps root s

-- | Wait for the next progress begin step
waitForProgressBegin :: Session ()
waitForProgressBegin = skipManyTill anyMessage $ satisfyMaybe $ \case
Expand Down
Loading