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.

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

Set the root for tests to the test directory #3830

Merged
merged 2 commits into from
Oct 28, 2023
Merged

Set the root for tests to the test directory #3830

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 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

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
39 of 42 checks passed
@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
Reviewers

@michaelpj michaelpj michaelpj approved these changes

@pepeiborra pepeiborra Awaiting requested review from pepeiborra

Assignees
No one assigned
Labels
merge me Label to trigger pull request merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fails to load with local tarballs in cabal.project
2 participants
@fendor @michaelpj