first level review #11
Conversation
| @@ -27,12 +27,12 @@ The Selenium tests are run on different platforms like on-prem, docker and Brows | |||
|
|
|||
| Maven: | |||
| ```sh | |||
| mvn install | |||
There was a problem hiding this comment.
Why skipping tests? if they are failing they should be fixed.
There was a problem hiding this comment.
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
| ``` | ||
|
|
||
| Gradle: | ||
| ```sh | ||
| gradle build |
| @@ -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' | |||
There was a problem hiding this comment.
Why remove this library?
There was a problem hiding this comment.
i couldn't find the usage in the first level glance of the repo. okay to keep if this is being used.
| group = 'com.browserstack' | ||
| version = '0.0.1-SNAPSHOT' | ||
| description = 'browserstack-examples-cucumber-testng' | ||
| java.sourceCompatibility = JavaVersion.VERSION_1_8 |
There was a problem hiding this comment.
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?
| <dependency> | ||
| <groupId>ch.qos.logback</groupId> | ||
| <artifactId>logback-classic</artifactId> | ||
| <version>${logback.version}</version> | ||
| <scope>test</scope> | ||
| </dependency> |
| /** | ||
| * Created with IntelliJ IDEA. | ||
| * | ||
| * @author Anirudha Khanna | ||
| */ |
There was a problem hiding this comment.
Please edit the docs not remove them
There was a problem hiding this comment.
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?
| /** | ||
| * Created with IntelliJ IDEA. | ||
| * | ||
| * @author Anirudha Khanna | ||
| */ |
There was a problem hiding this comment.
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?
| # - name: OSX_BigSur_Chrome_Latest | ||
| # os: OS X | ||
| # os_version: Catalina | ||
| # browser: Safari | ||
| # browser_version: '13.0' |
There was a problem hiding this comment.
Shouldn't we add it back.
There was a problem hiding this comment.
we could... wasn't sure on why we commented out. hence removed for now.
| # - name: iOS_iPhone XS_13.0 | ||
| # os: iPhone | ||
| # os_version: '13.0' | ||
| # device: iPhone XS | ||
| # real_mobile: true |
There was a problem hiding this comment.
Shouldn't we add it back as uncommented.
There was a problem hiding this comment.
we could... wasn't sure on why we commented out. hence removed for now.
No description provided.