-
-
Notifications
You must be signed in to change notification settings - Fork 390
Speed up hls-hlint-plugin-tests #4144
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
Conversation
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") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
6fbeafd
to
ea7270b
Compare
Move test data to temporary directory. Avoid `waitForDiagnosticsWithSource` as it unconditionally waits for diagnostics.
ea7270b
to
c6d45c4
Compare
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 |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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 😃
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
Closing in favour of #4339 |
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.