Skip to content

Refactor hls-test-util and reduce getCurrentDirectory after initilization #4231

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
merged 109 commits into from
May 27, 2024

Conversation

soulomoon
Copy link
Collaborator

@soulomoon soulomoon commented May 14, 2024

What's done

  • Refactor the runSession* family function, properly add TestConfig, runSessionWithTestConfig, as the most generic runSession* function.
  • remove raraly used variants of runSession* functions and replaced by runSessionWithTestConfig.
  • migrate ExceptionTests ClientSettingsTests CodeLensTests CPPTests CradleTests to use the hls-test-utils
  • Only shift to lsp root when current root is different from the lsp root in DefaultMain of ghcide.
  • Remove most usage for getCurrentDirectory(After DefaultMain is called), Only remain those in top level of wrapper and exe, implement Overhauling the HLS testsuite #3736 (comment)

What's remains:

  • golden tests are not using TestConfig, maybe we can refactor them to use TestConfig and cut the number of variants too.
  • Hlint, Stan and some Template haskell tests still depend on current working dir. But they are out of our reach.
  • hie.yaml specify relative files
    cradle:
    direct:
      arguments: ["-unit" ,"@a-1.0.0-inplace"
                 ,"-unit" ,"@b-1.0.0-inplace" 
                 ,"-unit" ,"@c-1.0.0-inplace"
                 ]
    
    

@soulomoon

This comment was marked as outdated.

@@ -848,7 +846,7 @@ getModIfaceFromDiskAndIndexRule recorder =
hie_loc = Compat.ml_hie_file $ ms_location ms
fileHash <- liftIO $ Util.getFileHash hie_loc
mrow <- liftIO $ withHieDb (\hieDb -> HieDb.lookupHieFileFromSource hieDb (fromNormalizedFilePath f))
hie_loc' <- liftIO $ traverse (makeAbsolute . HieDb.hieModuleHieFile) mrow
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 doc said hieModuleHieFile would return a full path, I guess it is not necessary to absolute it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm unsure about this sort of thing. If we need it to be absolute then I think it can be sensible to absolutize it just to be sure... since we don't track it in the types or anything.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we should also introduce an abstraction that tracks whether we expect a path to be absolute or relative?

Copy link
Collaborator Author

@soulomoon soulomoon May 26, 2024

Choose a reason for hiding this comment

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

Seems like a good idea, some places we might need this.
path from [ghc, hiedb, lsp client, cradle].

The ones from ghc should depend on how we load the file into ghc in sessionloader?

Copy link
Collaborator Author

@soulomoon soulomoon May 26, 2024

Choose a reason for hiding this comment

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

It is quite a mess in our codebase, we might need to open an issue and try to sweep and standardize them.

@soulomoon
Copy link
Collaborator Author

soulomoon commented May 14, 2024

Stan is blocking

https://github.com/soulomoon/haskell-language-server/blob/2ebfafcea751adb8d0be9c1ad9c40a4f8771f261/plugins/hls-stan-plugin/src/Ide/Plugin/Stan.hs#L144

         -- Note that Stan works in terms of relative paths, but the HIE come in as absolute. Without
         -- making its path relative, the file name(s) won't line up with the associated Map keys.

----------------update

Same for hlint, will shift to root only for these test

@soulomoon
Copy link
Collaborator Author

soulomoon commented May 27, 2024

Again, thank your for the reviews and suggestions, expecially since this PR contains a lot of changes, you have done me a great favor of tracking everything down ! I could not do it without your help.

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

Just some more attention to the Note, I still don't quite get the LSP root thingy, where does this affect us?

Comment on lines 546 to 548
-- But according to https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_workspaceFolders
-- The root dir is deprecated, but we still need this now,
-- since a lot of places in the codebase still rely on it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I don't quite get this, which root dir is this now?

@soulomoon
Copy link
Collaborator Author

soulomoon commented May 27, 2024

