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

[Migrate diagnosticTests] part of #4173 Migrate ghcide tests to hls test utils #4207

Merged
merged 25 commits into from
May 13, 2024
Merged

[Migrate diagnosticTests] part of #4173 Migrate ghcide tests to hls test utils #4207

merged 25 commits into from
May 13, 2024

Conversation

soulomoon
Copy link
Collaborator

@soulomoon soulomoon commented May 4, 2024 •
edited
Loading

  • migrate diagnosticTests, figure out how to pass --test-no-kick.
  • migrate openCloseTests
  • Lift a few functions from ghcide-test-utils to hls-test-utils
  • fixed deeply nested cyclic module dependency
  • modify runSessionWithServer' and ghcIde arguments to admit additional a switch for no-kick.

michaelpj
michaelpj reviewed May 4, 2024
View reviewed changes
ghcide/test/exe/Config.hs
@@ -31,9 +34,15 @@ runWithDummyPlugin' = runSessionWithServerInTmpDirCont' def dummyPlugin
runWithDummyPluginAndCap :: ClientCapabilities -> Session () -> IO ()
runWithDummyPluginAndCap cap = runSessionWithServerAndCapsInTmpDir def dummyPlugin cap (mkIdeTestFs [])

runWithDummyPluginAndCap' :: ClientCapabilities -> (FileSystem -> Session ()) -> IO ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW, I'm not sure all these functions are necessarily worth it. How bad would it be to just have the FileSystem -> Session () variants and then write _ -> whatever at the use sites?

Copy link
Collaborator Author

@soulomoon soulomoon May 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Emm, we are at somewhat funny situation, that the one without the extra FileSystem are the most used.
Sure, for not frequent used ones, it would be fine we write _ -> whatever.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess I'm wondering how bad it would be. Otherwise we have a lot of very similar variant functions 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a blocker, anyway

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, I mean since we have a lot of test that does not need the FileSystem ,
adding a tons of _ -> whatever is visually bad and and not convenient for us to do the migration.

That is why I want to either do the typeclass polymorphic or just duplicate the test runner function variant.

@soulomoon
Copy link
Collaborator Author

soulomoon commented May 5, 2024 •
edited
Loading

It seems pass --test-no-kick might require a lot of code change in setup in hls-test-utils, I don't know if there is alternative to this.

@soulomoon soulomoon added the WIP label May 5, 2024
@michaelpj
Copy link
Collaborator

What's the significance of test-no-kick?

@soulomoon
Copy link
Collaborator Author

What's the significance of test-no-kick?

My guess is that so the test can trigger the typecheck manually without a lot of rules being triggered by kick and hence capture the diagnostic in a more stable way

@soulomoon soulomoon mentioned this pull request May 10, 2024
Closed
31 tasks
@soulomoon soulomoon marked this pull request as ready for review May 10, 2024 03:08
@soulomoon
Copy link
Collaborator Author

soulomoon commented May 10, 2024 •
edited
Loading

Two new things have been done, I modify runSessionWithServer' and ghcIde arguments to admit additional a switch for no-kick. And also some setup rearrangement. To support the migration of diagnosticTests
Additionally, I put more test environment variables into pluginTestRecorder so we can use pluginTestRecorder to generate test recorder generically, any reason not to ?

@soulomoon soulomoon added status: in discussion Not actionable, because discussion is still ongoing or there's no decision yet status: needs review This PR is ready for review and removed WIP labels May 10, 2024
fendor
fendor requested changes May 10, 2024
View reviewed changes
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.

Changes look generally good. Some nitpicks, and the environment variables need to be discussed!

ghcide/src/Development/IDE/Main.hs Outdated Show resolved Hide resolved
ghcide/test/exe/DiagnosticTests.hs Outdated Show resolved Hide resolved
ghcide/test/exe/DiagnosticTests.hs Outdated Show resolved Hide resolved
hls-test-utils/src/Test/Hls.hs Outdated Show resolved Hide resolved
hls-test-utils/src/Test/Hls.hs Outdated Show resolved Hide resolved
hls-test-utils/src/Test/Hls.hs Outdated Show resolved Hide resolved
hls-test-utils/src/Test/Hls.hs Outdated Show resolved Hide resolved
hls-test-utils/src/Test/Hls.hs Outdated Show resolved Hide resolved
plugins/hls-refactor-plugin/test/Main.hs Outdated Show resolved Hide resolved
plugins/hls-refactor-plugin/test/Main.hs Outdated Show resolved Hide resolved
@soulomoon soulomoon changed the title Migrate diagnosticTests [Migrate diagnosticTests] part of #4173 Migrate ghcide tests to hls test utils May 11, 2024
fendor
fendor approved these changes May 12, 2024
View reviewed changes
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.

LGTM, thank you very much!

Only one comment nitpick and a thought :)

ghcide/test/exe/DiagnosticTests.hs Outdated Show resolved Hide resolved
ghcide/test/exe/DiagnosticTests.hs Outdated Show resolved Hide resolved
hls-test-utils/src/Test/Hls.hs Outdated Show resolved Hide resolved
hls-test-utils/src/Test/Hls.hs
@@ -419,22 +435,21 @@ runSessionWithServerInTmpDir' plugins conf sessConf caps tree act = runSessionWi
--
-- Note: cwd will be shifted into a temporary directory in @Session a@
runSessionWithServerInTmpDirCont ::
Pretty b =>
-- | whether we disable the kick action or not
Bool ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks a little bit ad-hoc, perhaps we might want a full TestConfig record in the future?
Just a thought, let's leave it as is, it is easy to refactor later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I am thinking about it too. perhaps tucking in things like Config, SessionConfig, ClientCapabilities into the one TestConfig too.

@soulomoon soulomoon enabled auto-merge (squash) May 12, 2024 22:56
@soulomoon soulomoon merged commit 61fd5c4 into haskell:master May 13, 2024
39 checks passed
@soulomoon soulomoon mentioned this pull request May 18, 2024
Merged
5 tasks
@BrewTestBot BrewTestBot mentioned this pull request Jun 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fendor fendor fendor approved these changes

@santiweight santiweight Awaiting requested review from santiweight santiweight is a code owner

@pepeiborra pepeiborra Awaiting requested review from pepeiborra

@michaelpj michaelpj Awaiting requested review from michaelpj

Assignees
No one assigned
Labels
status: in discussion Not actionable, because discussion is still ongoing or there's no decision yet status: needs review This PR is ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@soulomoon @michaelpj @fendor