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

ghcide test IfaceTests, we should remove interface cache dir before we run the test, or the diganostic simply gone #4200

Open
Tracked by #4193
soulomoon opened this issue May 1, 2024 · 11 comments

Comments

@soulomoon
Copy link
Collaborator

soulomoon commented May 1, 2024

This can happened in the second round doing the test.

Reproduce

Run this consectively

LSP_TEST_LOG_MESSAGES=1 LSP_TEST_LOG_STDERR=1  TASTY_PATTERN="iface-error-test-1" cabal test ghcide-tests

we are seeing

Debug | SUCCEEDED LOADING HIE FILE FOR /Users/ares/.cache/ghcide/main-67f5d3e80e9a5dab967334c225648bdcd080465f-67f5d3e80e9a5dab967334c225648bdcd080465f/P.hie

The test past again if we remove the cache dir entirely

@soulomoon
Copy link
Collaborator Author

soulomoon commented May 1, 2024

A possible solution to this would be to remove the .cache dir in setupTestEnvironment, after we migrate this test to hls-test-utils.
But I need to see how much it is slowing down the test.

@komikat
Copy link
Contributor

komikat commented May 31, 2024

The test seems to time out on re-runs.

Exception: Timed out waiting to receive a message from the server.
Last message received:
{
    "jsonrpc": "2.0",
    "method": "$/progress",
    "params": {
        "token": "11",
        "value": {
            "kind": "end",
            "message": "Finished indexing 3 files"
        }
    }
}

@soulomoon
Copy link
Collaborator Author

soulomoon commented May 31, 2024

@komikat Yes, since if we have the cache, we won't compile it again and hence the diganostic is not genereated

@komikat
Copy link
Contributor

komikat commented May 31, 2024

@soulomoon so expected behaviour should be to not wait for a diagnosis? or delete the specific component being cached before the test is run?

@soulomoon
Copy link
Collaborator Author

soulomoon commented May 31, 2024

@komikat The point of the test is to assert wether the diganostic would arrive. So may be the latter.

@komikat
Copy link
Contributor

komikat commented Jun 1, 2024

A possible solution to this would be to remove the .cache dir in setupTestEnvironment, after we migrate this test to hls-test-utils.

I tried removing the cache dir in setupTestEnvironment like this:

setupTestEnvironment :: IO FilePath
setupTestEnvironment = do
  tmpDirRoot <- getTemporaryDirectory
  let testRoot = tmpDirRoot </> "hls-test-root"
      testCacheDir = testRoot </> ".cache"
  dirExists <- doesDirectoryExist testCacheDir
  when dirExists $ removeDirectoryRecursive testCacheDir
  createDirectory testCacheDir
  setEnv "XDG_CACHE_HOME" testCacheDir
  pure testRoot

this solves iface-error-test-1 being flaky but causes other tests to fail.
Maybe we can move to a function which removes the directory for tests like this?

@soulomoon
Copy link
Collaborator Author

soulomoon commented Jun 1, 2024

this solves iface-error-test-1 being flaky but causes other tests to fail.

Then those tests are very suspicious !(Being not worked without cache) 🤔
as

-- However, it is totally safe to delete the directory between runs.

Something must be wrong in our tests.

@komikat
Copy link
Contributor

komikat commented Jun 1, 2024

Something must be wrong in our tests.

Maybe not!
I took a look at the fails and its this specific exception everytime:

Exception: ($TMPDIR):
removeDirectoryRecursive:removeContentsRecursive:removePathRecursive:removeContentsRecursive:removePathRecursive:getSymbolicLinkStatus: 
does not exist (No such file or directory)

This suggests there being a race condition somewhere causing the directory to be deleted in between these two lines.

dirExists <- doesDirectoryExist testCacheDir
when dirExists $ removeDirectoryRecursive testCacheDir

I tried handling the exception itself instead — this way:

setupTestEnvironment :: IO FilePath
setupTestEnvironment = do
  tmpDirRoot <- getTemporaryDirectory
  let testRoot = tmpDirRoot </> "hls-test-root"
      testCacheDir = testRoot </> ".cache"
  exc <- try $ removeDirectoryRecursive testCacheDir
  case exc of
    Left e ->
      if isDoesNotExistError e
         then return ()
         else ioError e
    Right _ ->
      return ()
  createDirectory testCacheDir
  setEnv "XDG_CACHE_HOME" testCacheDir
  pure testRoot

This gets rid of all previous exceptions(!) but creates a new one:

Exception: $TMPDIR: createDirectory: already exists (File exists)

This is probably due to the same race condition causing the previous issue. We can use createDirectoryIfMissing instead but
I'm not sure why these race conditions exist considering all of the tests are being run sequentially.

Tagging @fendor since I'm not sure if I'm heading in the right direction.

@fendor
Copy link
Collaborator

fendor commented Jun 2, 2024

It looks like sharing the .cache with all tests is too coarse, how about we create one .cache dir per test case? createTempDirectory might be helpful. E.g. createTempDirectory testRoot ".cache"

@komikat
Copy link
Contributor

komikat commented Jun 2, 2024

Thanks, that solves the issue. I'm only concerned about there being too many directories being created, would it be better to clean up all the temporary dirs after?

@fendor
Copy link
Collaborator

fendor commented Jun 2, 2024

All directories are created in the temporary hls root directory. So I think it is fine.

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 a pull request may close this issue.

3 participants