PR to review code with mentor #1
Conversation
| import org.openqa.selenium.support.ui.ExpectedConditions; | ||
| import org.openqa.selenium.support.ui.WebDriverWait; | ||
|
|
||
| public abstract class AbstractPage { |
There was a problem hiding this comment.
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.
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.
| super(driver); | ||
| } | ||
|
|
||
| public PastebinMainPage openPage() { |
There was a problem hiding this comment.
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.
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 = new ChromeDriver(); | ||
| driver.manage().window().maximize(); | ||
| pastebinMainPage = new PastebinMainPage(driver) | ||
| .openPage() |
There was a problem hiding this comment.
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.
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.
| .selectExpiration10M() | ||
| .typeInNewPasteTitle(NEW_PASTE_TITLE) | ||
| .submitNewPaste(); | ||
| pastebinSubmittedPastePage = new PastebinSubmittedPastePage(driver); |
There was a problem hiding this comment.
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.
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.
| @Test(description = "Check if submitted paste title matches initial paste title") | ||
| public void checkSubmittedPastePageTitle() { | ||
| WebElement titleSubmittedPaste = waitForElement(By.xpath(SUBMITTED_PASTE_TITLE_LOCATOR)); | ||
| String actualPageTitle = titleSubmittedPaste.getText(); |
There was a problem hiding this comment.
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.
Agree. I got it right doing the Cloud Calculator task.
| 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.
using locators in tests is not a good practice, it should be moved from here
| } | ||
|
|
||
| public GoogleCloudMainPage clickCalculatorPage() { | ||
| waitForWebElementVisible(resultsPage); |
There was a problem hiding this comment.
out of interest - why do you use Wait here?
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| public GoogleCloudPricingCalculatorPage fillInEstimationForm() { | ||
| String instances = "4"; |
| dropdownLocation.click(); | ||
| waitForWebElementVisible(itemLocation).click(); | ||
| dropdownUsage.click(); | ||
| waitForWebElementVisible(itemUsage).click(); |
There was a problem hiding this comment.
The method is overloaded and not clear - need to decompose it - you can split it into smaller logical private methods and trigger them here in the public method.
Moreover private small methods can be converted into public and used in your tests when they need to be extended
| } | ||
|
|
||
| protected WebElement waitForWebElementVisible(WebElement element) { | ||
| return new WebDriverWait(driver, WAIT_TIMEOUT_SECONDS).until(ExpectedConditions.visibilityOf(element)); |
There was a problem hiding this comment.
using Fluent waits can also be considered
|
|
||
| public GoogleCloudPricingCalculatorPage switchToCalculatorIFrame() { | ||
| driver.switchTo().frame(frameMain); | ||
| driver.switchTo().frame("myFrame"); |
No description provided.