-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
PR to review code with mentor #1
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<project xmlns="http://maven.apache.org/POM/4.0.0" | ||
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> | ||
<modelVersion>4.0.0</modelVersion> | ||
|
||
<groupId>com.epam.ftm.selenium.webdriver</groupId> | ||
<artifactId>epam-ftm-selenium-webdriver</artifactId> | ||
<version>1.0-SNAPSHOT</version> | ||
|
||
<properties> | ||
<maven.compiler.source>8</maven.compiler.source> | ||
<maven.compiler.target>8</maven.compiler.target> | ||
</properties> | ||
|
||
<dependencies> | ||
<!-- https://mvnrepository.com/artifact/org.seleniumhq.selenium/selenium-java --> | ||
<dependency> | ||
<groupId>org.seleniumhq.selenium</groupId> | ||
<artifactId>selenium-java</artifactId> | ||
<version>3.141.59</version> | ||
</dependency> | ||
<!-- https://mvnrepository.com/artifact/org.testng/testng --> | ||
<dependency> | ||
<groupId>org.testng</groupId> | ||
<artifactId>testng</artifactId> | ||
<version>7.4.0</version> | ||
<scope>test</scope> | ||
</dependency> | ||
</dependencies> | ||
|
||
</project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
package BringItOn.page; | ||
|
||
import org.openqa.selenium.WebDriver; | ||
import org.openqa.selenium.WebElement; | ||
import org.openqa.selenium.support.PageFactory; | ||
import org.openqa.selenium.support.ui.ExpectedConditions; | ||
import org.openqa.selenium.support.ui.WebDriverWait; | ||
|
||
public abstract class AbstractPage { | ||
protected WebDriver driver; | ||
|
||
protected final int WAIT_TIMEOUT_SECONDS = 10; | ||
|
||
protected AbstractPage(WebDriver driver) { | ||
this.driver = driver; | ||
PageFactory.initElements(driver, this); | ||
} | ||
|
||
protected WebElement waitForWebElementVisible(WebElement element) { | ||
return new WebDriverWait(driver, WAIT_TIMEOUT_SECONDS).until(ExpectedConditions.visibilityOf(element)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
package BringItOn.page; | ||
|
||
import org.openqa.selenium.WebDriver; | ||
import org.openqa.selenium.WebElement; | ||
import org.openqa.selenium.support.FindBy; | ||
import org.openqa.selenium.support.ui.WebDriverWait; | ||
|
||
public class PastebinMainPage extends AbstractPage { | ||
private final String BASE_URL = "https://pastebin.com"; | ||
|
||
@FindBy(id = "postform-text") | ||
WebElement textAreaNewPaste; | ||
|
||
@FindBy(id = "select2-postform-format-container") | ||
WebElement syntaxDropdown; | ||
@FindBy(xpath = "//li[text()='Bash']") | ||
WebElement syntaxBash; | ||
|
||
@FindBy(id = "select2-postform-expiration-container") | ||
WebElement expirationDropdown; | ||
@FindBy(xpath = "//li[text()='10 Minutes']") | ||
WebElement expiration10M; | ||
|
||
@FindBy(id = "postform-name") | ||
WebElement inputTitle; | ||
|
||
@FindBy(xpath = "//button[@type='submit']") | ||
WebElement buttonNewPaste; | ||
|
||
public PastebinMainPage(WebDriver driver) { | ||
super(driver); | ||
} | ||
|
||
public PastebinMainPage openPage() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know that this kind of implementation was presented on the lecture, but in real life projects very often void is used for methods which returns nothing, but not the page There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good to know! Because in the lecture it was said that it is better to avoid using void everywhere, so I went that call) |
||
driver.get(BASE_URL); | ||
return this; | ||
} | ||
|
||
public PastebinMainPage typeInNewPasteText(String newPasteText) { | ||
waitForWebElementVisible(textAreaNewPaste).sendKeys(newPasteText); | ||
return this; | ||
} | ||
|
||
public PastebinMainPage openSyntaxDropdown() { | ||
syntaxDropdown.click(); | ||
return this; | ||
} | ||
|
||
public PastebinMainPage selectSyntaxBash() { | ||
syntaxBash.click(); | ||
return this; | ||
} | ||
|
||
public PastebinMainPage openExpirationDropdown() { | ||
expirationDropdown.click(); | ||
return this; | ||
} | ||
|
||
public PastebinMainPage selectExpiration10M() { | ||
expiration10M.click(); | ||
return this; | ||
} | ||
|
||
public PastebinMainPage typeInNewPasteTitle(String newPasteTitle) { | ||
inputTitle.sendKeys(newPasteTitle); | ||
return this; | ||
} | ||
|
||
public PastebinMainPage submitNewPaste() { | ||
buttonNewPaste.click(); | ||
return this; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
package BringItOn.page; | ||
|
||
import org.openqa.selenium.WebDriver; | ||
import org.openqa.selenium.WebElement; | ||
import org.openqa.selenium.support.FindBy; | ||
|
||
public class PastebinSubmittedPastePage extends AbstractPage { | ||
@FindBy(xpath = "//div[@class='left']/a[text()='Bash']") | ||
WebElement syntaxSubmittedPaste; | ||
|
||
@FindBy(xpath = "//textarea") | ||
WebElement textSubmittedPaste; | ||
|
||
public PastebinSubmittedPastePage(WebDriver driver) { | ||
super(driver); | ||
} | ||
|
||
public String getSyntaxSubmittedPaste() { | ||
return syntaxSubmittedPaste.getText(); | ||
} | ||
|
||
public String getSubmittedPasteText() { | ||
return textSubmittedPaste.getText(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
package BringItOn.test; | ||
|
||
import org.openqa.selenium.By; | ||
import org.openqa.selenium.WebDriver; | ||
import org.openqa.selenium.WebElement; | ||
import org.openqa.selenium.chrome.ChromeDriver; | ||
import org.openqa.selenium.support.ui.ExpectedConditions; | ||
import org.openqa.selenium.support.ui.WebDriverWait; | ||
import org.testng.annotations.*; | ||
import BringItOn.page.PastebinMainPage; | ||
import BringItOn.page.PastebinSubmittedPastePage; | ||
|
||
public abstract class AbstractTest { | ||
protected WebDriver driver; | ||
|
||
protected static final String NEW_PASTE_TEXT = "git config --global user.name \"New Sheriff in Town\"\n" + | ||
"git reset $(git commit-tree HEAD^{tree} -m \"Legacy code\")\n" + | ||
"git push origin master --force"; | ||
protected static final String NEW_PASTE_TITLE = "how to gain dominance among developers"; | ||
protected static final String SUBMITTED_PASTE_TITLE_LOCATOR = "//div[@class='info-top']/h1"; | ||
protected static final String EXPECTED_PASTE_SYNTAX = "Bash"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. better to move all the String variables to Test class There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I did exactly this for Cloud Calculator task. Initially, when I was doing Pastebin task, I thought that it would be good to keep the Test class only for the tests themselves, but later realized that the Abstract class isn't intended to hold those variables. Left it like that to emphasize differences between implementations, since I wasn't sure which approach is better. |
||
PastebinMainPage pastebinMainPage; | ||
PastebinSubmittedPastePage pastebinSubmittedPastePage; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like access modifier protected should be used here. Moreover if to rework the work with the Pages here in this class then this initialization will be redundant There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It used to be protected initially - here and in many other parts of the code. But since I don't know much about it yet, I decided to follow IDE suggestions on code changes and edited it everywhere it was suggested to avoid a single warning from IDE. I definitely need to go deeper into it to understand purposes and approaches of using different modifiers in Java. |
||
|
||
@BeforeTest(alwaysRun = true) | ||
public void browserSetup() { | ||
driver = new ChromeDriver(); | ||
driver.manage().window().maximize(); | ||
pastebinMainPage = new PastebinMainPage(driver) | ||
.openPage() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if void is used for the method the implementation here would look like pastebinMainPage.openPage(); and so on for all the methods There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it! I thought that using a chain of methods calls like that would make the code more clean and readable, but apparently using void methods won't make it any less readable. |
||
.typeInNewPasteText(NEW_PASTE_TEXT) | ||
.openSyntaxDropdown() | ||
.selectSyntaxBash() | ||
.openExpirationDropdown() | ||
.selectExpiration10M() | ||
.typeInNewPasteTitle(NEW_PASTE_TITLE) | ||
.submitNewPaste(); | ||
pastebinSubmittedPastePage = new PastebinSubmittedPastePage(driver); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this BeforeTest method is overloaded with steps. it's called browserSetup() and it should be so. Maybe some interactions with opening the page should be added here as well. all the test logic with steps should be moved to the test class. By the way, why do you return the new page at the very end? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this was again my bad when I decided to have the Test class as minimal as possible. For the Cloud Calculator class I went another way and keep the AbstractTest class minimal instead. |
||
} | ||
|
||
public WebElement waitForElement(By by) { | ||
return new WebDriverWait(driver, 10).until(ExpectedConditions.presenceOfElementLocated(by)); | ||
} | ||
|
||
@AfterTest(alwaysRun = true) | ||
public void browserExit() { | ||
driver.quit(); | ||
driver = null; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
package BringItOn.test; | ||
|
||
import org.openqa.selenium.By; | ||
import org.openqa.selenium.WebElement; | ||
import org.testng.Assert; | ||
import org.testng.annotations.Test; | ||
|
||
public class PastebinTest extends AbstractTest { | ||
@Test(description = "Check if submitted paste title matches initial paste title") | ||
public void checkSubmittedPastePageTitle() { | ||
WebElement titleSubmittedPaste = waitForElement(By.xpath(SUBMITTED_PASTE_TITLE_LOCATOR)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. using locators in tests is not a good practice, it should be moved from here |
||
String actualPageTitle = titleSubmittedPaste.getText(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually as it was mentioned in the comment to the method browserSetup() all the logic with steps should be realized here as your test is not readable and it's not clear from the first sight what it tests There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree. I got it right doing the Cloud Calculator task. |
||
|
||
Assert.assertEquals(actualPageTitle, NEW_PASTE_TITLE, "Submitted paste title doesn't match initial paste title"); | ||
} | ||
|
||
@Test(description = "Check if syntax highlighting is Bash") | ||
public void checkSubmittedPasteSyntaxHighlighting() { | ||
String actualSyntax = pastebinSubmittedPastePage.getSyntaxSubmittedPaste(); | ||
|
||
Assert.assertEquals(actualSyntax, EXPECTED_PASTE_SYNTAX, "Submitted paste syntax highlighting is not Bash"); | ||
} | ||
|
||
@Test(description = "Check if submitted paste text matches initial text") | ||
public void checkSubmittedPasteText() { | ||
String actualText = pastebinSubmittedPastePage.getSubmittedPasteText(); | ||
|
||
Assert.assertEquals(actualText, NEW_PASTE_TEXT, "Submitted paste text doesn't match initial paste text"); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
package Hardcore.page; | ||
|
||
import org.openqa.selenium.JavascriptExecutor; | ||
import org.openqa.selenium.WebDriver; | ||
import org.openqa.selenium.WebElement; | ||
import org.openqa.selenium.support.PageFactory; | ||
import org.openqa.selenium.support.ui.ExpectedConditions; | ||
import org.openqa.selenium.support.ui.WebDriverWait; | ||
|
||
import java.util.ArrayList; | ||
|
||
public abstract class AbstractPage { | ||
protected WebDriver driver; | ||
|
||
protected final int WAIT_TIMEOUT_SECONDS = 10; | ||
|
||
protected AbstractPage(WebDriver driver) { | ||
this.driver = driver; | ||
PageFactory.initElements(driver, this); | ||
} | ||
|
||
protected WebElement waitForWebElementVisible(WebElement element) { | ||
return new WebDriverWait(driver, WAIT_TIMEOUT_SECONDS).until(ExpectedConditions.visibilityOf(element)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. using Fluent waits can also be considered |
||
} | ||
|
||
protected void openNewTab() { | ||
((JavascriptExecutor) driver).executeScript("window.open()"); | ||
} | ||
|
||
public void switchToPreviousTab() { | ||
ArrayList<String> tabs = new ArrayList<>(driver.getWindowHandles()); | ||
String currentTab = driver.getWindowHandle(); | ||
int currentTabIndex = tabs.indexOf(currentTab); | ||
driver.switchTo().window(tabs.get(currentTabIndex - 1)); | ||
} | ||
|
||
public void switchToNextTab() { | ||
ArrayList<String> tabs = new ArrayList<>(driver.getWindowHandles()); | ||
String currentTab = driver.getWindowHandle(); | ||
int currentTabIndex = tabs.indexOf(currentTab); | ||
driver.switchTo().window(tabs.get(currentTabIndex + 1)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
package Hardcore.page; | ||
|
||
import org.openqa.selenium.Keys; | ||
import org.openqa.selenium.WebDriver; | ||
import org.openqa.selenium.WebElement; | ||
import org.openqa.selenium.support.FindBy; | ||
|
||
public class GoogleCloudMainPage extends AbstractPage { | ||
String GC_BASE_URL = "https://cloud.google.com/"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. String variable to be moved to Test class |
||
|
||
@FindBy(name = "q") | ||
WebElement searchInput; | ||
|
||
@FindBy(xpath = "//div[@class='devsite-cse-results']") | ||
WebElement resultsPage; | ||
|
||
@FindBy(xpath = "//b[text()='Google Cloud Platform Pricing Calculator']") | ||
WebElement resultPattern; | ||
|
||
public GoogleCloudMainPage(WebDriver driver) { | ||
super(driver); | ||
} | ||
|
||
public GoogleCloudMainPage openPage() { | ||
driver.get(GC_BASE_URL); | ||
return this; | ||
} | ||
|
||
public GoogleCloudMainPage searchTerm(String term) { | ||
waitForWebElementVisible(searchInput).sendKeys(term + Keys.ENTER); | ||
return this; | ||
} | ||
|
||
public GoogleCloudMainPage clickCalculatorPage() { | ||
waitForWebElementVisible(resultsPage); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. out of interest - why do you use Wait here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did it because without wait it wasn't obvious that we click on the link from results, since this step was passed too fast. I agree that this is dumb to have it like that in terms of the test itself. I did it just for the purposes of flow demonstration. |
||
waitForWebElementVisible(resultPattern).click(); | ||
return this; | ||
} | ||
} |
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.
In your project you have 2 AbstractPage classes and if your project is extended further it may cause confusion. The best practice here would be to have one Abstract page for all your tests, but if to leave this implementation at least the naming of the Abstract class could be specified - e.g. AbstractBringItOn 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.
Appreciated for your feedback! I was trying to keep those two tasks separated, but still wanted them to share the same project, so I have it divided into one level packages. You're right, doing it this way, I might have named them differently.