Creating selenium framework tests (abstraction for rh-che)#10191

Merged

dmytro-ndp merged 7 commits intoeclipse-che:masterfrom

ScrewTSW:226-feature-rh-che-upstream-selenium-tests-compatibility

Jul 4, 2018

Member

ScrewTSW commented Jun 26, 2018 •

edited

Loading

What does this PR do?

This PR modifies some required classes and generalizes them for use outside of eclipse/che repo.
rh-che related PR : redhat-developer/rh-che/pull/721

What issues does this PR fix or reference?

redhat-developer/che-functional-tests/issues/266

ScrewTSW requested a review from vparfonov as a code owner

June 26, 2018 07:48

codenvy-ci commented Jun 26, 2018

Can one of the admins verify this patch?

1 similar comment

codenvy-ci commented Jun 26, 2018

Can one of the admins verify this patch?

benoitf added the status/code-review label

codenvy-ci commented Jun 26, 2018

Can one of the admins verify this patch?

ScrewTSW changed the title ~~WIP: Creating selenium framework tests (abstraction for rh-che)~~ Creating selenium framework tests (abstraction for rh-che)

ScrewTSW force-pushed the 226-feature-rh-che-upstream-selenium-tests-compatibility branch 2 times, most recently from 8d7e5c1 to ab33b6e Compare

June 26, 2018 08:01

Katka92 approved these changes

View reviewed changes

ScrewTSW force-pushed the 226-feature-rh-che-upstream-selenium-tests-compatibility branch from ab33b6e to 9c8de39 Compare

June 26, 2018 08:56

ScrewTSW mentioned this pull request

[WIP] Creating selenium framework tests (rh-che specific) redhat-developer/rh-che#721

Closed

garagatyi commented Jun 26, 2018

@musienko-maxim @skabashnyuk @tolusha FYI

garagatyi commented Jun 26, 2018

ci-build

codenvy-ci commented Jun 26, 2018

Build # 4561 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/4561/ to view the results.

dmytro-ndp reviewed

View reviewed changes

...um/che-selenium-test/src/main/java/org/eclipse/che/selenium/core/CheSeleniumSuiteModule.java Outdated

+                  bind(AbstractTestWorkspaceServiceClient.class).to(CheTestWorkspaceServiceClient.class);
+                  //
+                  // bind(TestWorkspaceServiceClientFactory.class).to(CheTestWorkspaceServiceClientFactory.class);

Contributor

dmytro-ndp Jun 26, 2018

This comment looks redundant here.

dmytro-ndp reviewed

View reviewed changes

...he-selenium-test/src/main/java/org/eclipse/che/selenium/core/CheSeleniumMultiUserModule.java Outdated

-              import org.eclipse.che.selenium.core.client.TestMachineServiceClient;
-              import org.eclipse.che.selenium.core.client.TestOrganizationServiceClient;
-              import org.eclipse.che.selenium.core.client.TestOrganizationServiceClientFactory;
+              import org.eclipse.che.selenium.core.client.*;

Contributor

dmytro-ndp Jun 26, 2018 •

edited

Loading

it's better to define dependencies explicitly

dmytro-ndp reviewed

View reviewed changes

...he-selenium-test/src/main/java/org/eclipse/che/selenium/core/CheSeleniumMultiUserModule.java Outdated

-              import org.eclipse.che.selenium.core.user.TestUser;
-              import org.eclipse.che.selenium.core.user.TestUserFactory;
-              import org.eclipse.che.selenium.core.user.TestUserImpl;
+              import org.eclipse.che.selenium.core.user.*;

Contributor

dmytro-ndp Jun 26, 2018 •

edited

Loading

it's better to define dependencies explicitly

dmytro-ndp reviewed

View reviewed changes

...enium-test/src/test/java/org/eclipse/che/selenium/dashboard/CreateAndDeleteProjectsTest.java Outdated

-              import org.eclipse.che.selenium.pageobject.NotificationsPopupPanel;
-              import org.eclipse.che.selenium.pageobject.ProjectExplorer;
-              import org.eclipse.che.selenium.pageobject.ToastLoader;
+              import org.eclipse.che.selenium.pageobject.*;

Contributor

dmytro-ndp Jun 26, 2018

it's better to define dependencies explicitly

dmytro-ndp reviewed

View reviewed changes

.../src/test/java/org/eclipse/che/selenium/dashboard/workspaces/AddOrImportProjectFormTest.java Outdated

-              import static org.eclipse.che.selenium.pageobject.dashboard.NewWorkspace.Stack.BLANK;
-              import static org.eclipse.che.selenium.pageobject.dashboard.NewWorkspace.Stack.DOT_NET;
-              import static org.eclipse.che.selenium.pageobject.dashboard.NewWorkspace.Stack.JAVA;
+              import static org.eclipse.che.selenium.pageobject.dashboard.NewWorkspace.Stack.*;

Contributor

dmytro-ndp Jun 26, 2018

it's better to define dependencies explicitly

Member Author

ScrewTSW Jun 27, 2018 •

edited

Loading

You shouldn't use static imports. instead referencing the values like Stack.JAVA.
There are of course some exceptions too, though.

dmytro-ndp reviewed

View reviewed changes

.../src/test/java/org/eclipse/che/selenium/dashboard/workspaces/AddOrImportProjectFormTest.java Outdated

-              import org.eclipse.che.selenium.pageobject.dashboard.DocumentationPage;
-              import org.eclipse.che.selenium.pageobject.dashboard.NewWorkspace;
-              import org.eclipse.che.selenium.pageobject.dashboard.ProjectOptions;
+              import org.eclipse.che.selenium.pageobject.dashboard.*;

Contributor

dmytro-ndp Jun 26, 2018

it's better to define dependencies explicitly

dmytro-ndp reviewed

View reviewed changes

...m-test/src/test/java/org/eclipse/che/selenium/dashboard/workspaces/NewWorkspacePageTest.java Outdated

-              import static org.openqa.selenium.Keys.ARROW_DOWN;
-              import static org.openqa.selenium.Keys.ARROW_UP;
-              import static org.openqa.selenium.Keys.ESCAPE;
+              import static org.openqa.selenium.Keys.*;

Contributor

dmytro-ndp Jun 26, 2018

it's better to define dependencies explicitly

dmytro-ndp reviewed

View reviewed changes

...um-test/src/test/java/org/eclipse/che/selenium/stack/CreateWorkspaceFromPythonStackTest.java Outdated

-              import org.eclipse.che.selenium.pageobject.NotificationsPopupPanel;
-              import org.eclipse.che.selenium.pageobject.ProjectExplorer;
-              import org.eclipse.che.selenium.pageobject.ToastLoader;
+              import org.eclipse.che.selenium.pageobject.*;

Contributor

dmytro-ndp Jun 26, 2018

it's better to define dependencies explicitly

dmytro-ndp reviewed

View reviewed changes

...src/main/java/org/eclipse/che/selenium/core/client/CheTestWorkspaceServiceClientFactory.java Outdated

