[TS-SELENIUM] Cover the "Start a Workspace from a devfile" step from "Happy path" scenario #13479

Ohrimenko1988 · 2019-06-05T11:16:07Z

What does this PR do?

Cover the "Start a Workspace from a devfile" step from "Happy path" scenario

What issues does this PR fix or reference?

Issue: #13356

Release Notes

Docs PR

Katka92 · 2019-06-05T11:29:27Z

e2e/pageobjects/ide/Editor.ts

@@ -13,44 +13,74 @@ import { DriverHelper } from '../../utils/DriverHelper';
 import { CLASSES } from '../../inversify.types';
 import { TestConstants } from '../../TestConstants';
 import { By, Key } from 'selenium-webdriver';
+import { error } from 'selenium-webdriver';


This can be appended to the upper row as import { By, Key, error } from 'selenium-webdriver';

Katka92 · 2019-06-05T11:33:39Z

e2e/pageobjects/ide/Editor.ts

        await this.driverHelper.waitVisibility(By.css(Editor.SUGGESTION_WIDGET_BODY_CSS), timeout);
    }

-    async waitSuggestionContainerClosed(attempts: number = TestConstants.TS_SELENIUM_DEFAULT_ATTEMPTS, polling: number = TestConstants.TS_SELENIUM_DEFAULT_POLLING) {
-        await this.driverHelper.waitDisappearance(By.css(Editor.SUGGESTION_WIDGET_BODY_CSS), attempts, polling);
+    public async waitSuggestionContainerClosed(attempts: number = TestConstants.TS_SELENIUM_DEFAULT_ATTEMPTS,


I suggest to remove parameters ' attempts ' and ' polling ' as they are not used.

Katka92 · 2019-06-05T11:37:31Z

e2e/pageobjects/ide/Editor.ts

+
+                await this.pressEscapeButton(editorTabTitle);
+                await this.waitSuggestionContainerClosed();
+                await this.pressControlSpaceCombination(editorTabTitle);


NOTE 1: If I understand correctly reason of this is - if the suggestion is not shown, test tries to view it by pressing ctrl + space. Shouldn't there be another check if the suggestion is shown correctly for that second attempt?

NOTE 2: Why does pressing ctrl + space have own method? Why not to call performKeyCombination directly from here?

Answer 1: Not exactly. Described logic encapsulated in the "wait" method, this is a kind of loop, it will repeat until called method returns some value, or timeout end.

await this.waitSuggestionContainer(); try { await this.driverHelper.waitVisibility(suggestionLocator, 5000); return true; } catch (err) { const isTimeoutError: boolean = err instanceof error.TimeoutError; if (!isTimeoutError) { throw err; } await this.pressEscapeButton(editorTabTitle); await this.waitSuggestionContainerClosed(); await this.pressControlSpaceCombination(editorTabTitle); }

Answer 2: It looks much better for me to use particular method for invoking suggestion container than repeat each time this.performKeyCombination(editorTabTitle, Key.chord(Key.CONTROL, Key.SPACE))

Answer 2: You still need to write a line of code - even performKeyCombination or pressControlSpaceCombination. It is quite weird to have methods that contain only one line this way. And it doesn't seem to be frequent method - only 2 usages. The escapeButton is called only once.

I must say, that I'm not really fond of this approach either. Maybe if these "typical" key combinations would live in separate classes, then it would be ok. But having such methods in Editor class is not very nice IMO.
But I don't want to block this PR because of this.

Katka92 · 2019-06-05T11:42:51Z

e2e/pageobjects/ide/Editor.ts

+                    throw err;
+                }
+
+                await this.pressEscapeButton(editorTabTitle);


Why not to call performKeyCombination directly from here?

Answer 2 #13479 (comment)

Ohrimenko1988 · 2019-06-05T12:28:39Z

ci-build

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

Ohrimenko1988 · 2019-06-05T13:32:57Z

ci-build

rhopp

I've put some comments to this PR.
We've also had meeting to discuss more possible changes, but those will be done in separate PR.

e2e/README.md

rhopp · 2019-06-05T11:54:51Z

e2e/driver/CheReporter.ts

-          });
-        }
-      });
+      const isReportDirExist: boolean = fs.existsSync(reportDirPath);


reportDirExists would be better name IMO.

I am guided by "Java" code conduct which pointed that "is" prefix should be used for boolean variables and methods. Is it the right convention for "Typescript"?

I'm not aware of such convention... Even in java it's usual to have different prefixes like has<Property>, should<beAbleToDoSomething>, ... Basically anything meaningfull is better, than something not making sense (in this case two verbs in one "sentence"(var name))
Anyway... looking at this code I don't see this variable used anywhere else, so maybe put the fs.existsSync() directly into the condition?

On my opinion this is

const reportDirExists: boolean = fs.existsSync(reportDirPath); if (!reportDirExists) { fs.mkdirSync(reportDirPath); }

more clear then

if (!fs.existsSync(reportDirPath)) { fs.mkdirSync(reportDirPath); }

Let's agree to disagree on the approach :-D What about the name?

Already changed =)

rhopp · 2019-06-05T11:55:16Z

e2e/driver/CheReporter.ts

+      }
+
+      // create dir for failed test report if not exist
+      const isTestReportDirExist: boolean = fs.existsSync(testReportDirPath);


testReportDirExists would be better name IMO.

I can see this pattern in more places in this PR, so I won't comment it any further, but the objection still holds ;-)

#13479 (comment)

rhopp · 2019-06-05T12:09:44Z

e2e/pageobjects/ide/Editor.ts

    }

