Skip to content

Fix selenium tests in factory package#10921

Merged
tolusha merged 2 commits into5730_java_ls_pocfrom
fixFactory
Aug 28, 2018
Merged

Fix selenium tests in factory package#10921
tolusha merged 2 commits into5730_java_ls_pocfrom
fixFactory

Conversation

@tolusha
Copy link
Copy Markdown
Contributor

@tolusha tolusha commented Aug 27, 2018 •
edited
Loading

Signed-off-by: Anatoliy Bazko abazko@redhat.com

What does this PR do?

Fixes tests in factory package

What issues does this PR fix or reference?

#10919

Release Notes

Docs PR

@tolusha
Fix selenium tests in factory package
1cf98ee 
Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
@tolusha tolusha self-assigned this Aug 27, 2018
@tolusha tolusha requested review from svor and tsmaeder August 27, 2018 10:21
tsmaeder
tsmaeder requested changes Aug 27, 2018
View reviewed changes
...main/java/org/eclipse/che/plugin/java/languageserver/JavaLanguageServerExtensionService.java Outdated
}

private CompletableFuture<ServerCapabilities> getOrInitializeLanguageServer() {
if (serverCapabilities.get() == null) {
Copy link
Copy Markdown
Contributor

@tsmaeder tsmaeder Aug 27, 2018

Choose a reason for hiding this comment

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

It makes no sense to do an AtomicReference and synchronized protection.

...main/java/org/eclipse/che/plugin/java/languageserver/JavaLanguageServerExtensionService.java Outdated
}
}

private CompletableFuture<ServerCapabilities> getOrInitializeLanguageServer() {
Copy link
Copy Markdown
Contributor

@tsmaeder tsmaeder Aug 27, 2018

Choose a reason for hiding this comment

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

We should not do this. Whether we decide to initialize a server or not should be done by the server initializer. Don't cache the result, it's not worth it.

...in/java/org/eclipse/che/plugin/maven/server/projecttype/handler/MavenProjectInitHandler.java Outdated
public MavenProjectInitHandler(WorkspaceSynchronizer workspaceSynchronizer) {
this.workspaceSynchronizer = workspaceSynchronizer;
}
public MavenProjectInitHandler(WorkspaceSynchronizer workspaceSynchronizer) {}
Copy link
Copy Markdown
Contributor

@tsmaeder tsmaeder Aug 27, 2018

Choose a reason for hiding this comment

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

If we're not using it, remove the parameter.

...in/java/org/eclipse/che/plugin/maven/server/projecttype/handler/MavenProjectInitHandler.java

workspaceSynchronizer.syncronizerWorkspaceAsync(params);
}
public void onProjectInitialized(String projectFolder) {}
Copy link
Copy Markdown
Contributor

@tsmaeder tsmaeder Aug 27, 2018

Choose a reason for hiding this comment

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

This looks like we're not using this project initializer anymore. Can you explain how this made a selenium test fail and why we need to change it?
Generally, I think this kind of "meaningful" change would warrant a separate bug report and PR.

Copy link
Copy Markdown
Contributor Author

@tolusha tolusha Aug 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Using MavenProjectInitHandler leads us to endless cycle:

  1. Project with problems is imported
  2. JavaLanguageServerExtensionService.updateWorkspace is invoked
  3. JavaLanguageServerExtensionService.updateProjectsWithProblems is invoked
  4. MavenProjectInitHandler.onProjectInitialized is invoked.
  5. WorkspaceSynchronizer.syncronizerWorkspaceAsync is invoked
  6. -> 2

