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

Extract fourmolu plugin into a standalone package #1823

Merged
merged 1 commit into from
May 14, 2021

Conversation

gustavoavena
Copy link
Contributor

@gustavoavena gustavoavena commented May 12, 2021

What

Extract fourmolu plugin to a standalone package.
Similar to what was done for stylish-haskell (#1604) and brittany (#1422) plugins.

RFC: Test case in Progress.hs

TL;DR: is it ok to leave the progress tests case from fourmolu plugin where it is?

I'm basing this PR on the ones that did the same for stylish-haskell (#1604) and brittany (#1422). I moved the functional tests from the Format test suite the same way, but unlike stylish-haskell and brittany, fourmolu has a test case in window/workDoneProgress test suite (in test/functional/Progress.hs).

My initial plan was to extract it to the fourmolu test suite, but it depends on some things from utility modules that are not visible to the new fourmolu test suite (e.g. hlsCommand from Test.Hls.Command).

It seems that to extract it, I would need to move some things around to avoid adding a dependency from the new test suite to the functional test suite. Since I wanted to avoid making bigger changes on my first PR, I wanted to double check if extracting that one test case is necessary, if it's passing as is.

@pepeiborra pepeiborra requested a review from berberman May 12, 2021 14:08
nix/default.nix Outdated Show resolved Hide resolved
@gustavoavena gustavoavena force-pushed the fourmolu_plugin branch 2 times, most recently from 2787573 to 5576997 Compare May 12, 2021 14:50
@pepeiborra
Copy link
Collaborator

pepeiborra commented May 12, 2021

My initial plan was to extract it to the fourmolu test suite, but it depends on some things from utility modules that are not visible to the new fourmolu test suite (e.g. hlsCommand from Test.Hls.Command). It seems that to extract it, I would need to move some things around to avoid adding a dependency from the new test suite to the functional test suite. Since I wanted to avoid making bigger changes on my first PR, I wanted to double check if extracting that one test case is necessary, if it's passing as is.

@gustavoavena nothing should be moved and hlsCommand should not be needed, but maybe some tests need to be refactored.
@berberman how difficult is it to extract the tests? I would think it should be pretty easy after your recent changes introducing his-test-utils.

@gustavoavena
Copy link
Contributor Author

gustavoavena commented May 12, 2021

@gustavoavena can you specify what needs to be moved? Maybe some tests need to be refactored instead.

One thing would be hlsCommand (from Test.Hls.Command). formatLspConfig and progressCaps are also used by the test case. There is also expectProgressReports.

I'm not very familiar with the code, but maybe refactoring the runServer* functions in Test.Hls to provide one that we could use for this test case would also work.

@berberman
Copy link
Collaborator

The purpose of hls-test-utils is to decouple plugins test suite with HLS executable, and we should never use hlsCommand in plugin packages. hlsCommand will be removed finally when we migrate all HLS tests (run them in the same process, without building server executable first), but it seems to take a bit more work to do (#1752).

Moreover, now there are some test suites in HLS referencing plugins' test data, e.g. Prograss mentioned above:

, testCase "eval plugin sends progress reports" $
runSession hlsCommand progressCaps "plugins/hls-eval-plugin/test/testdata" $ do
doc <- openDoc "T1.hs" "haskell"
expectProgressReports ["Setting up testdata (for T1.hs)", "Processing", "Indexing"]
[evalLens] <- getCodeLenses doc
let cmd = evalLens ^?! L.command . _Just
_ <- sendRequest SWorkspaceExecuteCommand $ ExecuteCommandParams Nothing (cmd ^. L.command) (decode $ encode $ fromJust $ cmd ^. L.arguments)
expectProgressReports ["Evaluating"]
, testCase "ormolu plugin sends progress notifications" $ do
runSession hlsCommand progressCaps "test/testdata/format" $ do
sendNotification SWorkspaceDidChangeConfiguration (DidChangeConfigurationParams (formatLspConfig "ormolu"))
doc <- openDoc "Format.hs" "haskell"
expectProgressReports ["Setting up testdata (for Format.hs)", "Processing", "Indexing"]
_ <- sendRequest STextDocumentFormatting $ DocumentFormattingParams Nothing doc (FormattingOptions 2 True Nothing Nothing Nothing)
expectProgressReports ["Formatting Format.hs"]
, testCase "fourmolu plugin sends progress notifications" $ do
runSession hlsCommand progressCaps "test/testdata/format" $ do
sendNotification SWorkspaceDidChangeConfiguration (DidChangeConfigurationParams (formatLspConfig "fourmolu"))
doc <- openDoc "Format.hs" "haskell"
expectProgressReports ["Setting up testdata (for Format.hs)", "Processing", "Indexing"]
_ <- sendRequest STextDocumentFormatting $ DocumentFormattingParams Nothing doc (FormattingOptions 2 True Nothing Nothing Nothing)
expectProgressReports ["Formatting Format.hs"]

Which is a bit hacky but works. Unfortunately, I don't have a good idea so far.

@gustavoavena Could you try changing the path "test/testdata/format" to your extracted test data, like eval plugin, and leaving other things in Prograss unchanged? P.S. There might be other HLS test suites like Prograss which reference fourmolu test data, and you'll need change them as well.

@gustavoavena gustavoavena force-pushed the fourmolu_plugin branch 3 times, most recently from 075e3c5 to 580de2a Compare May 13, 2021 09:29
@gustavoavena gustavoavena marked this pull request as ready for review May 13, 2021 11:03
@gustavoavena gustavoavena changed the title [WIP] Extract fourmolu plugin into a standalone package Extract fourmolu plugin into a standalone package May 13, 2021
@pepeiborra pepeiborra requested a review from berberman May 13, 2021 13:29
Copy link
Collaborator

@berberman berberman left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

plugins/hls-fourmolu-plugin/hls-fourmolu-plugin.cabal Outdated Show resolved Hide resolved
test/functional/Format.hs Outdated Show resolved Hide resolved
Copy link
Member

@Ailrun Ailrun left a comment

Choose a reason for hiding this comment

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

LGTM :)

@Ailrun Ailrun added the merge me Label to trigger pull request merge label May 14, 2021
@pepeiborra pepeiborra merged commit 0772f2d into haskell:master May 14, 2021
georgefst added a commit to georgefst/haskell-language-server that referenced this pull request May 20, 2021
Nobody likes `PackageImports`. And it's unnecessary since haskell#1823.
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.

4 participants