Skip to content
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

Introduce declarative test project definition for plugin tests #3767

Merged
merged 2 commits into from
Aug 29, 2023

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented Aug 20, 2023

Test data is currently often in some 'testdata' directory in 'test/'.
It can easily get quite messy with many files.
Especially since 'lsp-test' should load only exactly what is needed to
speed up tests and avoid test flakiness.

A subdirectory per test file is overkill and increases the
required boilerplate to write tests.

Thus, we introduce a declarative test project specification that runs
lsp-test in a temporary directory. The first advantage is that we can load
only exactly what we need, and create more accurate projects.

Adds environment variable to help you debug failing testcases, 'HLS_TEST_HARNESS_NO_TESTDIR_CLEANUP' for
not removing the temporary directory after the test run and 'HLS_TEST_HARNESS_STDERR' to log the temporary directory location.

@fendor fendor requested review from wz1000, joyfulmantis and michaelpj and removed request for wz1000 August 20, 2023 17:40
@fendor
Copy link
Collaborator Author

fendor commented Aug 20, 2023

This is the first piece of the puzzle to make tests a bit more independent of each other.

It is extracted from another PR that leverages the HLS API directly and avoids lsp-test altogether.
While this change improves the execution time by avoiding unnecessary calls to cabal, it is still not even close to be as quick as in the other PR. Unfortunately, that PR is not ready yet.

In local testing, this change removes the flakiness from 'hls-eval-plugin', but let's see what CI says.

@fendor fendor force-pushed the enhance/temp-filesystem branch 4 times, most recently from 4bf290d to 0056a24 Compare August 21, 2023 11:41
@fendor fendor requested a review from July541 as a code owner August 21, 2023 11:41
Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Looks very nice, I had some rambling thoughts about it. I thought I'd post them and then go away and think some more.

hls-test-utils/src/Test/Hls/FileSystem.hs Outdated Show resolved Hide resolved
hls-test-utils/src/Test/Hls.hs Outdated Show resolved Hide resolved
-- To debug test cases and verify the file system is correctly set up,
-- you should set the environment variable 'HLS_TEST_HARNESS_NO_TESTDIR_CLEANUP=1'.
-- Further, we log the temporary directory location on startup. To view
-- the logs, set the environment variable 'HLS_TEST_HARNESS_STDERR=1'.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel kind of bad about us having various random environment variables to make some logs go to stderr. I don't have a proposal for what to do about it, though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The environment variables are nice because they require no recompilation to take effect. When you run cabal test --test-options, then any log level change causes recompilation. cabal run <plugin-name>:tests -- <opts> doesn't work from the HLS project root, you have to cd to the plugin directory. cabal run and cabal test set the working directory differently.

VirtualFileTree ->
Session a ->
IO a
runSessionWithServerInTmpDir' plugins conf sessConf caps tree act = withLock lockForTempDirs $ do
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought running these in parallel was a feature? Why are we preventing it?

Copy link
Collaborator Author

@fendor fendor Aug 24, 2023

Choose a reason for hiding this comment

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

At the moment because runSessionWithServer' uses withCurrentDirectory, or something like that. Which modifies a global "CWD" variable. If the integration tests run in parallel, they affect each other's current working directories, and then all fail. I haven't looked into it whether we can remove the withCurrentDirectory call...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, for some reason I thought we were actually running the HLSs in subprocesses, but we aren't.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suspect we can't get rid of the current directory changing, since I bet the bits of the GHC API that we use rely on it...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we can, in my other branch that doesn't use lsp-test, I don't need the lock or withCurrentDirectory.

@@ -390,7 +622,7 @@ runSessionWithServer' ::
FilePath ->
Session a ->
IO a
runSessionWithServer' plugins conf sconf caps root s = withLock lock $ keepCurrentDirectory $ do
runSessionWithServer' plugins conf sconf caps root s = withLock lock $ keepCurrentDirectory $ do
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, this has a lock too. Why is there a different lock for tempdir one and the non-tempdir one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a UX thing, our tests seem to always run in parallel, but we use the lock to avoid spawning too many HLS sessions. When your first tasty test uses runSessionWithServerInTmpDir' and another uses runSessionWithServer', then runSessionWithServerInTmpDir' needs to perform slightly more work than the other test case. Since all tests run in parallel, the test runSessionWithServer' reaches the lock before the first test and, thus, acquires the lock first. The first test will not run, until the other finished, and tasty will only print results until the first test has finished execution.

