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

Speed up hls-hlint-plugin-tests #4144

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented Mar 19, 2024

Migrate hls-hlint-plugin-tests to use VirtualFileTree infrastructure for test data.
Avoid waitForDiagnosticsWithSource which makes the test wait for 5 seconds, even though it might have received the diagnostics already.

@fendor
Copy link
Collaborator Author

fendor commented Mar 19, 2024

This doesn't quite work yet, as the test asserts sometimes that no diagnostic is emitted from the very beginning. However, on initial load, HLS doesn't publish empty diagnostics, so we don't know when processing finished. We probably need to insert some dummy diagnostics, and clear them.

LSP.sendNotification (LSP.SMethod_CustomMethod msg) $
toJSON $ map fromNormalizedFilePath files

signal (Proxy @"kick/start/hlint")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we do this generically? Like, for every rule, send custom method notifications for starting and finishing? It seems like many (most?) tests want this sort of thing, just having a generic signal when rules start or finish would be quite handy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think only diagnostic tests need it, but yeah, we can definitely generalise it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I support it, since we are already tracing the action any way, see otTracedAction.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could add a log handler that intercepts rule start/finish logs and sends the custom messages, like we do for sending window/logMessage etc.

@fendor fendor marked this pull request as draft March 22, 2024 09:55
Move test data to temporary directory.
Avoid `waitForDiagnosticsWithSource` as it unconditionally waits for
diagnostics.
@fendor
Copy link
Collaborator Author

fendor commented Mar 22, 2024

Tests are much quicker now, 15 seconds instead of 2 minutes! Just not sure how to refactor this to make it reusable 😅

captureKickDiagnostics :: Session [Diagnostic]
captureKickDiagnostics = do
_ <- skipManyTill anyMessage nonTrivialKickStart2
messages <- manyTill anyMessage nonTrivialKickDone2
Copy link
Collaborator

@soulomoon soulomoon Apr 6, 2024

Choose a reason for hiding this comment

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

Would it work if we are using the combination of nonTrivialKickDone and nonTrivialKickDone?

Copy link
Collaborator Author

@fendor fendor Apr 6, 2024

Choose a reason for hiding this comment

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

Pardon, I think you wrote the same function twice 😃

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, one of them is nonTrivialKickStart. But never mind, I think it won't work anyway.
To make sure it works we would have to customize a signal for every rule. But It should only turned on if we enable it(Provide a switch as we did for ideTesting🤔), otherwise there would be too many rules being logged.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't even need to change the method name, we could put the information about what rule it was in the method parameters.

Or we could just use $/logTrace, which we don't use at all at the moment and is maybe quite suited to this?

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.

None yet

3 participants