Just some more attention to the Note, I still don't quite get the LSP root thingy, where does this affect us?

LSP root is deprecated, @michaelpj send me the link,
I guess that means we should cleanup dependency on the project root(Or $CWD) thing gradually, so muti workspaces can actually be supported when we use absolute path everywhere.

That might not be possible unless we have everything adapted to it, things like hlint and evaluation of template haskell.
But we should still be working towards the goal.

I'll add this to the note too.

@soulomoon soulomoon enabled auto-merge (squash) May 27, 2024 13:03
@soulomoon soulomoon merged commit 838a51f into haskell:master May 27, 2024
39 checks passed
@michaelpj
Copy link
Collaborator

I think we might want to seriously consider using one of the typed path libraries so we can assert that we have absolute paths everywhere. I agree it's just extremely unclear how you are supposed to deal with relative paths of any kind. There's also the issue that some of the tools we call like cabal are working directory sensitive, I don't know if we currently call cabal with --project-file or whatever we need to get it to run "as if" it were in a specific directory...

@fendor
Copy link
Collaborator

fendor commented May 28, 2024

Since we spawn a new process for cabal, we use https://hackage.haskell.org/package/process-1.6.20.0/docs/System-Process.html#t:CreateProcess to set the working directory for cabal, so that is unaffected.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Jan 31, 2025
## 2.9.0.0

- Bindists for GHC 9.10.1 by @wz1000, @jhrcek, @michaelpj
- More hls-graph reliability improvements by @soulomoon
- Refactoring of test suite runners by @soulomoon
- Fixes in multiple home units support by @wz1000

### Pull Requests

