Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Selenium: Inspect methods logic in the "CodenvyEditor" pageobject and rework it where it necessary #9031

Merged
merged 12 commits into from
Mar 14, 2018
Merged

Selenium: Inspect methods logic in the "CodenvyEditor" pageobject and rework it where it necessary #9031

merged 12 commits into from
Mar 14, 2018

Conversation

Ohrimenko1988
Copy link
Contributor

What does this PR do?

Inspect methods logic in the "CodenvyEditor" page object and rework it where it necessary

What issues does this PR fix or reference?

Issue: #9006

Release Notes

Docs PR

@Ohrimenko1988 Ohrimenko1988 added kind/task Internal things, technical debt, and to-do tasks to be performed. team/che-qe labels Mar 6, 2018
@Ohrimenko1988
Copy link
Contributor Author

ci-test

@benoitf benoitf added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Mar 6, 2018
@codenvy-ci
Copy link

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

@Ohrimenko1988
Copy link
Contributor Author

ci-test

@codenvy-ci
Copy link

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

@Ohrimenko1988
Copy link
Contributor Author

ci-test

@codenvy-ci
Copy link

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

@Ohrimenko1988 Ohrimenko1988 changed the title [WIP] Selenium: Inspect methods logic in the "CodenvyEditor" pageobject and rework it where it necessary Selenium: Inspect methods logic in the "CodenvyEditor" pageobject and rework it where it necessary Mar 13, 2018
dmytro-ndp
dmytro-ndp reviewed Mar 13, 2018
View reviewed changes
selenium/che-selenium-test/src/main/java/org/eclipse/che/selenium/pageobject/CodenvyEditor.java Outdated
.waitVisibility(By.xpath(format(TAB_FILE_NAME_XPATH + "/parent::div", fileName)))
.getAttribute("focused")
!= null;
}
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

empty line is missed

dmytro-ndp
dmytro-ndp reviewed Mar 13, 2018
View reviewed changes
selenium/che-selenium-test/src/main/java/org/eclipse/che/selenium/pageobject/CodenvyEditor.java Outdated
}

public boolean waitAndCheckFocus(String fileName) {
Copy link
Contributor

@dmytro-ndp dmytro-ndp Mar 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It's hard to understand the method propose in context of Codenvy Editor.

dmytro-ndp
dmytro-ndp reviewed Mar 13, 2018
View reviewed changes
selenium/che-selenium-test/src/main/java/org/eclipse/che/selenium/pageobject/CodenvyEditor.java
@@ -132,6 +146,7 @@ public CodenvyEditor(
String CONTEXT_MENU = "//div[@id='menu-lock-layer-id']/div[2]";
String EDITOR_TABS_PANEL = "gwt-debug-multiSplitPanel-tabsPanel";
String ACTIVE_LINE_NUMBER = "gwt-debug-cursorPosition";
String TAB_CONTEXT_MENU_BODY = "//*[@id='gwt-debug-contextMenu/closeAllEditors']/parent::tbody";
Copy link
Contributor

@dmytro-ndp dmytro-ndp Mar 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Too complex xpath, IMHO. It's better to address the element by id, or name.

Copy link
Contributor Author

@Ohrimenko1988 Ohrimenko1988 Mar 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Nothing complex just gets element by "ID" and gets it parent tag, only two steps and short note, this is usual case for "xpath".

Copy link
Contributor

@dmytro-ndp dmytro-ndp Mar 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I meant parent::tbody part. It's not reliable way to address parent element so as there could be anything instead of required one, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case it is quite acceptable.

dmytro-ndp
dmytro-ndp reviewed Mar 13, 2018
View reviewed changes
selenium/che-selenium-test/src/main/java/org/eclipse/che/selenium/pageobject/CodenvyEditor.java Outdated
@@ -141,8 +156,7 @@ public CodenvyEditor(
String ORION_CONTENT_ACTIVE_EDITOR_XPATH = ORION_ACTIVE_EDITOR_CONTAINER_XPATH + "/div";
String ACTIVE_LINES_XPATH =
"//div[@class='textviewSelection']/preceding::div[@class='annotationLine currentLine'][1]";
String ACTIVE_LINE_HIGHLIGHT =
"//div[@class='annotationLine currentLine' and @role='presentation']";
String ACTIVE_LINE_HIGHLIGHT = "//div[@class='annotationLine currentLine']";
Copy link
Contributor

Choose a reason for hiding this comment

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

XPath could be replaced by simpler variant - class name "annotationLine currentLine".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is constant is not being used now. But it can be useful in the future. If I change the current constant to class name "annotationLine currentLine" it will be hard to understand what it means. Because it looks like "Id". In my opinion, "xpath" is better than constant name "CLASS_NAME_ACTIVE_LINE_HIGHLIGHT" , or comment.

Copy link
Contributor

@dmytro-ndp dmytro-ndp Mar 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

it will be hard to understand what it means. Because it looks like "Id"

Suffix _CLASS solves this problem.

XPath is the worst way to refer a web element so as it consists of redundant elements, syntax sugar etc, which make it much harder to read, understand and change. And it forces chromium to behave in procedural way to find an element.

Just compare:
String ACTIVE_LINE_HIGHLIGHT = "//div[@class='annotationLine currentLine']";
with
String ACTIVE_LINE_HIGHLIGHT_CLASS="annotationLine currentLine";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"ACTIVE_LINE_HIGHLIGHT_CLASS" - I am thinking about the highlighted class name or something else but not about "By.className" locator.

Copy link
Contributor

Choose a reason for hiding this comment

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

let's name it "ACTIVE_LINE_HIGHLIGHT__CLASS"

dmytro-ndp
dmytro-ndp reviewed Mar 13, 2018
View reviewed changes
selenium/che-selenium-test/src/main/java/org/eclipse/che/selenium/pageobject/CodenvyEditor.java Outdated
@@ -185,6 +199,7 @@ public CodenvyEditor(
String JAVA_DOC_POPUP = "//div[@class='gwt-PopupPanel']//iframe";
String AUTOCOMPLETE_PROPOSAL_JAVA_DOC_POPUP =
"//div//iframe[contains(@src, 'api/java/code-assist/compute/info?')]";
String HIGHLIGHT_ITEM = "//li[@selected='true']//span[text()='%s']";
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a pattern. It's better to express this moment in field name to avoid confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And what is your version? "HIGHLIGHT_ITEM_PATTERN" ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it is.

@Ohrimenko1988
Copy link
Contributor Author

ci-test

@codenvy-ci
Copy link

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

@Ohrimenko1988 Ohrimenko1988 merged commit dd6411c into master Mar 14, 2018
@Ohrimenko1988 Ohrimenko1988 deleted the selen-inspect-editor branch March 14, 2018 08:58
@Ohrimenko1988 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 Mar 14, 2018
@Ohrimenko1988 Ohrimenko1988 mentioned this pull request Mar 14, 2018
Closed
@benoitf benoitf added this to the 6.3.0 milestone Mar 16, 2018
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmytro-ndp dmytro-ndp dmytro-ndp approved these changes

@SkorikSergey SkorikSergey SkorikSergey approved these changes

@vparfonov vparfonov Awaiting requested review from vparfonov

Assignees
No one assigned
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.
Projects
None yet
Milestone
6.3.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Ohrimenko1988 @codenvy-ci @dmytro-ndp @SkorikSergey @benoitf