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

Add GitHub Actions CI for testing #504

Merged
merged 13 commits into from
Oct 19, 2020
Merged

Add GitHub Actions CI for testing #504

merged 13 commits into from
Oct 19, 2020

Conversation

lukel97
Copy link
Collaborator

@lukel97 lukel97 commented Oct 14, 2020

Following the discussion in #6 , it looks like we should probably move away from CircleCI at some point. This adds GitHub Actions CI workflows, reusing the infrastructure we already have for the static binaries. It should run the tests on linux, macOS and windows now.

@jneira
Copy link
Member

jneira commented Oct 14, 2020

We will need haskell-language-server executable (without make the exe name shorter)

@jneira jneira self-requested a review October 14, 2020 21:27
@jneira
Copy link
Member

jneira commented Oct 15, 2020

It seems we need stack on path to make wrapper tests pass?

EDIT
In fact it is the exe wrapper itself, we would need to make the env var workaround for it too but why we are keeping the executables with shortened names around? We could use the original ones, no?

EDIT2
Ahh, it will fail in windows cause build-tool-depends uses the executable from dist-newstyle 😟

@lukel97
Copy link
Collaborator Author

lukel97 commented Oct 16, 2020

@jneira I've added another environment var HLS_WRAPPER_TEST_EXE, seems to work ok for now. Hopefully we can get rid of this whenever ghc supports the windows long paths extension!

@lukel97
Copy link
Collaborator Author

lukel97 commented Oct 16, 2020

Looks like those windows failures are genuine. Opened up a quick PR to hopefully fix them here: haskell/hie-bios#256

@lukel97
Copy link
Collaborator Author

lukel97 commented Oct 16, 2020

Not sure about these ones though:

expected: "module T17 where\n\n-- >>> :type  +no 42\n-- parse error on input \8216+\8217\n"
but got: "module T17 where\n\n-- >>> :type  +no 42\n-- parse error on input `+'\n"

Is this ghc outputting a different encoding or can the tests set anything to encode it as the one below? @jneira

@konn
Copy link
Collaborator

konn commented Oct 17, 2020

IIRC, GHC changes quotation marks according to the locale settings. I encountered the same behaviour when I contributed to https://github.com/nomeata/inspection-testing

@jneira
Copy link
Member

jneira commented Oct 17, 2020

Yeah, hie had similar failing tests too.
Afair not all ghc versions has the same behaviour so ignore quotation marks usually is the best bet.

@jneira
Copy link
Member

jneira commented Oct 18, 2020

I think delimiters used by ghc in windows is due to lack of support for utf-8 in windows shell by default. So to avoid output weird replacements they decided to use ascii delimiters for windows.
In ghcide recently i remove the delimiters from the expected string and in hie i duplicated expected strings, trying both variants always. It is ugly but somewhat worked.

@lukel97
Copy link
Collaborator Author

lukel97 commented Oct 18, 2020

@jneira thats the approach I've ended up using, I went down a rabbit hole today of trying to understand what decides the default localeEncoding but didn't get anywhere. If this gets merged we should probably squash all my trial and error commits!

@lukel97 lukel97 requested review from Ailrun, alanz and fendor October 18, 2020 23:05
env:
HLS_TEST_EXE: hls
HLS_WRAPPER_TEST_EXE: hls-wrapper
run: cabal test --test-options=-j1
Copy link
Member

Choose a reason for hiding this comment

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

did you add -j1 due a concrete issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Out of precaution, the tests previously failed on circleci because without -j1 tasty attempts to run all the functional tests in parallel, which means it spins up 20 or so hls instances simultaneously. I'm not sure if the GitHub runners can handle it or not but the circleCI runners definitely couldn't!

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it worths a comment?

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

Amazing work, as usual. Many thanks, specially for take care of minority os's.

@lukel97 lukel97 merged commit cdf50a6 into haskell:master Oct 19, 2020
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.

3 participants