Skip to content
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

Refactor data providers #121

Conversation

sebady
Copy link

@sebady sebady commented Jul 15, 2015

This pull request builds on changes in the dsimmons-changes branch to refactor our Data Providers into a standalone module.

This provides the benefit of allowing users to leverage our DataProviders with testng automation.

@sebady sebady changed the title Patch sebady refactor data providers Refactor data providers Jul 15, 2015
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<suiteXmlFiles>
<suiteXmlFile>${suiteXmlFile}</suiteXmlFile>
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this dataproviderSuiteXmlFile... if you build from the parent directory with -DsuiteXmlFile=PhantomJS-Suite.xml this module will not build because there is no such suite.. This is why travis ci has failed your PR -- https://travis-ci.org/paypal/SeLion/builds/71008236

Copy link
Author

Choose a reason for hiding this comment

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

I will fix this.

@krmahadevan
Copy link

@sebady
I have a few questions on this delivery.
I don't think we have yet come to a consensus on #114 Vs #115 .
Why is this PR targeting dsimmons-changes branch (which basically represents #114 ) rather than the develop branch ?

So I would recommend that you please hold off on this till we come to a consensus on that, (or) branch off of develop and raise a new Pull Request.

@sebady
Copy link
Author

sebady commented Jul 16, 2015

This PR builds on dsimmons-changes branch only because it requires the common module introduction on that branch. Having a new common module already in place make this dataprovider refactoring much cleaner to achieve at this point.

@krmahadevan
Copy link

@sebady
Then I believe we would need to extract out that part as a separate PR, vet that out, get it merged and have your branch rebase from develop. Else your PR is also likely to be stalled.

@mach6
Copy link
Contributor

mach6 commented Jul 16, 2015

114 is not stalled. Rather, it was not ready.

I was pretty clear in my comments in 115 that it should come as an iterative change after 114. As a result, we are targeting 114 first (when it's ready).. I'm not going to piece-mill 114 into multiple PR's. All of the changes are relevant to the forward progress we are tracking in M5 (refactoring, code and dependency cleanup, and minimal feature changes). Furthermore 114 puts us in a place to do a pre-release (beta or RC) without changing the existing approach.

We need to instead be talking about when and how the proposed changes in 115 come to light. My suggestion was after 1.0.0 but I could also see it before 1.0.0. I could also see a couple 1.0.0-beta releases between now and 1.0.0.. The point is we are close to some kind of release.. Generally speaking, outside of refactoring and optimizing our existing capabilities, we don't want serious changes between now and then.. We also don't want serious changes between now and the 1.0.0. Exceptions, for the right reasons and depending on timing/external considerations, etc are certainly acceptable. I feel 115 and this PR (121) are reasonable examples.. The important thing here is to figure out the timing and remain vigilant in our efforts to get to the 1.0.0 release.

@krmahadevan
Copy link

@mach6
I dont understand why #115 needs to come in as an iterative change over #114 (especially when it is eventually going to end up removing off a bunch of classes that were introduced in #114)

Furthermore 114 puts us in a place to do a pre-release (beta or RC) without changing the existing approach.

I believe this is the same case with #115 as well. I am yet to hear of any reasons which is going to state that this isn't the case. I believe both @fakrudeen78 and @sundaramrajendran took a look at it and none of them seemed to have called out anything which would be of cause for concern.

Generally speaking, outside of refactoring and optimizing our existing capabilities, we don't want serious changes between now and then..

I am not sure if you took a closer look at #115, but it is also just refactoring and optimizing existing capabilities. I am not sure what are you calling out here as serious changes.

@mach6
Copy link
Contributor

mach6 commented Jul 16, 2015

(especially when it is eventually going to end up removing off a bunch of classes that were introduced in #114)

Let's see. In my opinion, 115 will remove classes which were pre-existing and possibly refactored as part of 114. Having capabilities in "selion-grid" which add value, whether or not they are used by "client", to me, is reasonable.

I believe this is the same case with #115 as well. I am yet to hear of any reasons which is going to state that this isn't the case.

I don't disagree. There are other considerations in 114 which are done that is not part of 115. dependency convergence, misc fixes, minor improvement, common, etc. Regardless of the way client spins up a local web-only webdriver hub+node OR standalone+[chromedriver,phantomjs,etc] OR [chromedriver,phantoms,etc] in isolation (which imo, is the way we should do things for local web), the other changes should be incorporated.

I am not sure if you took a closer look at #115, but it is also just refactoring and optimizing existing capabilities. I am not sure what are you calling out here as serious changes.

Yes, I have looked at it in detail and have considered incorporating the salient parts into 114. I'd change some of the approach but don't really consider the root of the proposed change to be serious. I was speaking generically in my last comment about any potential change between now and release.

@mach6 mach6 force-pushed the dsimmons-changes branch 2 times, most recently from afd6180 to e98a1a8 Compare July 16, 2015 20:08
Doug Simmons added 3 commits July 20, 2015 11:13
nodes/hubs, introduce SeLion-Common, refactoring, and misc updates or fixes

Client Changes
-------------------
- Made Selenium-Server and SeLion-Grid dependencies optional in SeLion
  "client" - They are only required for local runs
- Removed ios-driver config values which do not apply to ios-driver 0.6.5
- Removed AbstractNode
- Introduced AbstractBaseLocalServerComponent which provides
  base implementations for all LocalServerComponents
- Removed LocalGridConfigFileParser and test -- class no longer required
- Removed localnode.json from client's src/main/resources
- Changed LocalGridManager, LocalNode, LocalIOSNode, and LocalHub,
  and LocalSelendroidNode to use SeLion-Grid for their needs
- Nodes are now started on probed ports vs. fixed ports
- Chromedriver, phantoms, etc downloaded and used for local runs, when
  missing
- Remove Hub.java hack
- Upgrade TestNG, appium-client, and snakeyaml dependencies
- Added guarding against ClassNotFound exception when SeLion-Grid
  and/or selenium-server is/are not available when running locally via
  SeLion client

Server Changes
----------------------
- Refactoring and introduction of new launcher/spawner classes in SeLion-Grid
	- Added IOSDriverJarSpawner
	- Added SelendroidJarSpawner
	- Added AppiumSpawner
	- Added ThreadedLauncher for launching selenium hub and web nodes
	  in the same process on a different Thread
	- Added shutdown() to SeLionGridLauncher
	- Introduced interface RunnableLauncher
	- Added abstract and base classes for RunnableLaunchers which are
	  a) in the same process, b) in a separate process, and c) mobile, in a
	  separate process
	- Introduced option to disable continuous restart for the
	  RunnableLaunchers which are process spawners
	- Introduced enumeration InstanceType which is used to track the current
	  instance SeLion-Grid is managing and/or booting (e.g. hub, node, etc)