...ore-api-project/src/main/java/org/eclipse/che/api/project/server/impl/ProjectServiceApi.java
BadRequestException {
if (wsPath != null) {
projectConfigDto.setPath(absolutize(wsPath));
wsPath = absolutize(wsPath);
Copy link
Copy Markdown
Contributor

@tsmaeder tsmaeder Aug 27, 2018

Choose a reason for hiding this comment

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

Again, what was the bug here?

Copy link
Copy Markdown
Contributor Author

@tolusha tolusha Aug 27, 2018

Choose a reason for hiding this comment

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

absolutize ensure that project path starts with /.
Without that it leads it emitting ProjectCreatedEvent for already registered projects.

Have a look at code below:

    boolean registeredEarly = projectManager.isRegistered(wsPath);
    RegisteredProject updated = projectManager.update(projectConfigDto);

Without absolutize registration checking and updating will be performed for different project paths.

@tolusha
Fixup
6cdded7 
Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
svor
svor approved these changes Aug 28, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@svor svor left a comment

Choose a reason for hiding this comment

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

One point here. All existed factories which use java won't work after merging jdt.ls into master because they don't have org.eclipse.che.ls.java installer. It can be a problem.

@tolusha
Copy link
Copy Markdown
Contributor Author

tolusha commented Aug 28, 2018

@tsmaeder Could you have a look again?

@benoitf benoitf added kind/task Internal things, technical debt, and to-do tasks to be performed. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. target/branch Indicates that a PR will be merged into a branch other than master. labels Aug 28, 2018
@tolusha tolusha merged commit d81ee5d into 5730_java_ls_poc Aug 28, 2018
@tolusha tolusha deleted the fixFactory branch August 28, 2018 14:14
@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 Aug 28, 2018
tsmaeder pushed a commit that referenced this pull request Aug 29, 2018
@tolusha @tsmaeder
Fix selenium tests in factory package (#10921)
f531278 
* Fix selenium tests in factory package

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>

* Fixup

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
tolusha added a commit that referenced this pull request Sep 5, 2018
@tolusha
Fix selenium tests in factory package (#10921)
601879a 
* Fix selenium tests in factory package

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>

* Fixup

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
tsmaeder pushed a commit that referenced this pull request Sep 13, 2018
@tolusha @tsmaeder
Fix selenium tests in factory package (#10921)
3b11819 
* Fix selenium tests in factory package

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>

* Fixup

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
tsmaeder pushed a commit that referenced this pull request Sep 20, 2018
@tolusha @tsmaeder
Fix selenium tests in factory package (#10921)
c36d93c 
* Fix selenium tests in factory package

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>

* Fixup

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
tsmaeder pushed a commit that referenced this pull request Sep 26, 2018
@tolusha @tsmaeder
Fix selenium tests in factory package (#10921)
f2a36e1 
* Fix selenium tests in factory package

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>

* Fixup

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
tsmaeder pushed a commit that referenced this pull request Oct 1, 2018
@tolusha @tsmaeder
Fix selenium tests in factory package (#10921)
323bbe5 
* Fix selenium tests in factory package

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>

* Fixup

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
tsmaeder pushed a commit that referenced this pull request Oct 5, 2018
@tolusha @tsmaeder
Fix selenium tests in factory package (#10921)
9394d1e 
* Fix selenium tests in factory package

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>

* Fixup

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
tsmaeder pushed a commit that referenced this pull request Oct 12, 2018
@tolusha @tsmaeder
Fix selenium tests in factory package (#10921)
3e7fe0b 
* Fix selenium tests in factory package

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>

* Fixup

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
tsmaeder pushed a commit that referenced this pull request Oct 16, 2018
@tolusha @tsmaeder
Fix selenium tests in factory package (#10921)
130a7e1 
* Fix selenium tests in factory package

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>

* Fixup

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
tsmaeder pushed a commit that referenced this pull request Oct 17, 2018
@tolusha @tsmaeder
Fix selenium tests in factory package (#10921)
1abb727 
* Fix selenium tests in factory package

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>

* Fixup

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
tsmaeder pushed a commit that referenced this pull request Oct 17, 2018
@tolusha @tsmaeder
Fix selenium tests in factory package (#10921)
a0b1a7f 
Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
tsmaeder pushed a commit that referenced this pull request Oct 17, 2018
@tolusha @tsmaeder
Fix selenium tests in factory package (#10921)
a42d60d 
Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
tsmaeder pushed a commit that referenced this pull request Oct 17, 2018
@tolusha @tsmaeder
Fix selenium tests in factory package (#10921)
2386d57 
Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
tsmaeder pushed a commit that referenced this pull request Oct 17, 2018
@tolusha @tsmaeder
Fix selenium tests in factory package (#10921)
2fb7654 
Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@svor svor svor approved these changes

@tsmaeder tsmaeder tsmaeder approved these changes

@dkulieshov dkulieshov Awaiting requested review from dkulieshov

@dmytro-ndp dmytro-ndp Awaiting requested review from dmytro-ndp

@musienko-maxim musienko-maxim Awaiting requested review from musienko-maxim

@Ohrimenko1988 Ohrimenko1988 Awaiting requested review from Ohrimenko1988

@vparfonov vparfonov Awaiting requested review from vparfonov

Assignees

@tolusha tolusha

Labels

kind/task Internal things, technical debt, and to-do tasks to be performed. target/branch Indicates that a PR will be merged into a branch other than master.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tolusha @svor @tsmaeder @benoitf