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

Using captureKicksDiagnostics to speed up multiple plugin tests #4339

Merged
merged 16 commits into from
Aug 2, 2024

Conversation

komikat
Copy link
Contributor

@komikat komikat commented Jun 27, 2024

Extending #4144 to cabal-plugin-tests and merging upstream changes on hlint-plugin-tests
Update: Extending to multiple plugins as noted below.
Update 2: Scope reverted to cabal and hlint plugins. (ref #4371)

TODO

  • update the remaining tests which waitForDiagnostics unconditionally
  • resolve the one hlint-plugin test failing after the update
  • fix pre-commit issues

fendor and others added 3 commits March 22, 2024 16:01
Move test data to temporary directory.
Avoid `waitForDiagnosticsWithSource` as it unconditionally waits for
diagnostics.
@komikat
Copy link
Contributor Author

komikat commented Jun 27, 2024

hey @soulomoon, any suggestions?

@komikat komikat changed the title Using captureKicksDiagnostics to speed up hls-cabal-plugin-tests [WIP] Using captureKicksDiagnostics to speed up hls-cabal-plugin-tests Jun 27, 2024
@fendor
Copy link
Collaborator

fendor commented Jun 27, 2024

@komikat Did you investigate the failing test case?

@komikat
Copy link
Contributor Author

komikat commented Jun 27, 2024

Yes, I'm looking into it, will hopefully have something by tomorrow.

PS: the failing testcase is ignoreHintGoldenResolveTest.

@fendor
Copy link
Collaborator

fendor commented Jun 27, 2024

This looks like... the testcase is wrong? I can't explain it, but changing "Eta reduce" with "Redundant id" works for me locally. The testcase itself is still wrong, you have to accept the differences, but no unknown code actions any more.

@fendor
Copy link
Collaborator

fendor commented Jun 27, 2024

Perhaps we lack a .hlint.yaml in the temp directory or something?

@soulomoon
Copy link
Collaborator

@komikat since I do not familiar with cabal plugin, I can only give very general suggestions. Take consideration of the following.

  1. the time gap the debouncer needed to emit the diagnostic
  2. Be sure the number of cabal kicks that would happened in the test logic. Seems like @fendor 's new finding suggest some incorrect logic in the test case?

@komikat
Copy link
Contributor Author

komikat commented Jun 30, 2024

hlint --hint=test-hlint-config.yaml UnrecognizedPragmasOff.hs
only returns Eta Reduce so it is a hlint config file issue

@komikat
Copy link
Contributor Author

komikat commented Jun 30, 2024

Setting .hlint.yaml correctly breaks the testcase "provides 3.8 code actions including apply all" while not fixing this one at the same time, seems like the .hlint.yaml might not have been copied correctly for the unrecognized pragma test case.

Will also have to look into "provides 3.8 code actions including apply all" since it's correctly failing after ignoring redundant id, it expects both redundant id and eta reduce currently.

@komikat
Copy link
Contributor Author

komikat commented Jul 10, 2024

Moved hlint tests to the virtual file system and fixed a config issue with the resolveTests.
hlint-plugin-tests takes 15s now.

@komikat komikat marked this pull request as ready for review July 10, 2024 20:03
@komikat komikat changed the title [WIP] Using captureKicksDiagnostics to speed up hls-cabal-plugin-tests Using captureKicksDiagnostics to speed up hls-cabal-plugin-tests Jul 10, 2024
Copy link
Collaborator

@soulomoon soulomoon left a comment

Choose a reason for hiding this comment

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

good work!

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.

Thanks for pushing this over the finish line! Please also make sure to remove dead code, e.g. code that is commented out.

plugins/hls-cabal-plugin/src/Ide/Plugin/Cabal.hs Outdated Show resolved Hide resolved
plugins/hls-cabal-plugin/test/Utils.hs Show resolved Hide resolved
plugins/hls-hlint-plugin/src/Ide/Plugin/Hlint.hs Outdated Show resolved Hide resolved
plugins/hls-hlint-plugin/test/Main.hs Outdated Show resolved Hide resolved
@komikat komikat changed the title Using captureKicksDiagnostics to speed up hls-cabal-plugin-tests Using captureKicksDiagnostics to speed up multiple plugin tests Jul 19, 2024
@komikat
Copy link
Contributor Author

komikat commented Jul 19, 2024

Updated PR scope, please refer TODO.

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, doing a final CI run to make sure the tests didn't get more flaky

@komikat
Copy link
Contributor Author

komikat commented Aug 2, 2024

seems like the timeout in PluginSimpleTests still happens erratically (unmarked as broken in #4259)

@fendor fendor enabled auto-merge (squash) August 2, 2024 09:41
@fendor fendor merged commit 9565d0b into haskell:master Aug 2, 2024
36 checks passed
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