- Added JVM shutdown hook for launcher/runners which spawn a new
  process -- This should help cleanup on Windows
- Relocated class ArtifactDetails and reduced visibility, it is only used by the
  FileDownloader
- Moved several classes from depending org.apache.commons.lang3 (a
  transitive  dependency of selenium-server -- may not be in the class path) to
  org.apache.commons.lang (which is available)
- Updates to ConfigParser -- fixed LOGGER.entering and added ability to
  getJsonObject
- Added ability for download.json to specify artifact "roles" -- which when
  considered by  FileDownloader will only result in an artifact download
  (or "checksum" verification) if the  artifact in question is required for the
  current InstanceType
- SeLionConfig.json now allows the user to define default args for Appium,
  Selendroid, and ios-driver
- Introduced MinimalIOSCapabilityMatcher in "server"
- Add Default-Suite.xml for SeLion-Grid
- FileDownloader can now skip last modified check on file AND
  can be told to not cleanup downloads which may have happened
  previously in the same JVM process
- FileDownloader can now process by artifact names

Other Misc Changes
----------------------------
- Addressed some Javadoc issues from misc classes
- Enabled Dependency Convergence
- Fix paypal#103 -- Usage of org.json
- Add some test classes
- Downgrade to ios-driver 0.6.5 everywhere -- many of our tests and
  sample code still require ios-driver 0.6.6-SNAPSHOT. 0.6.6-SNAPSHPOT
  can be re-introduced by updating the SeLionConfig.json for SeLion-Grid
  and through an pom.xml update on the user's end.
- Introduction of SeLion-Common - Started to push classes which were
  duplicate or needed by more than one SeLion component into this new artifact
- Relocated and/or renamed some classes which were pushed to common
- Moved some SeLionGridConstant vars into common in new class
  SeLionConstants -- they are needed for both client and server and
  since client can run w/o server (it's an optional dependency), it made
  sense to do this.
- Updated archetype to include SeLion-Grid and selenium-server dependencies
Also in this change set;
- Update to https url for download locations
- Update some tests
Also in this change set;
- fix travis ci with docker containers and openjdk builds
- Change behavior of webdriver dependency download --
  disabled by default for LocalNode
- Fix LocalNode issue when local hub started on non-default port
Sherif Ebady added 4 commits July 20, 2015 17:15
Refactor the SeLion DataProviders into a standalone module dataprovider
that is independent of the rest of SeLion client and can be used by
clients using TestNG.

Moved SeLionLogger (as well as LoggerConfig) and FileAssistant into
common as well as their associated tests.
Added selionbuildinfo.properties for client-dataproviders.
Moved dataprovider test resources into client-dataprovider.
depenedencies and remove unnecesssary ones
Cleanup some selionbuildinfo properties
@sebady sebady force-pushed the patch-sebady-refactorDataProviders branch from 93b3d64 to a268889 Compare July 20, 2015 22:16
@mach6 mach6 force-pushed the dsimmons-changes branch 2 times, most recently from a848fd8 to b4a9c89 Compare July 21, 2015 18:31
@mach6 mach6 closed this Jul 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants