search

eclipse / lsp4e

Language Server Protocol support in Eclipse IDE
Eclipse Public License 2.0
57 stars 54 forks source link

Semantic Tokens: Do not use default theme #1029

Closed ahmedneilhussain closed 3 weeks ago

ahmedneilhussain commented 3 weeks ago

Fix for https://github.com/eclipse/lsp4e/issues/1028 - we requested the default theme if no TM mapping for the editor, but this calls syncExec. Reconciler threads mustn't block on the UI thread as it is possible for the UI thread to be blocked waiting for reconciliation; this deadlock was indeed observed.

ahmedneilhussain commented 3 weeks ago

Hi @rubenporras without the fallback a test failed. I added a custom content type with a real TM grammar mapping and tweaked the expected tokens to match. Is that OK? Felt a bit heavyweight, but I couldn't see an easy way of mocking/injecting a TM mapping in the test setup. I thought it best to create a new content type mapping so that this change was segregated from the other tests.

rubenporras commented 3 weeks ago

hi @ahmedneilhussain ,

some tests are still failing. Are these some flaky tests or does the removal of the fallback break those tests as well?

Regards

ahmedneilhussain commented 3 weeks ago

hi @ahmedneilhussain ,

some tests are still failing. Are these some flaky tests or does the removal of the fallback break those tests as well?

Regards

HI according to this https://ci.eclipse.org/lsp4e/job/lsp4e-github/job/PR-1029/2/testReport/ all the tests are passing. Am I missing something? Some of the CI runs are complaining because of the eclipse version bump check - I'm not sure if I'm supposed to do something about that or whether it will get bumped as part of prep for next release.

There are definitely some flaky tests but I think the fallback should not interfere with the majority of tests: the mock language server won't have anything in place for semantic tokens other than where we are explicitly testing that functionality, and we don't configure tm4e other than where I just introduced it (for a new content type used only for this one case). I could be wrong, but the test run above and the fact they ran clean (but flaky!) locally suggests not...

Cheers, Ahmed

rubenporras commented 3 weeks ago

Sorry, I have not noticed that. You need to manually bump the version of org.eclipse.lsp4e.test, if I recall right in the manifest.mf and in the pom.xml.