So, if you are really unlucky, you don't see test results until the whole test suite finished, and the execution times are also completely skewed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TBH, all these locks are annoying, I'd rather not have them at all. But refactoring that was outside my time budget for now. I think it is still an improvement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, fair enough. I wonder if we can't just use tasty's NumThreads option and set it to 1 on test trees that have runSession in them 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I actually thought that was already the case.

hls-test-utils/src/Test/Hls/FileSystem.hs Outdated Show resolved Hide resolved
hls-test-utils/src/Test/Hls/FileSystem.hs Outdated Show resolved Hide resolved
-- | Virtual representation of a filesystem tree.
--
-- Operations of 'vftTree' are relative to 'vftTestDataRoot'.
-- In particular, any 'copy' etc. operation looks for the sources in 'vftTestDataRoot'.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems a little odd to me. At the moment, a VirtualFileTree is actually defined relative to some real files on disk. It could be entirely in memory, with the contents of the files read in. It seems to me that you could have a pattern like this:

data OnDisk = OnDisk
data InMemory = InMemory T.Text

data FileTree a
  = File FilePath a
  | Directory FilePath [FileTree]
  deriving (Show, Eq, Ord)

data FileSystem = FileSystem { root :: FilePath, tree :: [FileTree OnDisk] }
data VirtualFileSystem = VirtualFileSystem { tree :: FileTree InMemory }

virtualizeFS :: FileSystem -> VirtualFileSystem
materializeFS :: FilePath -> VirtualFileSystem -> IO FileSystem

That is:

  • User specifies the "real file system" in the test directory
  • That gets "virtualized" into a VFS that has all the content in memory
  • The VFS gets "materialized" back out into a real file system in a different root.

I wrote all this and then I looked further down and I see why this would be awkward. You want to mix the specification of files from disk with programmatically specified files when you specify the input file system. I'll leave this here as an idea, anyway. I think you could still maybe make it work if you had something like:

virtualizeFS (realFS [ "foo"
   directory "bar" [
      "baz.hs"
   ]
]) <> directCradle args

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or maybe I just want a third type?

data FileSystemSpec = FileSystemSpec { root :: FilePath, tree :: [FileTree Content] }
virtualizeFS :: FileSystemSpec -> VirtualFileSystem

Copy link
Collaborator Author

@fendor fendor Aug 24, 2023

Choose a reason for hiding this comment

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

Yeah, I agree, that would be all really cool. If we manage to avoid IO, then our tests will be incredibly fast! However, one step at a time.
I hope the current design, providing an edsl, allows us to change internals without too much hassle in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I wasn't actually suggesting we run on the VFS. Just that the current types seem to be a mix of the virtual and non-virtual that I find a bit confusing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In my opinion, this mix is an essential part, and still explicit enough.

plugins/hls-eval-plugin/test/Main.hs Show resolved Hide resolved
@michaelpj
Copy link
Collaborator

I think this is definitely an improvement and I support merging it even if just so Fendor can keep experimenting :)

Test data is currently often in some 'testdata' directory in 'test/'.
It can easily get quite messy with many files.
Especially since 'lsp-test' should load only exactly what is needed to
speed up tests and avoid test flakiness.

A subdirectory per test file is quite overkill and increases the
required boilerplate to write tests.

Thus, we introduce a declarative test project specification that runs
lsp-test in a temporary directory. The first advantage is that we can load
only exactly what we need, and create more accurate projects.
Set ups a test project per file in a temporary directory.
Speeds up test cases quite a when no cabal invocation is required or
desired.

Additionally, reduces flakiness since HLS often loads only a single file
then.

Proof of concept s.t. other flaky plugin test suites can be migrated
later as well.
@fendor
Copy link
Collaborator Author

fendor commented Aug 29, 2023

Let's merge this, hls-eval-plugin test suite runs much quicker.

@michaelpj michaelpj merged commit 25e953d into haskell:master Aug 29, 2023
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants