-
-
Notifications
You must be signed in to change notification settings - Fork 391
[Migrate OutlineTests.hs ferenceTests.hs] part of 4173 Migrate ghcide tests to hls test utils #4182
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
[Migrate OutlineTests.hs ferenceTests.hs] part of 4173 Migrate ghcide tests to hls test utils #4182
Conversation
700ebca
to
84d6383
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ghcide test changes look good, I'm a little unsure about the refactoring of the test support functions
hls-test-utils/src/Test/Hls.hs
Outdated
@@ -363,6 +365,78 @@ initialiseTestRecorder envVars = do | |||
-- ------------------------------------------------------------ | |||
-- Run an HLS server testing a specific plugin | |||
-- ------------------------------------------------------------ | |||
class TestRunner cont res where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this? I can see you're abstracting over some common patterns, but I would have thought we could do that with a wrapper function or similar rather than needing a typeclass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It provides us a way to expose the vsf during the test.
Some test in ghcide test need that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might not be a good idea to do it with wrapper
since it need to be exposed from the very inside, down to the materialization of the vfs.
It might introduce a lot of duplicated code🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just expose it to all tests and mostly ignore it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated to use wrapper version. But not sure if it is better than typeclass version🤔
several wrapper functions need to be introduced. aadf028
WDYT
@@ -66,6 +67,7 @@ data VirtualFileTree = | |||
data FileTree | |||
= File FilePath Content | |||
| Directory FilePath [FileTree] | |||
| CopiedDirectory FilePath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is this different? some haddock would be helpful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it corresponds to a directory glob.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'm confused about how it differs from the previous entry 🤔 Do we need two ways of specifying directories?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have have to manually specify the whole file tree down to the file levels in the second entry.
CopiedDirectory
allows us to simply copied an existing folder to vfs.
Matching those tests in ghcide depend on the whole folder in the test/data
.(Right, something like directory glob
)
Yep, maybe a doc comment need to be added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
…to-use-hls-test-utils
copyDir' :: FilePath -> FilePath -> IO [FileTree] | ||
copyDir' root dir = do | ||
files <- listDirectory (root </> dir) | ||
traverse (\f -> pure $ copy (dir </> f)) files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should only copy files that have been added to the git index. Cabal testsuite does something similar: https://github.com/haskell/cabal/blob/master/cabal-testsuite/src/Test/Cabal/Monad.hs#L417
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, let's copy some of the cabal code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One concern though, does it mean we have to stage the new test files to get a correct behaviour when we run the tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right the --modified
should list unstaged files
--others
should list unstaged files, and still ignore .gitignores'
…to-use-hls-test-utils
…to-use-hls-test-utils
I have done some update base on the reviews, see what is still need to be improve? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this version better, thanks :)
part of #4173
OutlineTests result is really good speed improvement: (13.51s) -> (3.62s)
from
to