Skip to content

Set the root for tests to the test directory #3830

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.

Sign up for GitHub

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

Jump to bottom
Merged
fendor merged 2 commits into from
Oct 28, 2023
Merged

Set the root for tests to the test directory #3830

fendor merged 2 commits into from
Oct 28, 2023

Conversation

@fendor
Copy link
Collaborator

@fendor fendor commented Oct 6, 2023

On startup, HLS performs a sanity check to validate the GHC version for the project is the same as the GHC version used to build HLS. Due to some oversights, the tests always use HLS's cabal package to perform this check.
This is (almost) valid for the tests but adds overhead for finding the GHC version for each integration test in HLS.
Improves the startup time for each integration test that depends on hls-test-utils. Thus, ghcide test suite is unaffected.

Fix findCradle invocations to point to a file in the root directory. findCradle from hie-bios doesn't work correctly for directories and requires a file, even if that file doesn't exist.

Incidentally, I think this fixes #3074, too.

We should fix hie-bios's findCradle to also work for directories. If this PR works as hoped, I'll update hie-bios.
@fendor
Set the root for tests to the test directory
2747a15 
On startup, HLS performs a sanity check to validate the GHC version for the project is the same as the GHC version used to build HLS. Due to some oversights, the tests always use HLS's cabal package to perform this check. This is (almost) valid for the tests but adds overhead for finding the GHC version for each integration test in HLS. Improves the startup time for each integration tests that depends on hls-test-utils. Thus, ghcide test suite is unaffected. Fix `findCradle` invocations to point to a file in the root directory. `findCradle` from hie-bios doesn't work correctly for directories and requires a file, even if that file doesn't exist.
@fendor fendor requested a review from pepeiborra as a code owner October 6, 2023 10:02
michaelpj
michaelpj reviewed Oct 9, 2023
View reviewed changes
michaelpj
michaelpj approved these changes Oct 9, 2023
View reviewed changes
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.

I'm amazed anything worked without this. Don't a bunch of the tests have their own hie.yamls? So doesn't this mean we were ignoring them all because we were using the top-level cradle?
@fendor
Copy link
Collaborator Author

fendor commented Oct 9, 2023 •
edited
Loading

So doesn't this mean we were ignoring them all because we were using the top-level cradle?

No, I think, only to make sure HLS is compiled against the correct GHC version.
Basically a sanity check.

For running the tests, the proper hie.yaml files are used.
@michaelpj
Copy link
Collaborator

michaelpj commented Oct 9, 2023

For running the tests, the proper hie.yaml files are used.

But when we start the server in the test utils we were setting the wrong root? I don't see how that wouldn't end up running in the wrong place entirely?
@fendor fendor mentioned this pull request Oct 9, 2023
Merged
@fendor
Copy link
Collaborator Author

fendor commented Oct 9, 2023 •
edited
Loading

I am not entirely sure, as in "I haven't checked", but a Cradle produces the cradle root directory and HLS should be able to handle this easily. The only reason why it might not work correctly is that it might infer the wrong GHC version for a project, e.g., when it is a stack project, ...
This is a problem in practice, but not in CI as we make sure that all cradles (not sure if we still test stack cradles) use the same GHC version.
@fendor
Copy link
Collaborator Author

fendor commented Oct 9, 2023

Hie-bios PR haskell/hie-bios#415
@fendor fendor enabled auto-merge October 28, 2023 15:04
@fendor fendor added the merge me Label to trigger pull request merge label Oct 28, 2023
@fendor fendor merged commit 5bf1b75 into haskell:master Oct 28, 2023
@Porkepix Porkepix mentioned this pull request Dec 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge me Label to trigger pull request merge

2 participants

@fendor @michaelpj