-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
fix: various failures and issues #185
Conversation
Fix Analyze Groovy files in folder which was always available resulting in failures dependent on the context it was used. It is now only available from a explorer folder. Also correct the name of the parameter passed so explorer folder requests work. Fixes nvuillam#177
Fix fixes in files which still have remaining issues not applying due to the change in behaviour of npm-groovy-lint status code handling in 11.0.0 by now requesting failon: none.
Add more debugging so that its easier to understand what is happening. Default debugging setting to that the DEBUG environment if set so that the developer doesn't have to set it manually in settings when running a debug session. Also enable npm-groovy-lint debugging which is key to understanding issues. While this was previous set for debugging sessions in the environment the setting would always override it.
Fix all the CI linting errors and update modules to address vulnerabilities.
Update all client and server dependencies to eliminate security issues.
58d38d6
to
9134fb7
Compare
It smells masterclass here ❤️ |
@stevenh it's like you deep dived in my mind in the past, understood everything and made it more elegant, that's amazing, can't wait to see this PR merged and build a new golden statue :) |
I see the test cases... they seem to have long execution time, maybe CodeNarc Server is not called and npm-groovy-lint fallbacks to new java command line ? :/ |
There's definitely some slowness, focusing in on stability for now. |
Looks like there is still a race deep in the code, it's not clear if its vscode extension, npm-groovy-lint or both, but still more work to do. |
Main race is fixed by nvuillam/npm-groovy-lint#343 |
e38f355
to
901544e
Compare
Refactor tests to be more reliable, making them independent of each other, so if one test fails others are not effected. This includes self managing timeouts as mocha doesn't handle pending promises, which makes it harder to debug issues. Run and Debug commands: * Fix path to workspace files of "Language Server E2E Test" Run and Debug command. * Rename to make it clear what each config is used for. Add mochaExplorer configuration, to improve test visibility. Re-enable big groovy test file, now it works as expected. Rename test files to eliminate stuttering. Fix test debugging by using NPM_DEBUG instead of DEBUG env var which is filtered out by vscode, see: microsoft/vscode#197494. Remove duplicate lint call that's now handled by onDidChangeContent. Remove fixed delay comment so we don't have to update when the delay changes. Update Node and JVM versions in README.md. Update all packages, to address security issues and bring in the latest version of npm-groovy-lint which includes a critical race condition fix. Clean up imports. Use onDidChangeContent to trigger re-linting to improve performance and remove skipNextOnDidChangeContents which conflicts with this change. Eliminate else statements where previous block returns, to improve code readability. Refactor resetDiagnostics into deleteDiagnostics as its now only used for the delete flow, so we can remove unnecessary logic and diagnostic changes. Remove commented out code. Convert high noise debugging to trace. Add missing await to docManager.updateDocumentSettings call Use latest xvfb-action@v1 instead of v1.0 to bring in the latest fixes.
901544e
to
a7b63d6
Compare
@nvuillam apologies for another huge set of changes, but wanted to get to a place where the tests were solid, which has taken quite a refactor. I think we're finally there now, which should give us a solid base to move forward. Please pay particular attention to the logic changes in the server component when reviewing, as there could be some reason for the original flow which I didn't understand. |
@stevenh the original flow was to cancel the current linting request when there is a new one incoming for the same file ^^ ( lint while wiring code) Is such feature still handled ? :) |
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.
Amazing work :)
I cloned your branch locally and made a few successful tests :)
Time for a new golden statue :)
Thanks for that context, yes that case should still be handled 😀 |
@stevenh by the way... sorry, I used Canva.com again 🤣 |
Refactor tests to be more reliable, making them independent of each other, so if one test fails others are not effected. This includes self managing timeouts as mocha doesn't handle pending promises, which makes it harder to debug issues.
Run and Debug commands:
Add mochaExplorer configuration, to improve test visibility.
Re-enable big groovy test file, now it works as expected.
Rename test files to eliminate stuttering.
Fix test debugging by using NPM_DEBUG instead of DEBUG env var which is filtered out by vscode, see: microsoft/vscode#197494.
Remove duplicate lint call that's now handled by onDidChangeContent.
Remove fixed delay comment so we don't have to update when the delay changes.
Update Node and JVM versions in README.md.
Update all packages, to address security issues and bring in the latest version of npm-groovy-lint which includes a critical race condition fix.
Clean up imports.
Use onDidChangeContent to trigger re-linting to improve performance and remove skipNextOnDidChangeContents which
conflicts with this change.
Eliminate else statements where previous block returns, to improve code readability.
Refactor resetDiagnostics into deleteDiagnostics as its nowonly used for the delete flow, so we can remove unnecessary
logic and diagnostic changes.
Remove commented out code.
Convert high noise debugging to trace.
Add missing await to docManager.updateDocumentSettings call
Use latest xvfb-action@v1 instead of v1.0 to bring in the latest fixes.
Fixes #177 #174 #181 #172
Readiness Checklist
Author/Contributor
Reviewing Maintainer
breaking
if this is a large fundamental changeautomation
,bug
,documentation
,enhancement
,infrastructure
, orperformance