Skip to content

first level review #11

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

first level review #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,12 @@ The Selenium tests are run on different platforms like on-prem, docker and Brows

Maven:
```sh
mvn install
Copy link
Contributor

Choose a reason for hiding this comment

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

Why skipping tests? if they are failing they should be fixed.

Copy link
Collaborator

@samirans89 samirans89 Dec 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

intention is not to skip the tests but to have build and test commands separated out.
the test commands are already called out as we progress through the readme.
the install command with tests will give users a bad experience since they haven't followed the "Prerequisites" section for the default profile of the readme yet

which is here: Prerequisites

mvn install -DskipTests
```

Gradle:
```sh
gradle build
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above.

gradle build -x test
```

## About the tests in this repository
Expand Down
16 changes: 6 additions & 10 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@
* This file was generated by the Gradle 'init' task.
*/

/* groovylint-disable-next-line CompileStatic */
plugins {
id 'java'
id 'maven-publish'
}

repositories {
mavenLocal()
mavenCentral()
google()
}
Expand All @@ -19,25 +19,21 @@ dependencies {
testImplementation 'com.browserstack:browserstack-local-java:1.0.6'
testImplementation 'org.apache.commons:commons-lang3:3.11'
testImplementation 'org.slf4j:slf4j-api:1.7.30'
testImplementation 'ch.qos.logback:logback-classic:1.2.3'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove this library?

Copy link
Collaborator

Choose a reason for hiding this comment

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

i couldn't find the usage in the first level glance of the repo. okay to keep if this is being used.

testImplementation 'org.testng:testng:7.4.0'
testImplementation 'io.cucumber:cucumber-java:6.10.4'
testImplementation 'io.cucumber:cucumber-core:6.10.4'
testImplementation 'io.cucumber:cucumber-picocontainer:6.10.4'
testImplementation 'io.cucumber:cucumber-testng:6.10.4'
testImplementation 'com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:2.12.2'
testImplementation 'com.fasterxml.jackson.core:jackson-databind:2.12.2'
testImplementation 'com.browserstack:webdriver-framework-testng:0.0.1'
}

group = 'com.browserstack'
version = '0.0.1-SNAPSHOT'
description = 'browserstack-examples-cucumber-testng'
java.sourceCompatibility = JavaVersion.VERSION_1_8
Comment on lines -32 to -35
Copy link
Contributor

Choose a reason for hiding this comment

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

Reason to remove?

Copy link
Collaborator

Choose a reason for hiding this comment

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

we are picking up from remote URL right?

I added - "testImplementation 'com.browserstack:webdriver-framework-testng:0.0.1'"

I believe the above code picks up the lib locally. is that needed?


tasks.named('test') {
systemProperty "cucumber.filter.tags", System.getProperty("cucumber.filter.tags")
systemProperty "dataproviderthreadcount", System.getProperty("num.parallels")
useTestNG() {
/* groovylint-disable-next-line DuplicateStringLiteral */
systemProperty 'cucumber.filter.tags', System.getProperty('cucumber.filter.tags')
systemProperty 'dataproviderthreadcount', System.getProperty('num.parallels')
useTestNG {
suites 'src/test/resources/conf/testng.xml'
parallel = 'methods'
}
Expand Down
8 changes: 0 additions & 8 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,11 @@
<maven-surefire-plugin.version>2.22.2</maven-surefire-plugin.version>
<selenium-java.version>3.141.59</selenium-java.version>
<browserstack-local-java.version>1.0.6</browserstack-local-java.version>
<json-simple.version>1.1.1</json-simple.version>
<comslang.version>3.11</comslang.version>
<alluretestng.version>2.12.0</alluretestng.version>
<allure.version>2.10.0</allure.version>
<slf4j.version>1.7.30</slf4j.version>
<cucumber.version>6.9.1</cucumber.version>
<logback.version>1.2.3</logback.version>
<testng.version>7.4.0</testng.version>
<cucumber.version>6.10.4</cucumber.version>
<jackson.version>2.12.2</jackson.version>
Expand Down Expand Up @@ -56,12 +54,6 @@
<scope>test</scope>
</dependency>

<dependency>
<groupId>ch.qos.logback</groupId>
<artifactId>logback-classic</artifactId>
<version>${logback.version}</version>
<scope>test</scope>
</dependency>
Comment on lines -59 to -64
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above


<dependency>
<groupId>org.testng</groupId>
Expand Down
1 change: 1 addition & 0 deletions settings.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* groovylint-disable CompileStatic */
/*
* This file was generated by the Gradle 'init' task.
*/
Expand Down
5 changes: 0 additions & 5 deletions src/test/java/com/browserstack/examples/stepdefs/AbstractBaseSteps.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,6 @@
import com.browserstack.examples.tests.RunWebDriverCucumberTests;
import com.browserstack.webdriver.core.WebDriverFactory;

/**
* Created with IntelliJ IDEA.
*
* @author Anirudha Khanna
*/
Comment on lines -8 to -12
Copy link
Contributor

Choose a reason for hiding this comment

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

Please edit the docs not remove them

Copy link
Collaborator

Choose a reason for hiding this comment

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

removed since we have not followed this throughout the repos... even for this repo ... it is not consistently followed across all files. can we descope for now?

public abstract class AbstractBaseSteps {

private WebDriver webDriver;
Expand Down
27 changes: 13 additions & 14 deletions src/test/java/com/browserstack/examples/stepdefs/HomePageSteps.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package com.browserstack.examples.stepdefs;



import java.util.List;
import java.util.stream.Collectors;
import org.openqa.selenium.By;
Expand All @@ -14,15 +12,16 @@
import io.cucumber.java.en.And;
import io.cucumber.java.en.Then;


public class HomePageSteps extends AbstractBaseSteps {

WebDriverWait wait;

@And("I add two products to cart")
public void iAddProductsToCart() {
WebDriverWait wait = new WebDriverWait(getWebDriver(), 10);
Utility.waitForJSLoad(getWebDriver());
wait.until(ExpectedConditions.visibilityOfElementLocated(By.cssSelector("#\\31 > .shelf-item__buy-btn"))).click();
wait.until(ExpectedConditions.visibilityOfElementLocated(By.cssSelector("#\\31 > .shelf-item__buy-btn")))
.click();
wait.until(ExpectedConditions.visibilityOfElementLocated(By.cssSelector("div.float-cart__close-btn"))).click();
getWebDriver().findElement(By.cssSelector("#\\32 > .shelf-item__buy-btn")).click();
Utility.waitForJSLoad(getWebDriver());
Expand All @@ -38,8 +37,7 @@ public void iPressTheAppleVendorFilter() {
try {
getWebDriver().findElement(By.cssSelector(".filters-available-size:nth-child(2) .checkmark")).click();
Utility.waitForJSLoad(getWebDriver());
}
catch (NoSuchElementException e) {
} catch (NoSuchElementException e) {
throw new AssertionError();
}
}
Expand All @@ -55,7 +53,8 @@ public void iShouldUserLoggedIn(String user) {
WebDriverWait wait = new WebDriverWait(getWebDriver(), 10);
Utility.waitForJSLoad(getWebDriver());
try {
String loggedInUser = wait.until(ExpectedConditions.visibilityOfElementLocated(By.cssSelector(".username"))).getText();
String loggedInUser = wait.until(ExpectedConditions.visibilityOfElementLocated(By.cssSelector(".username")))
.getText();
Assert.assertEquals(user, loggedInUser);
} catch (NoSuchElementException e) {
throw new AssertionError(user + " is not logged in");
Expand All @@ -69,7 +68,7 @@ public void iShouldSeeNoImageLoaded() {
String src = "";
try {
src = getWebDriver().findElement(By.cssSelector("img[alt='iPhone 12']")).getAttribute("src");
Assert.assertEquals(src.isEmpty(),false);
Assert.assertEquals(src.isEmpty(), false);
} catch (NoSuchElementException e) {
throw new AssertionError("No Images are loaded");
}
Expand All @@ -80,8 +79,10 @@ public void iShouldSeeItemsInTheList(int productCount) {
try {
WebDriverWait webDriverWait = new WebDriverWait(getWebDriver(), 10);
Utility.waitForJSLoad(getWebDriver());
webDriverWait.until(ExpectedConditions.visibilityOf(getWebDriver().findElement(By.cssSelector(".shelf-item__title"))));
List<String> values = getWebDriver().findElements(By.cssSelector(".shelf-item__title")).stream().map(WebElement::getText).collect(Collectors.toList());
webDriverWait.until(
ExpectedConditions.visibilityOf(getWebDriver().findElement(By.cssSelector(".shelf-item__title"))));
List<String> values = getWebDriver().findElements(By.cssSelector(".shelf-item__title")).stream()
.map(WebElement::getText).collect(Collectors.toList());
Assert.assertEquals(productCount, values.size());
} catch (NoSuchElementException e) {
throw new AssertionError("Error in page load");
Expand All @@ -90,12 +91,10 @@ public void iShouldSeeItemsInTheList(int productCount) {

@Then("I should see prices in ascending order")
public void iShouldSeePricesInAscendingOrder() {
WebDriverWait wait = new WebDriverWait(getWebDriver(), 10);
try {
WebDriverWait webDriverWait = new WebDriverWait(getWebDriver(), 10);
Utility.waitForJSLoad(getWebDriver());
// webDriverWait.until(ExpectedConditions.visibilityOf(ThreadLocalDriver.getWebDriver().findElement(By.cssSelector(".shelf-item__price > div.val > b"))));
List<WebElement> priceWebElement = getWebDriver().findElements(By.cssSelector(".shelf-item__price > div.val > b"));
List<WebElement> priceWebElement = getWebDriver()
.findElements(By.cssSelector(".shelf-item__price > div.val > b"));
Utility.waitForJSLoad(getWebDriver());
Assert.assertTrue(Utility.isAscendingOrder(priceWebElement, priceWebElement.size()));
} catch (NoSuchElementException e) {
Expand Down
5 changes: 0 additions & 5 deletions src/test/java/com/browserstack/examples/tests/RunWebDriverCucumberTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,6 @@
import io.cucumber.testng.PickleWrapper;
import io.cucumber.testng.TestNGCucumberRunner;

/**
* Created with IntelliJ IDEA.
*
* @author Anirudha Khanna
*/
Comment on lines -19 to -23
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Collaborator

Choose a reason for hiding this comment

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

removed since we have not followed this throughout the repos... even for this repo ... it is not consistently followed across all files. can we descope for now?

@CucumberOptions(
features = "classpath:features",
glue = "com.browserstack.examples.stepdefs",
Expand Down
1 change: 0 additions & 1 deletion src/test/resources/capabilities-single.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ cloudDriver:
browserstack.debug: true
browserstack.networkLogs: true
browserstack.console: debug
# browserstack.idleTimeout: 300
platforms:
- name: Win10_Chrome_Latest-1
os: Windows
Expand Down
11 changes: 0 additions & 11 deletions src/test/resources/capabilities.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ cloudDriver:
browserstack.debug: true
browserstack.networkLogs: true
browserstack.console: debug
# browserstack.idleTimeout: 300
platforms:
- name: Win10_IE11
os: Windows
Expand All @@ -39,18 +38,8 @@ cloudDriver:
browser_version: latest
capabilities:
browserstack.selenium_version: 3.141.59
# - name: OSX_BigSur_Chrome_Latest
# os: OS X
# os_version: Catalina
# browser: Safari
# browser_version: '13.0'
Comment on lines -42 to -46
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we add it back.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we could... wasn't sure on why we commented out. hence removed for now.

- name: Android_Samsung Galaxy S21_11.0
os: Android
os_version: '11.0'
device: Samsung Galaxy S21
real_mobile: true
# - name: iOS_iPhone XS_13.0
# os: iPhone
# os_version: '13.0'
# device: iPhone XS
# real_mobile: true
Comment on lines -52 to -56
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we add it back as uncommented.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we could... wasn't sure on why we commented out. hence removed for now.