-
Notifications
You must be signed in to change notification settings - Fork 20
Conversation
Conflicts: pom.xml
Conflicts: pom.xml src/main/java/ru/sbtqa/tag/pagefactory/Page.java
LFing generic step defs
Conflicts: src/main/java/ru/sbtqa/tag/pagefactory/DriverExtensions.java src/main/java/ru/sbtqa/tag/pagefactory/Page.java src/main/java/ru/sbtqa/tag/pagefactory/PageFactory.java src/main/java/ru/sbtqa/tag/pagefactory/PageShell.java src/main/java/ru/sbtqa/tag/pagefactory/aspects/ClickAspect.java src/main/java/ru/sbtqa/tag/pagefactory/stepdefs/GenericStepDefs.java src/main/java/ru/sbtqa/tag/pagefactory/stepdefs/SetupDefsBase.java src/main/java/ru/sbtqa/tag/pagefactory/support/ScreenShooter.java
Conflicts: pom.xml src/main/java/ru/sbtqa/tag/pagefactory/Page.java src/main/java/ru/sbtqa/tag/pagefactory/PageFactory.java src/main/java/ru/sbtqa/tag/pagefactory/PageWrapper.java
Conflicts: src/main/java/ru/sbtqa/tag/pagefactory/DriverExtensions.java src/main/java/ru/sbtqa/tag/pagefactory/Page.java src/main/java/ru/sbtqa/tag/pagefactory/PageFactory.java src/main/java/ru/sbtqa/tag/pagefactory/stepdefs/GenericStepDefs.java src/main/java/ru/sbtqa/tag/pagefactory/stepdefs/SetupDefsBase.java
|
||
public static Environment getEnvironment() { | ||
switch (ENVIRONMENT) { | ||
case "web": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В константы: WEB_ENVIRONMENT и MOBILE_ENVIRONMENT
WebElement targetWebElement; | ||
Class<? extends Page> elementRedirect; | ||
TypifiedElement typifiedElement; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not declare it. It used in one place only.
} | ||
|
||
setAspectsDisabled(true); | ||
LOG.info("Aspect disabled"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to log this, since it always disabled for mobile. Or move it into DEBUG level. Useless information for user
private static WebDriver webDriver; | ||
private static final int ATTEMPTS_TO_START_WEBDRIVER = Integer.parseInt(Props.get("driver.create.attempts", "3")); | ||
private static BrowserMobProxy proxy; | ||
private static final String WEBDRIVER_PATH = "src/test/resources/webdrivers/"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This path must support configuration from props. Or it should possible to not set it. By default selenium looks it from $PATH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the legacy. Will not be fixed in this issue.
} catch (UnreachableBrowserException e) { | ||
log.warn("Failed to create web driver. Attempt number {}", i, e); | ||
if (null != webDriver) { | ||
// Don't dispose when driver is already null, cus it causes new driver creation at Init.getWebDriver() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cuz
capabilities.setCapability(CapabilityType.PROXY, seleniumProxy); | ||
} | ||
switch (PageFactory.getBrowserName()) { | ||
case "Firefox": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add support for marionette and EDGE driver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will not be fixed in this issue.
log.warn("Failed to quit web driver", e); | ||
} finally { | ||
try { | ||
//TODO take out into a separate method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Complete TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the legacy. Will not be fixed in this issue.
*/ | ||
public static void waitUntilPagePrepared(WebElement webElement) { | ||
try { | ||
new WebDriverWait(PageFactory.getDriver(), PageFactory.getTimeOutInSeconds() / 2). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you divide by 2 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the legacy. Will not be fixed in this issue.
* | ||
* @param sec a int. | ||
*/ | ||
private static void sleep(int sec) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated code, whu not generalize it for public use in utils?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the legacy. Will not be fixed in this issue.
|
||
private static final Logger LOG = LoggerFactory.getLogger(SetupDefsBase.class); | ||
private static final Logger log = LoggerFactory.getLogger(SetupStepDefs.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOG!
Need to add application.properties-sample! |
private static void createDriver() throws UnsupportedBrowserException { | ||
DesiredCapabilities capabilities = new DesiredCapabilitiesParser().parse(); | ||
|
||
if (Props.get("webdriver.remote.host").isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two swich-cases with almost duplicate code. I'd merge them into one switch, and set driver in each case if Props.get("webdriver.remote.host").isEmpty() && !Props.get("proxy.enable").isEmpty()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, this is legacy, I see
webDriver.get(PageFactory.getInitialUrl()); | ||
} | ||
|
||
public static void dispose() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too many try-catches, and all of them are basically doing the same: log the error message and proceed. Need to simplify this into one big try-catch block, that either catches all the shit in the end, and log all exceptions via e.getMessage()
, or just make a try-catch-catch-catch block with different messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a legacy too. Please, create issues on all of your comments to the legacy code.
@@ -1,12 +1,12 @@ | |||
package ru.sbtqa.tag.pagefactory.exceptions; | |||
|
|||
public class GetValueException extends Exception { | |||
public class DirectionException extends Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saw it in the methods signature, but couldn't find any place where it is really being thrown. Is it thrown anywhere? I'd leave only SwipeException, it seems enough for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fnd please avoid generic exceprion where it possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bug I agree. I'll remove this exception.
@@ -0,0 +1,30 @@ | |||
package ru.sbtqa.tag.pagefactory.exceptions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exception also looks excess for me. It is being thrown only in one place (WebExtension:32):
if (elementId == null) {
throw new ElementValueException("Getting value is not support in element without id");
}
IllegalArgumentException
fits well enough here, why not use it?
starty = endy = size.height / 2; | ||
break; | ||
default: | ||
throw new FactoryRuntimeException("Failed to swipe to direction " + direction); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not SwipeException
?
No description provided.