- Fix quadratic memory usage in GetLocatedImports
  ([#4318](haskell/haskell-language-server#4318)) by @mpickering
- Bump stack configs + CI to 9.6.5 and 9.8.2
  ([#4316](haskell/haskell-language-server#4316)) by @jhrcek
- Add support for Fourmolu 0.16
  ([#4314](haskell/haskell-language-server#4314)) by @ brandonchinn178
- Code action to remove redundant record field import (fixes #4220)
  ([#4308](haskell/haskell-language-server#4308)) by @battermann
- Use restricted monad for plugins (#4057)
  ([#4304](haskell/haskell-language-server#4304)) by @awjchen
- 4301 we need to implement utility to wait for all runnning keys in hls graph done
  ([#4302](haskell/haskell-language-server#4302)) by @soulomoon
- Call useWithStale instead of useWithStaleFast when calling ParseCabalFields
  ([#4294](haskell/haskell-language-server#4294)) by @VeryMilkyJoe
- test: add test documenting #806
  ([#4292](haskell/haskell-language-server#4292)) by @develop7
- ghcide: drop ghc-check and ghc-paths dependency
  ([#4291](haskell/haskell-language-server#4291)) by @wz1000
- Limit number of valid hole fits to 10
  ([#4288](haskell/haskell-language-server#4288)) by @akshaymankar
- Add common stanza to completion data
  ([#4286](haskell/haskell-language-server#4286)) by @VeryMilkyJoe
- FindImports: ThisPkg means some home unit, not "this" unit
  ([#4284](haskell/haskell-language-server#4284)) by @wz1000
- Remove redudant absolutization in session loader
  ([#4280](haskell/haskell-language-server#4280)) by @soulomoon
- Bump to new lsp versions
  ([#4279](haskell/haskell-language-server#4279)) by @michaelpj
- Put more test code into pre-commit
  ([#4275](haskell/haskell-language-server#4275)) by @soulomoon
- Delete library ghcide test utils
  ([#4274](haskell/haskell-language-server#4274)) by @soulomoon
- Delete testUtil from ghcide-tests
  ([#4272](haskell/haskell-language-server#4272)) by @soulomoon
- CI change, only run bench on performance label
  ([#4271](haskell/haskell-language-server#4271)) by @soulomoon
- Migrate WatchedFileTests
  ([#4269](haskell/haskell-language-server#4269)) by @soulomoon
- Migrate UnitTests
  ([#4268](haskell/haskell-language-server#4268)) by @soulomoon
- Migrate SafeTests
  ([#4267](haskell/haskell-language-server#4267)) by @soulomoon
- Migrate SymlinkTests
  ([#4266](haskell/haskell-language-server#4266)) by @soulomoon
- Remove unused and outdated CHANGELOG files
  ([#4264](haskell/haskell-language-server#4264)) by @fendor
- Enable cabal flaky test
  ([#4263](haskell/haskell-language-server#4263)) by @soulomoon
- Migrate RootUriTests
  ([#4261](haskell/haskell-language-server#4261)) by @soulomoon
- Migrate PreprocessorTests
  ([#4260](haskell/haskell-language-server#4260)) by @soulomoon
- Migrate PluginSimpleTests
  ([#4259](haskell/haskell-language-server#4259)) by @soulomoon
- Migrate ClientSettingsTests
  ([#4258](haskell/haskell-language-server#4258)) by @soulomoon
- Unify critical session running in hls
  ([#4256](haskell/haskell-language-server#4256)) by @soulomoon
- Bump cachix/cachix-action from 14 to 15
  ([#4255](haskell/haskell-language-server#4255)) by @dependabot[bot]
- Bump haskell-actions/setup from 2.7.2 to 2.7.3
  ([#4254](haskell/haskell-language-server#4254)) by @dependabot[bot]
- Bump haskell-actions/setup from 2.7.2 to 2.7.3 in /.github/actions/setup-build
  ([#4253](haskell/haskell-language-server#4253)) by @dependabot[bot]
- Shorter file names completion
  ([#4252](haskell/haskell-language-server#4252)) by @VenInf
- Fix progress start delay
  ([#4249](haskell/haskell-language-server#4249)) by @michaelpj
- Bump cachix/install-nix-action from 26 to 27
  ([#4245](haskell/haskell-language-server#4245)) by @dependabot[bot]
- Bump haskell-actions/setup from 2.7.1 to 2.7.2
  ([#4244](haskell/haskell-language-server#4244)) by @dependabot[bot]
- Bump haskell-actions/setup from 2.7.1 to 2.7.2 in /.github/actions/setup-build
  ([#4243](haskell/haskell-language-server#4243)) by @dependabot[bot]
- Enable test for #717
  ([#4241](haskell/haskell-language-server#4241)) by @soulomoon
- Remove Pepe from CODEOWNERS
  ([#4239](haskell/haskell-language-server#4239)) by @michaelpj
- Fix resultBuilt(dirty mechanism) in hls-graph
  ([#4238](haskell/haskell-language-server#4238)) by @soulomoon
- Support for 9.10
  ([#4233](haskell/haskell-language-server#4233)) by @wz1000
- Refactor hls-test-util and reduce getCurrentDirectory after initilization
  ([#4231](haskell/haskell-language-server#4231)) by @soulomoon
- [Migrate BootTests] part of #4173 Migrate ghcide tests to hls test utils
  ([#4227](haskell/haskell-language-server#4227)) by @soulomoon
- Actually enable pedantic flag in ci flags job
  ([#4224](haskell/haskell-language-server#4224)) by @jhrcek
- Cleanup cabal files, ghc compat code, fix ghc warnings
  ([#4222](haskell/haskell-language-server#4222)) by @jhrcek
- Another attempt at using the lsp API for some progress reporting
  ([#4218](haskell/haskell-language-server#4218)) by @michaelpj
- [Migrate diagnosticTests] part of #4173 Migrate ghcide tests to hls test utils
  ([#4207](haskell/haskell-language-server#4207)) by @soulomoon
- Prepare release 2.8.0.0
  ([#4191](haskell/haskell-language-server#4191)) by @wz1000
- Stabilize the build system by correctly house keeping the dirtykeys and rule values [flaky test #4185 #4093]
  ([#4190](haskell/haskell-language-server#4190)) by @soulomoon
- hls-cabal-plugin: refactor context search to use `readFields`
  ([#4186](haskell/haskell-language-server#4186)) by @fendor
- 3944 extend the properties api to better support nested configuration
  ([#3952](haskell/haskell-language-server#3952)) by @soulomoon

## 2.8.0.0

- Bindists for GHC 9.6.5
- New hls-notes plugin (#4126, @jvanbruegge)
- Floskell, hlint and stylish-haskell plugins enabled for GHC 9.8
- Improvements for hls-graph increasing robustness (#4087, @soulomoon)
- Improvements to multi-component support (#4096, #4109, #4179, @wz1000, @fendor)

### Pull Requests

- Bump haskell-actions/setup from 2.7.0 to 2.7.1
  ([#4189](haskell/haskell-language-server#4189)) by @dependabot[bot]
- Bump haskell-actions/setup from 2.7.0 to 2.7.1 in /.github/actions/setup-build
  ([#4188](haskell/haskell-language-server#4188)) by @dependabot[bot]
- Fix ghcdie-tests CI
  ([#4184](haskell/haskell-language-server#4184)) by @soulomoon
- Fix ghc and hlint warnings, fix formatting
  ([#4181](haskell/haskell-language-server#4181)) by @jhrcek
- Allow users to specify whether to use `cabal`'s multi-repl feature
  ([#4179](haskell/haskell-language-server#4179)) by @fendor
- Improve parsing of import suggestions extending multiple multiline imports (fixes #4175)
  ([#4177](haskell/haskell-language-server#4177)) by @jhrcek
- move ghcide-tests to haskell-language-server.cabal and make it depend on hls-test-utils
  ([#4176](haskell/haskell-language-server#4176)) by @soulomoon
- enable ThreadId for when testing
  ([#4174](haskell/haskell-language-server#4174)) by @soulomoon
- Drop Legacy Logger from Codebase
  ([#4171](haskell/haskell-language-server#4171)) by @fendor
- get rid of the `unsafeInterleaveIO` at start up
  ([#4167](haskell/haskell-language-server#4167)) by @soulomoon
- Remove EKG
  ([#4163](haskell/haskell-language-server#4163)) by @michaelpj
- Mark plugins as not buildable if the flag is disabled
  ([#4160](haskell/haskell-language-server#4160)) by @michaelpj
- Fix references to old CPP names in tests, update tests
  ([#4159](haskell/haskell-language-server#4159)) by @jhrcek
- Bump haskell-actions/setup from 2.6.3 to 2.7.0
  ([#4158](haskell/haskell-language-server#4158)) by @dependabot[bot]
- Bump haskell-actions/setup from 2.6.3 to 2.7.0 in /.github/actions/setup-build
  ([#4157](haskell/haskell-language-server#4157)) by @dependabot[bot]
- Remove dead code in ghcide and hls-graph for priority
  ([#4151](haskell/haskell-language-server#4151)) by @soulomoon
- Bump haskell-actions/setup from 2.6.2 to 2.6.3 in /.github/actions/setup-build
  ([#4150](haskell/haskell-language-server#4150)) by @dependabot[bot]
- Bump haskell-actions/setup from 2.6.2 to 2.6.3
  ([#4149](haskell/haskell-language-server#4149)) by @dependabot[bot]
- Run ExceptionTests in temporary directory
  ([#4146](haskell/haskell-language-server#4146)) by @fendor
- hls-eval-plugin: Replicate #4139
  ([#4140](haskell/haskell-language-server#4140)) by @mattapet
- Update comment in refactor tests
  ([#4138](haskell/haskell-language-server#4138)) by @jhrcek
- Update contact info in docs
  ([#4137](haskell/haskell-language-server#4137)) by @jhrcek
- hls-notes-plugin: Do not error if no note is under the cursor
  ([#4136](haskell/haskell-language-server#4136)) by @jvanbruegge
- improve logging in semantic tokens rule
  ([#4135](haskell/haskell-language-server#4135)) by @soulomoon
- Bump softprops/action-gh-release from 1 to 2
  ([#4133](haskell/haskell-language-server#4133)) by @dependabot[bot]
- Bump cachix/install-nix-action from 25 to 26
  ([#4132](haskell/haskell-language-server#4132)) by @dependabot[bot]
- Use Set.member instead of Foldable.elem
  ([#4128](haskell/haskell-language-server#4128)) by @jhrcek
- hls-notes-plugin: Initial implementation
  ([#4126](haskell/haskell-language-server#4126)) by @jvanbruegge
- Enable floskell and hlint plugins for ghc 9.8
  ([#4125](haskell/haskell-language-server#4125)) by @jhrcek
- Integrate stylish-haskell into hls executable with ghc 9.8
  ([#4124](haskell/haskell-language-server#4124)) by @jhrcek
- Reduce usage of partial functions
  ([#4123](haskell/haskell-language-server#4123)) by @jhrcek
- Benchmark: Enable 9.6, 9.8
  ([#4118](haskell/haskell-language-server#4118)) by @soulomoon
- Bump haskell-actions/setup from 2.6.1 to 2.6.2 in /.github/actions/setup-build
  ([#4116](haskell/haskell-language-server#4116)) by @dependabot[bot]
- Bump haskell-actions/setup from 2.6.1 to 2.6.2
  ([#4115](haskell/haskell-language-server#4115)) by @dependabot[bot]
- eval: more robust way to extract comments from ParsedModule
  ([#4113](haskell/haskell-language-server#4113)) by @jhrcek
- Improve isolation of build artefacts of test runs
  ([#4112](haskell/haskell-language-server#4112)) by @fendor
- Improve handling of nonsense rename attempts
  ([#4111](haskell/haskell-language-server#4111)) by @jhrcek
- Exit with non-zero exitcode if wrapper fails to launch
  ([#4110](haskell/haskell-language-server#4110)) by @fendor
- Replace checkHomeUnitsClosed with a faster implementation
  ([#4109](haskell/haskell-language-server#4109)) by @wz1000
- Don't distribute gifs or plugin readmes
  ([#4107](haskell/haskell-language-server#4107)) by @fendor
- Remove locale workaround for Module name that conatins non-ascii characters
  ([#4106](haskell/haskell-language-server#4106)) by @fendor
- Track extra-source-files of plugins more accurately
  ([#4105](haskell/haskell-language-server#4105)) by @fendor
- remove non-ascii name
  ([#4103](haskell/haskell-language-server#4103)) by @soulomoon
- Add cabal-gild as a cabal file formatter plugin
  ([#4101](haskell/haskell-language-server#4101)) by @fendor
- Remove more workarounds for GHCs < 9.2 (#4092)
  ([#4098](haskell/haskell-language-server#4098)) by @jhrcek
- session-loader: Don't loop forever when we don't find a file in any multi component
  ([#4096](haskell/haskell-language-server#4096)) by @wz1000
- Prepare release 2.7.0.0
  ([#4095](haskell/haskell-language-server#4095)) by @fendor
- Remove more workarounds for GHCs < 9.0
  ([#4092](haskell/haskell-language-server#4092)) by @jhrcek
- Fix hls-graph: phantom dependencies invoke in branching deps (resolve #3423)
  ([#4087](haskell/haskell-language-server#4087)) by @soulomoon
- Rename only if the current module compiles (#3799)
  ([#3848](haskell/haskell-language-server#3848)) by @sgillespie
- Reintroduce ghc-lib flag for hlint plugin
  ([#3757](haskell/haskell-language-server#3757)) by @RaoulHC
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flaky test status: needs review This PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants