[TS-SELENIUM] Cover the "Use Java IDE features and the inner loop" step from "Happy path" scenario #13615

Ohrimenko1988 · 2019-06-21T16:06:20Z

What does this PR do?

Cover the "Use Java IDE features and the inner loop" step from "Happy path" scenario

What issues does this PR fix or reference?

Issue: #13519

Release Notes

Docs PR

Ohrimenko1988 · 2019-07-01T13:36:39Z

ci-build

Ohrimenko1988 · 2019-07-02T06:44:18Z

ci-build

… path' scenario Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

Ohrimenko1988 · 2019-07-02T07:02:20Z

ci-build

Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

Katka92 · 2019-07-02T13:25:18Z

e2e/pageobjects/ide/Editor.ts

    async closeTab(tabTitle: string, timeout: number = TestConstants.TS_SELENIUM_DEFAULT_TIMEOUT) {
-        const tabCloseButtonLocator: By = By.xpath(`//div[text()='${tabTitle}']/parent::li//div[contains(@class, 'p-TabBar-tabCloseIcon')]`);
+        const tabCloseButtonLocator: By = this.getTabCloseIconLocator(tabTitle);


Why is the logic moved to method which is used only once?

Katka92 · 2019-07-02T13:25:50Z

e2e/pageobjects/ide/Editor.ts

+        }, timeout);
+    }
+
+    public async waitHighlightedSuggestion(editorTabTitle: string,


This method is not used.

e2e/pageobjects/ide/Editor.ts

Katka92 · 2019-07-03T09:16:37Z

e2e/pageobjects/ide/Editor.ts

@@ -140,9 +187,13 @@ export class Editor {
        timeout: number = TestConstants.TS_SELENIUM_DEFAULT_TIMEOUT,
        polling: number = TestConstants.TS_SELENIUM_DEFAULT_POLLING) {

+        await this.ide.closeAllNotifications();


I would move this.ide.closeAllNotifications() to the method selectTab and use this method here instead of current code. So from currently:

await this.ide.closeAllNotifications(); await this.clickOnTab(editorTabTitle); await this.waitTabFocused(editorTabTitle);

I would call await this.selectTab(editorTabTitle);

The selectTab method has been already implemented. Just put it here.

Katka92 · 2019-07-03T09:30:42Z

e2e/pageobjects/ide/Editor.ts

+                }
+
+                if (err instanceof error.WebDriverError) {
+                    await this.driverHelper.wait(polling);


Why should we wait when WebDriverError occures?

Katka92 · 2019-07-03T09:49:27Z

e2e/pageobjects/ide/Terminal.ts

+export class Terminal {
+    constructor(@inject(CLASSES.DriverHelper) private readonly driverHelper: DriverHelper) { }
+
+    async waitTab(tabTitle: string, timeout: number = TestConstants.TS_SELENIUM_DEFAULT_TIMEOUT) {


Some of the methods like waitTab or closeTab seems to be same as those in Editor. Maybe it would be better to move them to some another class to avoid code duplication.

Editor & terminal are two different "parts of IDE"... they may have these methods in common, but I don't think it would be wise to abstract this (make a common ancestor for these components), as these components are not very related....

Katka92 · 2019-07-03T10:05:22Z

e2e/tests/e2e_happy_path/HappyPath.spec.ts

+    });
+});
+
+suite.skip('Language server validation', async () => {


Is the skip there intended?

Katka92 · 2019-07-03T10:06:46Z

e2e/tests/e2e_happy_path/HappyPath.spec.ts

+        await driverHelper.wait(5000);
+
+        // await topMenu.selectOption('Terminal', 'Run Task...');
+        // await quickOpenContainer.clickOnContainerItem('che: run');


Why are those comments left there?

Katka92 · 2019-07-03T10:18:20Z

e2e/utils/DriverHelper.ts

+        const attempts: number = TestConstants.TS_SELENIUM_DEFAULT_ATTEMPTS;
+        const polling: number = TestConstants.TS_SELENIUM_DEFAULT_POLLING;
+
+        for (let i = 0; i < attempts; i++) {


Can you please elaborate why await with timeout is not suitable and you have to use for loop here?

Because timeout assumes that wait will be failed when the timeout end. And try may be not repeated if waiting for the element is too long. In this case, I want to ensure that try will be repeated.

So the possible solution could be reduce the timeout for waiting for the elements inside the "waiting function" and increasing timeout for the wait, right? But yeah... As you mentioned under my comment here.. this could be done in other PR.

rhopp

I've put some comments in here... Most of them are not blocking the PR, but I would like to have some reaction to them before merging the PR.

Also what I'm missing, when reading and - more importantly - running the tests is proper logging. I would like to know what is happening... The most obvious examples are the waiting functions... I would like to see in the logs what is currently being executed, how long are we waiting for different stuff etc. But this is something we would need to tackle in some other PR.

rhopp · 2019-07-03T09:55:12Z

deploy/kubernetes/helm/che/templates/configmap.yaml

@@ -112,3 +112,6 @@ data:
  JAEGER_SAMPLER_PARAM: "1"
  JAEGER_REPORTER_MAX_QUEUE_SIZE: "10000"
  CHE_METRICS_ENABLED: {{ .Values.global.metricsEnabled | quote }}
+  CHE_WORKSPACE_JAVA__OPTIONS: "-Xmx2000m"
+  CHE_WORKSPACE_MAVEN__OPTIONS: "-Xmx20000m"
+  CHE_INFRA_KUBERNETES_WORKSPACE__START__TIMEOUT__MIN: "15"


Have we tried that without this timeout? (Just with default one?) I would prefer having default one and put this extended one just in case the default proves to be not sufficient.

Without this variables each launch fails with "heap space" error

I may have not been completely clear here... I was talking just about CHE_INFRA_KUBERNETES_WORKSPACE__START__TIMEOUT__MIN. This one is not affecting command execution (build inside workspace) in any way.

rhopp · 2019-07-03T09:55:39Z

e2e/driver/CheReporter.ts


+      // take browser console logs and write to file


Nice addition. +1 ;-)

rhopp · 2019-07-03T09:56:53Z

e2e/pageobjects/ide/DebugView.ts

+        const configurationItemLocator: By = By.xpath(`//select[@class='debug-configuration']//option[text()=\'${itemText}\']`);
+
+        // for ensure that drop-down list has been opened
+        await this.driverHelper.wait(2000);


Couldn't these "hard" waits be replaced by some conditional waits? I think having "dumb" sleeps in test code is not a good practice.

In this case, it is not critical because it works properly even without this sleep. This is my overcautiousness for CI launches

I strongly believe any sleep like this should be either removed, or replaced by conditional wait.
These three waits in this method are perfect example of this... You even put into comments what should we wait for.
This is the strongest reason preventing me from giving approval to this PR. If it works OK without them (even on CI), we could remove them. If it doesn't work right without them, we should replace them with conditional waits.

rhopp · 2019-07-03T09:57:43Z

e2e/pageobjects/ide/Editor.ts

+                return true;
+            } catch (err) {
+                const isTimeoutError: boolean = err instanceof error.TimeoutError;
+                if (!isTimeoutError) {


hehe.. just a nitpicking:

if (! (err instanceof error.TimeoutError)) { throw err; }

This version seems readable enough for me without needing to create new variable. This is not blocking this PR at all, I'm just expressing my opinion ;-)

Yup, I used this variable for debug logging, logging was removed and the variable is left

rhopp · 2019-07-03T10:12:13Z

e2e/pageobjects/ide/Editor.ts

+                return;
+            } catch (err) {
+                // ignore errors and wait
+                await this.driverHelper.wait(polling);


Do we know what errors can happen here? Shouldn't we at least log them?

In this case, we should ignore all exceptions

Yeah.. I can see that from the code, that we are ignoring all the errors... My question was "Why?"
I'm not saying this "silent swallowing" of errors have to be wrong. But usually it's a bad code smell and I think it needs more explanation.
Furthermore, there is quite a big chunk (relatively) of code in the try-catch block and I can imagine quite lot of possible errors could happen - having them logged could help in possible debugging.

Because actions inside of try/catch are very sensitive, and I tried to process most common exceptions and final was that the "WebDriverError" may be happening and it does not make sense to process other exceptions because it is a parent of all driver exceptions. Anyway, if something went wrong exception with full explanation will be thrown in the further code.

rhopp · 2019-07-03T10:35:06Z

e2e/tests/e2e_happy_path/HappyPath.spec.ts

-        await editor.waitTabFocused(javaFileName);
+        await editor.selectTab(javaFileName);
+
+        await ide.checkLsInitializationStart('Starting Java Language Server');
        await ide.waitStatusBarTextAbcence('Starting Java Language Server', 360000);


I know this is out of scope of this PR, but for some next time - Abcence doesn't exist - it's Absence

e2e/tests/e2e_happy_path/HappyPath.spec.ts

rhopp · 2019-07-03T10:39:38Z

e2e/tests/e2e_happy_path/HappyPath.spec.ts

+    }
+}
+
+// async function runTask(taskTitle: string, terminalTabTitle: string) {


Some forgotten commented code. Pls remove this from the final PR.

rhopp · 2019-07-03T10:44:19Z

e2e/utils/DriverHelper.ts

+        const attempts: number = TestConstants.TS_SELENIUM_DEFAULT_ATTEMPTS;
+        const polling: number = TestConstants.TS_SELENIUM_DEFAULT_POLLING;
+
+        for (let i = 0; i < attempts; i++) {


I know you have already lot of methods like this, where you implemented your polling mechanism, but is this really needed? Couldn't we just use https://github.com/SeleniumHQ/selenium/blob/master/javascript/node/selenium-webdriver/lib/webdriver.js#L448? (and as allways... I'm pretty new to this tech, so there may be reasons for this to be not possible... If yes, please tell me the reasons - I want to get wiser :-D)

Similar question and ansver #13615 (comment) .
And a small addition. Most of the mentioned methods really maybe reworked by waits. I have already done something similar https://github.com/eclipse/che/blob/master/e2e/utils/DriverHelper.ts#L150-L170
It may be done in the separate PR

e2e/pageobjects/ide/Terminal.ts

Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

rhopp

I've put some more comments here... I'm also requesting changes, as I feel strongly about "hard sleeps" ( like driverHelper.wait(2000)).

rhopp · 2019-07-04T09:35:35Z

e2e/files/happy-path/happy-path-workspace.yaml

+components:
+  - type: kubernetes
+    alias: petclinic-web
+    reference: https://gist.githubusercontent.com/l0rd/52eda1d1b57878a218cba17c0c93a477/raw/9a2d56e777d22e77c25c45744caea35bb62d8688/petclinic.yaml


It's possible to have URL to raw file also by branch.. that may be better solution to this.

https://raw.githubusercontent.com/eclipse/che/master/e2e/files/happy-path/containers-happy-path.yaml

Thinking about it more... I guess this will come in followup PR, right? (To not break current state)

Already changed locally to the branch URL

e2e/pageobjects/ide/Editor.ts

rhopp · 2019-07-04T09:49:42Z

e2e/pageobjects/ide/Editor.ts

+                return;
+            } catch (err) {
+                // ignore errors and wait
+                await this.driverHelper.wait(polling);


Yeah.. I can see that from the code, that we are ignoring all the errors... My question was "Why?"
I'm not saying this "silent swallowing" of errors have to be wrong. But usually it's a bad code smell and I think it needs more explanation.
Furthermore, there is quite a big chunk (relatively) of code in the try-catch block and I can imagine quite lot of possible errors could happen - having them logged could help in possible debugging.

rhopp · 2019-07-04T09:51:19Z

e2e/pageobjects/ide/Editor.ts

+        elementStyleValue = elementStyleValue.replace('position: absolute; top: ', '');
+        elementStyleValue = elementStyleValue.replace('px; width: 100%; height: 19px;', '');
+
+        const lineYCoordinate: number = Number.parseInt(elementStyleValue, 10);


@Ohrimenko1988 Would be nice to respond even if you don't agree with the proposed solution ;-) I, for example, think the regexp solution is a bit more robust... If you think it's not needed to have more robust solution, please respond ;-)

rhopp · 2019-07-04T09:53:51Z

e2e/pageobjects/ide/Ide.ts

-        await this.driverHelper.getDriver().wait(async () => {
-            const elementText: string = await this.driverHelper.waitAndGetText(statusBarLocator, timeout);
+        // for ensuring that check is not invoked in the gap of status displaying
+        for (let i: number = 0; i < 3; i++) {


@Ohrimenko1988 Explanation? I genuinely don't understand this piece of code and it doesn't make much sense to me...

rhopp · 2019-07-04T10:25:41Z

deploy/kubernetes/helm/che/templates/configmap.yaml

@@ -112,3 +112,6 @@ data:
  JAEGER_SAMPLER_PARAM: "1"
  JAEGER_REPORTER_MAX_QUEUE_SIZE: "10000"
  CHE_METRICS_ENABLED: {{ .Values.global.metricsEnabled | quote }}
+  CHE_WORKSPACE_JAVA__OPTIONS: "-Xmx2000m"
+  CHE_WORKSPACE_MAVEN__OPTIONS: "-Xmx20000m"
+  CHE_INFRA_KUBERNETES_WORKSPACE__START__TIMEOUT__MIN: "15"


I may have not been completely clear here... I was talking just about CHE_INFRA_KUBERNETES_WORKSPACE__START__TIMEOUT__MIN. This one is not affecting command execution (build inside workspace) in any way.

rhopp · 2019-07-04T10:30:34Z

e2e/pageobjects/ide/DebugView.ts

+        const configurationItemLocator: By = By.xpath(`//select[@class='debug-configuration']//option[text()=\'${itemText}\']`);
+
+        // for ensure that drop-down list has been opened
+        await this.driverHelper.wait(2000);


I strongly believe any sleep like this should be either removed, or replaced by conditional wait.
These three waits in this method are perfect example of this... You even put into comments what should we wait for.
This is the strongest reason preventing me from giving approval to this PR. If it works OK without them (even on CI), we could remove them. If it doesn't work right without them, we should replace them with conditional waits.

rhopp · 2019-07-04T10:32:59Z

e2e/pageobjects/ide/Terminal.ts

+export class Terminal {
+    constructor(@inject(CLASSES.DriverHelper) private readonly driverHelper: DriverHelper) { }
+
+    async waitTab(tabTitle: string, timeout: number = TestConstants.TS_SELENIUM_DEFAULT_TIMEOUT) {


Editor & terminal are two different "parts of IDE"... they may have these methods in common, but I don't think it would be wise to abstract this (make a common ancestor for these components), as these components are not very related....

e2e/pageobjects/ide/Terminal.ts

rhopp · 2019-07-04T10:34:34Z

e2e/pageobjects/ide/Terminal.ts

+        await this.selectTerminalTab(tabTitle, timeout);
+
+        // for ensure that element is interactable
+        await this.driverHelper.wait(3000);


I guess this is still something I strongly feel about... Until I'm proven we really need this and it couldn't be done any other way I would request this to change.

Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

…ppy-4

Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

rhopp

There are still some questions/nitpicks unanswered, but all of them are of minor character.

Ohrimenko1988 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. team/che-qe labels Jun 21, 2019

Ohrimenko1988 requested review from rhopp and musienko-maxim June 21, 2019 16:06

musienko-maxim approved these changes Jul 1, 2019

View reviewed changes

Ohrimenko1988 requested review from l0rd and sleshchenko as code owners July 1, 2019 14:24

l0rd approved these changes Jul 1, 2019

View reviewed changes

Ohrimenko1988 force-pushed the selen-happy-4 branch from 22c8bc6 to bdcb8c7 Compare July 2, 2019 06:47

Cover the 'Use Java IDE features and the inner loop' step from 'Happy…

9575e3c

… path' scenario Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

Ohrimenko1988 force-pushed the selen-happy-4 branch from bdcb8c7 to 9575e3c Compare July 2, 2019 06:56

Ohrimenko1988 added 9 commits July 2, 2019 10:28

Changes for debug

59ad850

Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

Changes for debug

ffa1310

Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

Add ability to store browser logs after test fail

cca7303

Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

Changes for debug

5408847

Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

Changes for debug

749bec7

Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

Changes for debug

7757982

Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

Changes for debug

0fe673f

Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

Changes for debug

ffe530e

Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

Changes for debug

4e966b5

Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

l0rd mentioned this pull request Jul 3, 2019

Che 7.0.0 Endgame Plan #13637

Closed

85 tasks

Workaround for the '#13681' bug

fb7a683

Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

Katka92 reviewed Jul 3, 2019

View reviewed changes

rhopp reviewed Jul 3, 2019

View reviewed changes

Ohrimenko1988 added 2 commits July 3, 2019 14:29

Cleanup test

9d95830

Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

Do changes according to review

dc35ae9

Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

Add yaml files which defined test workspace

628c64b

Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

rhopp requested changes Jul 4, 2019

View reviewed changes

Ohrimenko1988 added 5 commits July 4, 2019 14:47

Cleanup tests

796215e

Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

Merge branch 'master' of https://github.com/eclipse/che into selen-ha…

fafb5b9

…ppy-4

Remove hardcoded waits

ad94922

Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

Do changes according to review

d44d716

Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

remove redundant dependencies

31def9b

Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>

rhopp approved these changes Jul 4, 2019

View reviewed changes

Ohrimenko1988 merged commit b045651 into master Jul 5, 2019

Ohrimenko1988 deleted the selen-happy-4 branch July 5, 2019 08:35

Ohrimenko1988 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 5, 2019

Ohrimenko1988 mentioned this pull request Jul 5, 2019

[TS-SELENIUM] Cover the "Use Java IDE features and the inner loop" step from "Happy path" scenario #13519

Closed

4 tasks

[TS-SELENIUM] Cover the "Use Java IDE features and the inner loop" step from "Happy path" scenario #13615

[TS-SELENIUM] Cover the "Use Java IDE features and the inner loop" step from "Happy path" scenario #13615

Conversation

Ohrimenko1988 commented Jun 21, 2019 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

Ohrimenko1988 commented Jul 1, 2019

Ohrimenko1988 commented Jul 2, 2019

Ohrimenko1988 commented Jul 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhopp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhopp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhopp left a comment

Choose a reason for hiding this comment

Ohrimenko1988 commented Jun 21, 2019 •

edited

Loading