+              import com.google.inject.assistedinject.Assisted;
+              import org.eclipse.che.selenium.core.user.TestUser;
+              public interface CheTestWorkspaceServiceClientFactory extends TestWorkspaceServiceClientFactory {

Contributor

dmytro-ndp Jun 26, 2018

It ought to be TestWorkspaceServiceClientFactory which produces TestWorkspaceServiceClient instance.

Contributor

Katka92 Jun 27, 2018

I see but how should we then differentiate to include RhCheTestWorkspaceServiceClient or CheTestWorkspaceServiceClient ?

Contributor

dmytro-ndp Jun 27, 2018 •

edited

Loading

Guice will substitute proper concrete class automatically depending on what is bound in the module class to the TestWorkspaceServiceClient class.

Contributor

dmytro-ndp Jun 27, 2018

So, we doesn't need this class CheTestWorkspaceServiceClientFactory if we have already had TestWorkspaceServiceClientFactory.

dmytro-ndp requested changes

View reviewed changes

Contributor

dmytro-ndp left a comment •

edited

Loading

@ScrewTSW: I reviewed the code again and realized that there is lack of interface class TestWorkspaceClientService similar to TestUserServiceClient. In that case class hierarchy will looks like the follow:
TestWorkspaceClientService -> AbstractTestWorkspaceClientService -> CheTestWorkspaceClientService.

In that case:

we doesn't need to change a lot of files replacing TestWorkspaceClientService on AbstractTestWorkspaceClientService;
we shouldn't use CheTestWorkspaceServiceClient in the test classes explicitly so as we are binding it in the module class;
we should bind interface TestWorkspaceServiceClient to concrete class CheTestWorkspaceServiceClient in CheSeleniumSuiteModule.

ScrewTSW force-pushed the 226-feature-rh-che-upstream-selenium-tests-compatibility branch 3 times, most recently from d485018 to 00976a3 Compare

June 27, 2018 08:00

Member Author

ScrewTSW commented Jun 27, 2018

Hello @dmytro-ndp @Katka92.
I have added new interface for TestWorkspaceServiceClient and refactored the code.
Please review.

dmytro-ndp reviewed

View reviewed changes

.../src/test/java/org/eclipse/che/selenium/dashboard/workspaces/AddOrImportProjectFormTest.java

                   // check that name field saves it state after choosing another stack
                   newWorkspace.waitPageLoad();
-                  newWorkspace.waitStackSelected(BLANK);
+                  newWorkspace.waitStackSelected(Stack.BLANK);

Contributor

dmytro-ndp Jun 27, 2018

Please, restore static imports in this class.

Member Author

ScrewTSW Jul 2, 2018

is there some specific reason the static imports are used? using static imports should be avoided.

Contributor

dmytro-ndp Jul 2, 2018 •

edited

Loading

Why it should be avoided? In this case it makes the code clearer because Stack. prefix doesn't add significant info to the code.

How about restoring initial content of file?

Member Author

ScrewTSW Jul 3, 2018

https://carlosbecker.com/posts/avoid-static-imports/
Here's a nice article explaining the issues that may arise, when developers that have never seen the code try to contribute.
Still it's just my opinion. I can restore the class.

Contributor

dmytro-ndp Jul 3, 2018

I agree that static imports can arise the problems, and they are actually not recommended to use in production. But here we have tests, and also there are situations when static imports make code much more readable, for example, in lambdas.
Anyway, these changes don't block merging into master, IMHO.

dmytro-ndp reviewed

View reviewed changes

...ore/src/main/java/org/eclipse/che/selenium/core/workspace/AbstractTestWorkspaceProvider.java Outdated

@@ @@ -37,25 +33,25 @@ @@
                * @author Anatolii Bazko
                */
               @Singleton

Contributor

dmytro-ndp Jun 27, 2018 •

edited

Loading

You shouldn't apply @Singleton annotation to abstract types. It doesn't make sense.
Use it in the extending class CheTestWorkspaceProvider so that it will work.

dmytro-ndp reviewed

View reviewed changes

...src/main/java/org/eclipse/che/selenium/core/client/CheTestWorkspaceServiceClientFactory.java Outdated

+              import com.google.inject.assistedinject.Assisted;
+              import org.eclipse.che.selenium.core.user.TestUser;
+              public interface CheTestWorkspaceServiceClientFactory extends TestWorkspaceServiceClientFactory {

Contributor

dmytro-ndp Jun 27, 2018

So, we doesn't need this class CheTestWorkspaceServiceClientFactory if we have already had TestWorkspaceServiceClientFactory.

...ium-core/src/main/java/org/eclipse/che/selenium/core/workspace/CheTestWorkspaceProvider.java Outdated

+                    @Named("workspace.default_memory_gb") int defaultMemoryGb,
+                    DefaultTestUser defaultUser,
+                    WorkspaceDtoDeserializer workspaceDtoDeserializer,
+                    CheTestWorkspaceServiceClient testWorkspaceServiceClient,

Contributor

dmytro-ndp Jun 27, 2018 •

edited

Loading

We can declare TestWorkspaceServiceClient and TestWorkspaceServiceClientFactory here in constructor.

dmytro-ndp reviewed

View reviewed changes

...um/che-selenium-test/src/main/java/org/eclipse/che/selenium/core/CheSeleniumSuiteModule.java Outdated

                   install(new FactoryModuleBuilder().build(TestUserHttpJsonRequestFactoryCreator.class));
-                  install(new FactoryModuleBuilder().build(TestWorkspaceServiceClientFactory.class));
+                  install(new FactoryModuleBuilder().build(CheTestWorkspaceServiceClientFactory.class));

Contributor

dmytro-ndp Jun 27, 2018

We shouldn't use CheTestWorkspaceServiceClientFactory here.

Contributor

Katka92 Jun 27, 2018 •

edited

Loading

Could you please shortly elaborate why? Where should we then specify to use CheTestWorkspaceServiceClientFactory?

Contributor

dmytro-ndp Jun 27, 2018

Because TestWorkspaceServiceClientFactory should work perfectly well with CheWorkspaceServiceClient itself.

Contributor

dmytro-ndp commented Jun 27, 2018

Please restore TestWorkspaceServiceClient in classes where it were replaced on CheTestWorkspaceServiceClient accidentally.

ScrewTSW force-pushed the 226-feature-rh-che-upstream-selenium-tests-compatibility branch 5 times, most recently from 8ba2ed8 to eac13d5 Compare

July 2, 2018 10:11

vzhukovs and others added 2 commits

July 3, 2018 10:38


          Prepairing upstream che for abstraction (rh-che tests)

9a25393

Signed-off-by: Tibor Dancs <tdancs@redhat.com>

Modified TestWorkspaceProvider -> Abstraction, so that rh-che can override needed methods.

Modified TestWorkspaceServiceClient for rh-che tests extension

Abstracted TestWorkspaceServiceClient for RhChe implementation

Fixed inject issues

Signed-off-by: Tibor Dancs <tdancs@redhat.com>

Modified required abstractions, using interfaces instead of implementations of the interface (e.g. DefaultTestUser)

Modified TestWorkspaceProvider -> Abstraction, so that rh-che can override needed methods.

Fixed inject issues

Signed-off-by: Tibor Dancs <tdancs@redhat.com>


          Fixing abstraction, added interface for TestWorkspaceServiceClient

b1a72f9

ScrewTSW force-pushed the 226-feature-rh-che-upstream-selenium-tests-compatibility branch from eac13d5 to b1a72f9 Compare

July 3, 2018 08:39


          Reverting test class imports to interfaces

fec2322

Member Author

ScrewTSW commented Jul 3, 2018 •

edited

Loading

@dmytro-ndp Becasue of the added interface, the Guice now panics, complaining that the factory needs a concrete class implementation for install(new FactoryBuilder().build(<insert factory interface class here>))


          Fix installing of TestWorkspaceServiceClientFactory

3f15265

Signed-off-by: Dmytro Nochevnov <dnochevnov@codenvy.com>

Contributor

dmytro-ndp commented Jul 3, 2018 •

edited

Loading

@ScrewTSW: please, look at my fresh fixup - it provides implementation of TestWorkspaceServiceClient inside the TestWorkspaceServiceClientFactory explicitely.

Contributor

dmytro-ndp commented Jul 3, 2018

ci-test

dmytro-ndp reviewed

View reviewed changes

...he-selenium-test/src/main/java/org/eclipse/che/selenium/core/CheSeleniumMultiUserModule.java Outdated

                 protected void configure() {
                   bind(TestAuthServiceClient.class).to(KeycloakTestAuthServiceClient.class);
                   bind(TestMachineServiceClient.class).to(CheTestMachineServiceClient.class);
+                  bind(TestWorkspaceServiceClient.class).to(CheTestWorkspaceServiceClient.class);

Contributor

dmytro-ndp Jul 3, 2018

We can omit binding TestWorkspaceServiceClient here so as we have already done it in CheSeleniumSuiteModule.java.


          removed redundant bind

eee6101

codenvy-ci commented Jul 3, 2018

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

dmytro-ndp approved these changes

View reviewed changes

Contributor

dmytro-ndp commented Jul 3, 2018

ci-build

Contributor

dmytro-ndp commented Jul 3, 2018

ci-test-docker-single

codenvy-ci commented Jul 4, 2018

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

Dmytro Nochevnov added 2 commits

July 4, 2018 10:54


          Get rid of grouping of static imports in CreateWorkspaceFromBlankStac…

b0e6e9d

…kTest

Signed-off-by: Dmytro Nochevnov <dnochevnov@codenvy.com>


          Merge remote-tracking branch 'upstream/master' into 226-feature-rh-ch…

6a2827e

…e-upstream-selenium-tests-compatibility

dmytro-ndp merged commit 09671a1 into eclipse-che:master

benoitf removed the status/code-review label

benoitf added this to the 6.8.0 milestone

codenvy-ci commented Jul 5, 2018

Build # 4567 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/4567/ to view the results.

hbhargav pushed a commit to hbhargav/che that referenced this pull request


          Creating selenium framework tests (abstraction for rh-che) (eclipse-c…

d36bc2f

…he#10191)

* Prepairing upstream che for abstraction (rh-che tests)

Signed-off-by: Tibor Dancs <tdancs@redhat.com>

Modified TestWorkspaceProvider -> Abstraction, so that rh-che can override needed methods.

Modified TestWorkspaceServiceClient for rh-che tests extension

Abstracted TestWorkspaceServiceClient for RhChe implementation

Fixed inject issues

Signed-off-by: Tibor Dancs <tdancs@redhat.com>

Modified required abstractions, using interfaces instead of implementations of the interface (e.g. DefaultTestUser)

Modified TestWorkspaceProvider -> Abstraction, so that rh-che can override needed methods.

Fixed inject issues

Signed-off-by: Tibor Dancs <tdancs@redhat.com>

* Fixing abstraction, added interface for TestWorkspaceServiceClient

* Reverting test class imports to interfaces

* Fix installing of TestWorkspaceServiceClientFactory

Signed-off-by: Dmytro Nochevnov <dnochevnov@codenvy.com>

* removed redundant bind

* Get rid of grouping of static imports in CreateWorkspaceFromBlankStackTest

Signed-off-by: Dmytro Nochevnov <dnochevnov@codenvy.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet