Add K8S infrastructure for selenium tests by musienko-maxim · Pull Request #10384 · eclipse-che/che

musienko-maxim · 2018-07-11T14:24:19Z

What does this PR do?

Add property for managing K8S infrastructure
Improve managing of configurations for different environments (docker, openshift, k8s)
Add K8S TestGroup for dependent tests

What issues does this PR fix or reference?

dmytro-ndp · 2018-07-11T14:41:32Z

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

-    } else {
-      throw new RuntimeException(
-          format("Infrastructure '%s' hasn't been supported by tests.", cheInfrastructure));
+    final Infrastructure cheInfrastructure =


Please, restore configureInfrastructureRelatedDependencies() method and update its content according to your PR.

dmytro-ndp · 2018-07-11T14:42:12Z

selenium/che-selenium-test/src/test/resources/templates/factory/k8s/minimal.json

+      }
+    }
+  }
+}


empty line missed at the end of file

Ohrimenko1988 · 2018-07-11T14:44:53Z

...elenium-core/src/main/java/org/eclipse/che/selenium/core/factory/TestFactoryInitializer.java

  }

+  private String getTemplateDirectory(String template) {
+    String templateDirectoryName;


Can be simplified:

private String getTemplateDirectory(String template) { String templateDirectoryName = infrastructure.toString().toLowerCase(); if (infrastructure == k8s){ templateDirectoryName = k8s.toString().toLowerCase(); } return format("/templates/factory/%s/%s", templateDirectoryName, template);

Yes, it can, but case-statement is more preferable so as it's more obvious compare to if-statement, and it is more flexible

Ohrimenko1988 · 2018-07-11T14:48:19Z

...elenium-core/src/main/java/org/eclipse/che/selenium/core/utils/WorkspaceDtoDeserializer.java

+    switch (infrastructure) {
+      case openshift:
+      case k8s:
+        templateDirectoryName = Infrastructure.openshift.toString().toLowerCase();


Is it OK that case is "k8s" but infrastructure which using is "openshift"?

Also may be optimized like in the previous remark

Is it OK that case "k8s" but infrastructure which using is "openshift"?

Yes - it was requirement to use openshift templates for k8s.

Also may be optimized like in the previous remark

Please take a look at the answer https://github.com/eclipse/che/pull/10384/files/a178ec69b067878bf86a824ead181e7232512185#r201723043

skabashnyuk · 2018-07-12T06:38:19Z

...m/che-selenium-core/src/main/java/org/eclipse/che/selenium/core/constant/Infrastructure.java

+
+@Singleton
+public enum Infrastructure {
+  docker,


upper case please

You are right that it is the constant and have to be in upper case.
But we are using Infrastructure enum to simplify injection from environment variable CHE_INFRASTRUCTURE like the follow:

@Inject @Named("che.infrastructure") private Infrastructure infrastructure;

Such injection is case sensitive, and CHE_INFRASTRUCTURE uses lower-case values like docker, openshift etc.

I've asked about values of the enum, not about the name of enum itself.

We can transform the value of CHE_INFRASTRUCTURE variable to upper case in time of configuring CheSeleniumSuiteModule to make it possible to inject upper case values: Infrastructure.DOCKER, Infrastructure.OPENSHIFT etc.

Not sure what do you mean. But lowercase enum values looks weird.

skabashnyuk · 2018-07-12T06:38:23Z

...m/che-selenium-core/src/main/java/org/eclipse/che/selenium/core/constant/Infrastructure.java

+package org.eclipse.che.selenium.core.constant;
+
+import com.google.inject.Singleton;
+


dmytro-ndp · 2018-07-12T12:16:26Z

...m/che-selenium-core/src/main/java/org/eclipse/che/selenium/core/constant/Infrastructure.java

+  docker,
+  openshift,
+  k8s,
+  supershift


supershift is redundant here

I like an idea of supporting supershift in Che 😄

dmytro-ndp · 2018-07-12T12:18:35Z

...elenium-core/src/main/java/org/eclipse/che/selenium/core/factory/TestFactoryInitializer.java

+    switch (infrastructure) {
+      case openshift:
+      case k8s:
+        templateDirectoryName = Infrastructure.k8s.toString().toLowerCase();


Did you mean Infrastructure.openshift.toString().toLowerCase() here?

dmytro-ndp · 2018-07-12T12:19:17Z

...elenium-core/src/main/java/org/eclipse/che/selenium/core/factory/TestFactoryInitializer.java

+
+      case docker:
+      default:
+        templateDirectoryName = infrastructure.toString().toLowerCase();


Did you mean Infrastructure.docker.toString().toLowerCase() here?

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

dmytro-ndp · 2018-07-12T15:40:30Z

ci-test

riuvshin · 2018-07-12T22:52:32Z

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

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

musienko-maxim added 8 commits June 27, 2018 11:12

fix logic error for checking marker type

5a49391

Merge branch 'master' into quick_fix_for_Csharp

255b974

implement constants and methods for handling infrastructure

423ef77

Merge branch 'master' into CHE-9768

d984183

inject k8s infrastructure info into selenium tests

db96bc6

fix the test

3c85ee5

merge with master

c69ff0a

fix error after merging

a178ec6

musienko-maxim requested a review from vparfonov as a code owner July 11, 2018 14:24

musienko-maxim requested review from Ohrimenko1988 and dmytro-ndp July 11, 2018 14:24

benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/task Internal things, technical debt, and to-do tasks to be performed. labels Jul 11, 2018

dmytro-ndp reviewed Jul 11, 2018

View reviewed changes

Ohrimenko1988 reviewed Jul 11, 2018

View reviewed changes

Add empty line

06d3e57

Ohrimenko1988 reviewed Jul 11, 2018

View reviewed changes

Ohrimenko1988 approved these changes Jul 11, 2018

View reviewed changes

skabashnyuk reviewed Jul 12, 2018

View reviewed changes

dmytro-ndp reviewed Jul 12, 2018

View reviewed changes

Fix Infrastructure enum values case

d74b7c5

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

Fix java doc of Infrastructure class

f89b554

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

dmytro-ndp approved these changes Jul 13, 2018

View reviewed changes

dmytro-ndp merged commit fc91a43 into master Jul 13, 2018

benoitf added this to the 6.9.0 milestone Jul 13, 2018

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 Jul 13, 2018

SkorikSergey mentioned this pull request Jul 17, 2018

Unexpected skipping of selenium tests from OSIO test group #10448

Closed

dmytro-ndp deleted the CHE-9768 branch August 1, 2018 16:20

+                    }
+                  }
+                }
+              }

                
                    No newline at end of file

		package org.eclipse.che.selenium.core.constant;

		import com.google.inject.Singleton;

Conversation

musienko-maxim commented Jul 11, 2018

What does this PR do?

What issues does this PR fix or reference?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmytro-ndp Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ohrimenko1988 Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmytro-ndp Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skabashnyuk Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmytro-ndp commented Jul 12, 2018

Uh oh!

riuvshin commented Jul 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dmytro-ndp Jul 11, 2018 •

edited

Loading

Ohrimenko1988 Jul 11, 2018 •

edited

Loading

dmytro-ndp Jul 11, 2018 •

edited

Loading

skabashnyuk Jul 12, 2018 •

edited

Loading