Closed LerryAlexander closed 1 year ago
@LerryAlexander Thanks so much for this PR, this will allow us to use Docker Selenium without having to configure a relay node in Docker. This is something my team is eagerly awaiting. Is there any sort of timeline for moving this forward?
cc @budtmo
@LerryAlexander Thanks so much for this PR, this will allow us to use Docker Selenium without having to configure a relay node in Docker. This is something my team is eagerly awaiting. Is there any sort of timeline for moving this forward?
cc @budtmo
Hi @esaari, thank you so much for your feedback on this PR, I'm willing to retake this feature and merge these changes into the master branch, but first I was hoping to have some feedback from @budtmo and maybe tackle the failed checks.
Hi @LerryAlexander ,
sorry for late response because I am still working on the whole restructuring for this project like I did on appium-docker-android to make it more readable and easier to maintain.
I have already commented few things so you can have a look and also you need to fix the failing test first, it seems the toml is missing and you can add it here.
To be honest, I dont use Selenium for long time so I dont know how it works with Selenium 4, but I dont really like installing jar file inside the project. does it need to be inside the same container?
let me know if you need any support from me.
Hi @budtmo ,
Thank you so much for your comments and feedback, they mostly make a lot of sense to me as well. I'm going to get back to you for any support or questions. I will be working on this in the following days.
Thanks in advance!
Merging #338 (b722286) into master (abcdf3f) will decrease coverage by
14.95%
. The diff coverage is50.00%
.
:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more
@@ Coverage Diff @@
## master #338 +/- ##
===========================================
- Coverage 70.39% 55.44% -14.95%
===========================================
Files 2 2
Lines 152 202 +50
===========================================
+ Hits 107 112 +5
- Misses 45 90 +45
Impacted Files | Coverage Δ | |
---|---|---|
src/app.py | 54.31% <50.00%> (-15.08%) |
:arrow_down: |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
@LerryAlexander I am just curious about Selenium Hub integrated with Appium.
how stable the Selenium 4.0 that are connected to Appium Server (compared with Selenium 3.x)? I have tried with Selenium 3.x long time ago and it is not stable at all and thats why I dont use Selenium Hub anymore and I created my own hub privately to replace it.
@LerryAlexander I am just curious about Selenium Hub integrated with Appium.
how stable the Selenium 4.0 that are connected to Appium Server (compared with Selenium 3.x)? I have tried with Selenium 3.x long time ago and it is not stable at all and thats why I dont use Selenium Hub anymore and I created my own hub privately to replace it.
That's a good question @budtmo, I have used selenium 3.x in the past, and works fine but I've seen some instability as well. To be honest, I've used Selenium grid 4.x only for creating this PR, and seems to be more stable, it would be fair to maybe give it a chance to Selenium 4.x
@budtmo I have used Selenium 4 Grid extensively and it's quite a bit better than Selenium 3. One big compelling feature of Selenium Grid 4 is that devtool protocol is supported, so that gives us the ability to do things like throttling, network request interception, etc.
Is it possible we could reopen this PR and allow @LerryAlexander to continue his work? I think quite a few people are eagerly anticipating this feature.
have already discussed with @LerryAlexander privately and it is the best way to do this "custom selenium node" in docker-selenium project to get better update and maintenance activity as it include downloading selenium.jar file and the custom node could probably used for other type of node (not just for Appium) in the future. Lerry is a bit busy so it might take time but feel free if you want to help with the implementation in docker-selenium project @esaari
Purpose of changes
Currently,
docker-android
doesn't support the selenium grid with the newest architecture of Selenium 4, which means the selenium grid architecture has changed too, so in order to be able to use the new grid 4 architecture I'm proposing this PR which contains the following main changes:Files:
src/app.py
: added code logic to accept new node connections using the Selenium Grid 4 architecture when the environment variableCONNECT_TO_GRID_4=true
.src/run_app.sh
: new file added before runningpython -m src.app
to install all dependencies for running python withtoml
package. The new selenium grid architecture now usestoml
file instead ofjson
file to create node configurations. More info about this heresrc/start-selenium-node.sh
: this new file added contains the logic to start a new selenium grid node docker using the selenium 4 architecture. This file is basically taken directly from the SeleniumHQ/docker-selenium repo using the same approach from this file to create and connect new docker nodes to selenium hub. To read more about this code logic and how it works you can take a look at this folderdocker-compose-grid-4.yml
: this new docker-compose file is added as a reference or as an example of how to usedocker-android
with selenium grid using the latest selenium hub architecture and the use of the new environment variables.Environment Variables:
CONNECT_TO_GRID_4
: new (optional) environment variable to differentiate grid connections with selenium 4 (new architecture) from selenium 3 or below (old architecture).SE_EVENT_BUS_HOST
,SE_EVENT_BUS_PUBLISH_PORT
, andSE_EVENT_BUS_SUBSCRIBE_PORT
are for registering node docker through the Event Bus defined in Selenium 4. The explanation of this process is here in the official documentation of the SeleniumHQ/docker-selenium repo. Note: Since these variables are always set with the same value, maybe we can just remove them so the user doesn’t have to take care of those values.Types of changes
Put an
x
in the boxes that applyHow has this been tested?
These changes were locally tested on a Mac OSx machine.