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

Make sure running tests locally pick up the correct cradle type #3869

Merged
merged 3 commits into from
Nov 15, 2023

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented Nov 14, 2023

If developers have a local hie.yaml for HLS development, this causes the tests to pick up said hie.yaml. This causes these tests to use a cabal cradle, slowing down the test execution. Especially annoying in conjunction with the cabal issue haskell/cabal#8930.

We make sure that all tests have a custom hie.yaml or are copied outside the project directory.
Should have no effect on CI, though, which never has a hie.yaml in the root of the project.

If developers have a local `hie.yaml` for HLS development, this causes
the tests to pick up said `hie.yaml`. This causes these tests to use a
cabal cradle, slowing down the test execution. Should have no effect on
CI, though, which never has a `hie.yaml` in the root of the project.
@fendor fendor changed the title Make sure local tests pick up the correct cradle types. Make sure running tests locally pick up the correct cradle type Nov 14, 2023
@michaelpj
Copy link
Collaborator

Is this the right branch? I don't see any hie.yamls?

@fendor
Copy link
Collaborator Author

fendor commented Nov 15, 2023

Is this the right branch? I don't see any hie.yamls?

Locally, I have a simple cabal cradle:

cradle:
  cabal:

That's what is causing the issue for local development, if you happen to write a hie.yaml file.

@michaelpj
Copy link
Collaborator

I mean, I don't see how the changes in this PR correspond to what you wrote in the description? Or maybe I'm being dumb?

@fendor
Copy link
Collaborator Author

fendor commented Nov 15, 2023

If it is unclear, then I need to make sure I express myself better :D

When running tests, we should make sure that tests use a direct cradle unless they test a cabal-specific feature for performance but also test flakiness reasons. In CI, the HLS repository does not have a hie.yaml file. Thus, if the tests do not have an explicit hie.yaml file, the implicit cradle logic autodetects the default cradle (or the local cabal package hls-refactor-plugin, not totally sure).

However, when you develop HLS, you may have a locally defined hie.yaml file. The cradle autodetection mechanism in a test without an explicit hie.yaml will pick up the hie.yaml file in the project root and infer that the tests are executed in the context of the HLS main repo. This slows down the tests immensely, since there are multiple cabal calls involved, and may fail since the testdata is not actually part of the cabal project.

This PR makes sure that the add argument tests of the hls-refactor-plugin do have an explicit hie.yaml file, avoiding this issue when the developer (in this particular instance, me) has a hie.yaml file defined.
We make use of the virtual file tree system to copy tests into temporary directories and create a direct cradle.

@michaelpj
Copy link
Collaborator

Ah okay, so it's just the add-argument test. Is it not a problem for other tests?

@fendor
Copy link
Collaborator Author

fendor commented Nov 15, 2023

The other tests already copy the test data into a temporary directory and run the tests in there. I want to migrate these to the file tree feature I implemented a couple of months ago, too.
Currently I am mostly making sure I can reliably run the tests locally.

@fendor fendor added the merge me Label to trigger pull request merge label Nov 15, 2023
@mergify mergify bot merged commit 5a923ce into haskell:master Nov 15, 2023
45 checks passed
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants