Skip to content

Include testdata hie.yaml cradles #399

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

Closed
wants to merge 1 commit into from

Conversation

isovector
Copy link
Collaborator

My tests were mysteriously failing on CI but not locally. Turns out the cradle is necessary for getting tests to run.

This also correlates with how the context functional tests are timing out (no hie.yaml checked in) but eval works just fine (has a hie.yaml).

My tests were mysteriously failing on CI but not locally. Turns out the cradle is necessary for getting tests to run. 

This also correlates with how the `context` functional tests are timing out (no hie.yaml checked in) but `eval` works just fine (has a hie.yaml).
@fendor
Copy link
Collaborator

fendor commented Sep 13, 2020

Shouldn't these be generated as needed?

@isovector
Copy link
Collaborator Author

@fendor if they should be, they currently aren't.

@fendor
Copy link
Collaborator

fendor commented Sep 13, 2020

Since you are not adding them to the git repo, I am wondering why it works when you stop ignoring them?

@fendor
Copy link
Collaborator

fendor commented Sep 13, 2020

I think the issue is here: https://github.com/haskell/haskell-language-server/blob/master/test/utils/Test/Hls/Util.hs#L71

We did not have time to get our existing test-setup into shape since the merge with ghcide.

@isovector
Copy link
Collaborator Author

I'm not adding them to the git repo, but this gitignore caused me to spend 3 hours chasing my tail trying to figure out why the hell CI was broken but my tests worked locally. This PR doesn't fix anything, but it brings attention and will save future plugin authors pain.

@fendor
Copy link
Collaborator

fendor commented Sep 13, 2020

But you should not commit them right?

I am sorry this caused so much pain for you, could you explain why it was hard to debug? We should improve this situation for everyone!

@fendor fendor requested a review from lukel97 September 13, 2020 19:21
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.

I am fine with it, but we should tackle the issue of getting the tests up-to-date

Copy link
Collaborator

@lukel97 lukel97 left a comment

Choose a reason for hiding this comment

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

Should we be calling setupBuildToolFiles at the start of these tests instead to fix this?

@fendor
Copy link
Collaborator

fendor commented Sep 24, 2020

Isnt it called?
oh well. Yes, we should.

@jneira
Copy link
Member

jneira commented Oct 5, 2020

I infer from the discussion that this pr should be closed o changed to add setupBuildToolFiles instead unignore hie.yaml files, right?
//cc @isovector

@lukel97
Copy link
Collaborator

lukel97 commented Oct 5, 2020

@jneira yes that would seem like the root cause. But now I'm stumped as to how the tests are working in the first place even

@jneira
Copy link
Member

jneira commented Oct 19, 2020

But now I'm stumped as to how the tests are working in the first place even

@bubba @fendor @isovector me too, analyzing the test code and data:

  • Most tests are being ignored so although there is no hie.yaml, they pass
  • Working tests (eval, tactic, etc) have a hie.yaml in its subdirectory, despite they were intended to be ignored with

# ignore hie.yaml's for testdata
test/testdata/**/hie.yaml

I suppose they were added with git add --force. 😺

  • Moreover the hie.yaml generated by setupBuildToolFiles will not work anymore, as they should include modules explicitly

So, i would ignore hie.yaml's but:

  • setupBuildToolFiles has to include auto all modules in the directory where it is about to be created (not sure if it would be feasible)
  • specific tests should call a version of setupDirectFilesIn with a new parameter with the modules to add in the direct cradle
    • and remove the generic setupBuildToolFiles

@jneira
Copy link
Member

jneira commented Oct 19, 2020

I am gonna implement the second appoach in the hlint pull request, if you agree (but without touching existing working tests)

Finally i've added hie.yaml forcily in the pr to go forward. The fix should be in another pr.

@jneira
Copy link
Member

jneira commented Oct 19, 2020

Opened #517 to track the issue with testdata hie.yaml.
@isovector what do you think about close this pr as, hopefully, another one will be opened to fix the issue taking another path?

@isovector isovector closed this Oct 19, 2020
@isovector isovector deleted the patch-1 branch May 21, 2021 16:43
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 this pull request may close these issues.

4 participants