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

Apache Camel Language LSP client compatible with Java files #10264

Merged

Conversation

apupier
Copy link
Contributor

@apupier apupier commented Jul 4, 2018

Signed-off-by: Aurélien Pupier apupier@redhat.com

What does this PR do?

Triggering the Apache Camel Language Server on Java files

What issues does this PR fix or reference?

#10262

Release Notes

Docs PR

@apupier apupier requested a review from vparfonov as a code owner July 4, 2018 12:39
@benoitf benoitf added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Jul 4, 2018
@codenvy-ci
Copy link

Can one of the admins verify this patch?

@apupier
Copy link
Contributor Author

apupier commented Jul 4, 2018

to test it, Apache Camel Language Server "Installer"needs to be activated:
image

and this file with a .java extension can be used :

import org.apache.camel.builder.RouteBuilder;

/**
 * A Camel Java DSL Router
 */
public class MyRouteBuilder extends RouteBuilder {

    /**
     * Let's configure the Camel routing rules using Java code...
     */
    public void configure() {

        // here is a sample which processes the input files
        // (leaving them in place - see the 'noop' flag)
        // then performs content based routing on the message using XPath
        from("file:src/data?noop=true&antExclude=aa&antFilterCaseSensitive=true&runLoggingLevel=OFF")
            .choice()
                .when(xpath("/person/city = 'London'"))
                    .to("file:target/messages/uk")
                .otherwise()
                    .to("file:target/messages/others");
    }
}