-    async waitSuggestion(suggestionText: string, timeout: number = TestConstants.TS_SELENIUM_DEFAULT_TIMEOUT) {
-        await this.driverHelper.waitVisibility(By.xpath(this.getSuggestionLineXpathLocator(suggestionText)), timeout);
+    public async waitSuggestion(editorTabTitle: string,


If I understand this correctly, all the methods here are now requiring editor tab title (a.k.a. name of the file opened in this editor?).
Wouldn't it be better to pass that in the constructor? This way we could have multiple instances of editor - the same way, as user can have multiple editors opened.
Doing that we could go even further - having method editor.setFocus() to set focus to this editor. And maybe even further? Calling the setFocus() every time, when we try to operate with this editor (for example by editor.type()) to make sure the correct editor is in focus?

Added to issue: #13479 (comment)

rhopp · 2019-06-06T10:41:08Z

e2e/pageobjects/ide/Ide.ts


-            return elementText.search(expectedText) > 0;
+            if (isTextPresent) {


Wouldn't simple return isTextPresent be nicer? ;-)

Not exactly.
Method "wait" waits until some value is returned,
if we return "false" method will pass,
so that's why we need this "if" check and return
value just in case of successful check

For a {@link webdriver.Condition} or function, the wait will repeatedly evaluate the condition until it returns a truthy value.

From this it seems to me, that returning false should cause the wait loop to be executed again...

You can check it yourself, it works as I described

Hehe... Unfortunately I dont have time to try now, so I believe you :-D

rhopp · 2019-06-06T10:41:35Z

e2e/pageobjects/ide/Ide.ts


-        }, timeout);
+            if (isTextAbsent) {


same as with isTextPresent

#13479 (comment)

rhopp · 2019-06-06T10:44:52Z

e2e/pageobjects/ide/Editor.ts

+        }, timeout);
+    }
+
+    async waitSuccessBuildText(editorTabTitle: string,


Such specific method shouldn't be here... Instead we should have something like waitForText(editor, text, timeout, polling)

rhopp · 2019-06-06T10:46:33Z

e2e/pageobjects/ide/PreviewWidget.ts

+        await this.driverHelper.waitDisappearance(By.css('div.theia-mini-browser'));
+    }
+
+    async waitSpringAvailable(timeout: number = TestConstants.TS_SELENIUM_DEFAULT_TIMEOUT,


Too specific method for page object... Instead of this we should take By(?) locator, which we should look for inside the preview page object?

rhopp · 2019-06-06T10:48:45Z

e2e/pageobjects/ide/PreviewWidget.ts

+                return true;
+            }
+
+            await this.driverHelper.getDriver().switchTo().defaultContent();


This section seems weird to me... Is it some workaround for some problem?

part of the preview widget placed in the iframe

Ohrimenko1988 · 2019-06-06T12:32:08Z

I have created an issue with described required improvements
#13486

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

rhopp · 2019-06-07T11:21:50Z

e2e/pageobjects/ide/Editor.ts

+        }, timeout);
+    }
+
+    async waitForText(editorTabTitle: string,


Hmm... now, the method is called "generally", but it's still waiting for some specific, hard-coded text.
My original suggestion was to rewrite the method to take string parameter (text to wait for).

Now I can also see we already have such method (waitText), but that one has slightly different logic (it's not scrolling down, as this one).
Maybe this one shoud be renamed&rewritten to something like

followAndWaitForText(editorTabTitle: string, textToSearchFor: string, timeout: number, polling:number)

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

Ohrimenko1988 added kind/task Internal things, technical debt, and to-do tasks to be performed. team/che-qe labels Jun 5, 2019

Ohrimenko1988 requested review from rhopp and musienko-maxim June 5, 2019 11:16

Ohrimenko1988 requested a review from vparfonov as a code owner June 5, 2019 11:16

Katka92 reviewed Jun 5, 2019

View reviewed changes

Ohrimenko1988 removed the request for review from musienko-maxim June 5, 2019 12:27

Katka92 approved these changes Jun 5, 2019

View reviewed changes

Ohrimenko1988 requested a review from musienko-maxim June 5, 2019 12:39

musienko-maxim approved these changes Jun 5, 2019

View reviewed changes

Cover the 'Start a Workspace from a devfile' step from 'Happy path' s…

60b1e89

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

Ohrimenko1988 force-pushed the selen-happy-path branch from c1a143f to 60b1e89 Compare June 5, 2019 13:22

rhopp reviewed Jun 6, 2019

View reviewed changes

Ohrimenko1988 added 2 commits June 6, 2019 16:35

Do changes according to review

e8e91f6

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

Do changes according to review

eb34689

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

rhopp reviewed Jun 7, 2019

View reviewed changes

Do changes according to review

c4a99b4

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

rhopp approved these changes Jun 7, 2019

View reviewed changes

Ohrimenko1988 merged commit 8af9570 into master Jun 7, 2019

Ohrimenko1988 deleted the selen-happy-path branch June 7, 2019 15:49

che-bot added this to the 7.0.0 milestone Jun 27, 2019


		return elementText.search(expectedText) > 0;
		if (isTextPresent) {

[TS-SELENIUM] Cover the "Start a Workspace from a devfile" step from "Happy path" scenario #13479

[TS-SELENIUM] Cover the "Start a Workspace from a devfile" step from "Happy path" scenario #13479

Conversation

Ohrimenko1988 commented Jun 5, 2019

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ohrimenko1988 Jun 5, 2019 • edited Loading

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

Ohrimenko1988 commented Jun 5, 2019

Ohrimenko1988 commented Jun 5, 2019

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

Ohrimenko1988 Jun 6, 2019 • edited Loading

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

Ohrimenko1988 commented Jun 6, 2019

Choose a reason for hiding this comment

Ohrimenko1988 Jun 5, 2019 •

edited

Loading

Ohrimenko1988 Jun 6, 2019 •

edited

Loading