if you call the completion just after the quote from(" L.16 you should have several results including ahc-httpUri

@codenvy-ci
Copy link

Can one of the admins verify this patch?

1 similar comment
@codenvy-ci
Copy link

Can one of the admins verify this patch?

@benoitf
Copy link
Contributor

benoitf commented Jul 4, 2018

ci-test

@benoitf benoitf requested a review from tsmaeder July 4, 2018 13:10
@codenvy-ci
Copy link

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10264
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@apupier
Copy link
Contributor Author

apupier commented Jul 4, 2018

there are some test failures but they seem not related to my PR (as far I understand them)
They have always a success rate which is not 100%, does it mean that they are all flaky tests?

@tolusha tolusha requested a review from dkulieshov July 5, 2018 07:22
@tolusha
Copy link
Contributor

tolusha commented Jul 5, 2018

@musienko-maxim
Could you have a look at tests result?

@dkulieshov
Copy link

@apupier

Hi, recently there were introduced several changes to regular expressions for language servers, they were called upon to give regexes more flexibility, and as of what I see probably by a mistake those changes were not applied to Camel language regexes, so I would recommend to replace

private static final String REGEX = ".*\\.(xml|java)";

with

private static final String REGEX = ".*\\.(xml|java)$";

@dkulieshov dkulieshov requested a review from musienko-maxim July 5, 2018 09:40
@apupier apupier force-pushed the 10262-HaveCamelLSPWorkingWithJava branch from b39ba79 to 4d491c0 Compare July 5, 2018 09:51
@apupier
Copy link
Contributor Author

apupier commented Jul 5, 2018

@dkuleshov updated PR with your advice

@skabashnyuk
Copy link
Contributor

ci-test

@apupier
Copy link
Contributor Author

apupier commented Jul 5, 2018

with my PR it seems that the Java Language Server is not started at all:
image

i have no Java completion, neither Java Syntax coloring. Does it mean that 2 Language Server on the same file is not supported? Or that I misconfigured something?

@codenvy-ci
Copy link

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10264
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@apupier
Copy link
Contributor Author

apupier commented Jul 6, 2018

I tend to say that several LSP on same file should work given this code:
https://github.com/eclipse/che/blob/89d6c1664aee6a25b117b2a63216e1804742d0da/wsagent/che-core-api-languageserver/src/main/java/org/eclipse/che/api/languageserver/TextDocumentService.java#L246-L247

I'm not able to have a working development environment so can't really investigate more unfortunately

@apupier
Copy link
Contributor Author

apupier commented Jul 6, 2018

@tsmaeder explained me that this approach won't work until jdt.ls is used in Che #6157 .

@musienko-maxim
Copy link
Contributor

musienko-maxim commented Jul 6, 2018

After last selenium launching, was found that java files from a maven project have wrong syntax highlighting. It may be depend on incorrect detecting of editor type.
Steps to reproduce:

  • Create a workspace based on javastack.
  • Create for instance the spring project from wizard and open the GreetingController file
  • More details on the screenshot:

selection_027

@vparfonov
Copy link
Contributor

do not merge before @musienko-maxim approve it

@apupier
Copy link
Contributor Author

apupier commented Jul 6, 2018

this PR cannot be merged until jdt.ls has been integrated

@apupier apupier force-pushed the 10262-HaveCamelLSPWorkingWithJava branch from 20694ff to 790270a Compare October 25, 2018 09:50
@apupier
Copy link
Contributor Author

apupier commented Oct 25, 2018

ci-test

@riuvshin
Copy link
Contributor

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:10264
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@apupier
Copy link
Contributor Author

apupier commented Oct 25, 2018

the ci build error i smentioning missing theia stuff but this PR is not touching it.

@apupier
Copy link
Contributor Author

apupier commented Oct 25, 2018

ci-test

@riuvshin
Copy link
Contributor

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:10264
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@dkulieshov
Copy link

@apupier Seems like at the moment for some reason CI tests fail during build, so not to waste your time I would recommend you to wait until the issue is resolved and then relaunch the tests. @musienko-maxim probably will tell you more.

@apupier
Copy link
Contributor Author

apupier commented Oct 25, 2018

ok, thanks for letting me know. Please ping me when it is fixed and if I need to rebase on master to get the fix too.

…che#10262)

- remove language declaration for Camel, Camel is not a language type
but just a provider on top of other languages

Signed-off-by: Aurélien Pupier <apupier@redhat.com>
@apupier apupier force-pushed the 10262-HaveCamelLSPWorkingWithJava branch from 790270a to ebba742 Compare October 26, 2018 14:39
@apupier
Copy link
Contributor Author

apupier commented Oct 26, 2018

ci-test

1 similar comment
@apupier
Copy link
Contributor Author

apupier commented Oct 29, 2018

ci-test

@riuvshin
Copy link
Contributor

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:10264
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@apupier
Copy link
Contributor Author

apupier commented Oct 29, 2018

some tests failures seems due to this regression: #11760

for instance here is the screenshot of one failing test:
image

@apupier
Copy link
Contributor Author

apupier commented Oct 30, 2018

relaunching because it is working manually and the modifications should not have an impact to these failed tests

@apupier
Copy link
Contributor Author

apupier commented Oct 30, 2018

ci-test

@riuvshin
Copy link
Contributor

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:10264
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@apupier
Copy link
Contributor Author

apupier commented Oct 31, 2018

without any changes, another set of tests has failed.
i'm not able to launch tests locally.
Testing the feature manually works well.

Can someone review this PR?

@dkulieshov
Copy link

@musienko-maxim can you please check test results?

@musienko-maxim
Copy link
Contributor

musienko-maxim commented Nov 5, 2018

So we had many failed tests - main reason problems on test-infrastructure. After rerunning all problem tests passed properly. But we have just unexpected regression with ApacheCamelFileEditingTest. The logic of tests works as well. But we need some clarification. @SkorikSergey is going to ask you about that

@SkorikSergey
Copy link
Contributor

@apupier ApacheCamelFileEditingTest selenium test failed due to missing expected "Initialized language server 'org.eclipse.che.plugin.camel.server.languageserver'' message in "dev-machine" console after opening "camel.xml" file. And there is an exception in "dev-machine" console.

org eclipse che selenium languageserver apachecamelfileeditingtest checklanguageserverinitialized_time-1540890573621-millis

@apupier
Copy link
Contributor Author

apupier commented Nov 5, 2018

stacktrace is similar to the one in #11760
looks alike the JavaLanguageServerExtensionService is not robust enough when there are projects containing a single file

@SkorikSergey
Copy link
Contributor

Selenium tests execution on Eclipse Che Multiuser on OCP (https://ci.codenvycorp.com/job/che-pullrequests-test-ocp/1111//Selenium_20tests_20report/) doesn't show any regression against this Pull Request.

@apupier
Copy link
Contributor Author

apupier commented Nov 7, 2018

Do I need to change something or to provide more information to have this PR merged?

@dmytro-ndp
Copy link
Contributor

Looks like you have enough approvals to merge the PR into the master.

@apupier
Copy link
Contributor Author

apupier commented Nov 7, 2018

Just to be sure that it is clear, I don't have rights to merge myself.

@dmytro-ndp dmytro-ndp merged commit 8089d3d into eclipse-che:master Nov 7, 2018
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Nov 7, 2018
@benoitf benoitf added this to the 6.14.0 milestone Nov 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/question Questions that haven't been identified as being feature requests or bugs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.