-
Notifications
You must be signed in to change notification settings - Fork 254
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
Support version notation for selenium browsers #60
Conversation
With this patch, you can set MOODLE_DOCKER_BROWSER to options like the existing options: firefox chrome But also you can specify a tag/version for each, such as: firefox:2.53.1 chrome:3.5.2 Fixes #20
This is completely untested on windows - any testers welcome! |
I still have a win env used at the time of #34 (comment): could you provide one/two "package" to be tested or do you want all the behat tests to be run? |
Thanks Matteo, I'm only concerned about testing the |
Here is the result of the tests done about setting up the env (before running the test):
Will comment on your PR. |
bin/moodle-docker-compose.cmd
Outdated
REM Split MOODLE_DOCKER_BROWSER by : to get selenium tag if sepecified | ||
FOR /f "tokens=1,2 delims=:" %%i in ("%MOODLE_DOCKER_BROWSER%") do ( | ||
SET MOODLE_DOCKER_BROWSER_NAME=%%i | ||
SET MOODLE_DOCKER_BROWER_TAG=%%j |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: BROWER
instead of BROWSER
bin/moodle-docker-compose.cmd
Outdated
SET MOODLE_DOCKER_BROWER_TAG=%%j | ||
) | ||
|
||
IF "%MOODLE_DOCKER_BROWER_NAME%"=="" ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: BROWER
instead of BROWSER
bin/moodle-docker-compose.cmd
Outdated
SET MOODLE_DOCKER_BROWER_NAME="firefox" | ||
) | ||
|
||
IF "%MOODLE_DOCKER_BROWER_TAG%"=="" ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: BROWER
instead of BROWSER
bin/moodle-docker-compose.cmd
Outdated
) | ||
|
||
IF "%MOODLE_DOCKER_BROWER_NAME%"=="" ( | ||
SET MOODLE_DOCKER_BROWER_NAME="firefox" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: BROWER
instead of BROWSER
Error: do not use double quotes when setting a variable otherwise it will contain the quotes too
bin/moodle-docker-compose.cmd
Outdated
|
||
IF "%MOODLE_DOCKER_BROWER_TAG%"=="" ( | ||
IF "%MOODLE_DOCKER_BROWSER_NAME%"=="firefox" ( | ||
SET MOODLE_DOCKER_BROWER_TAG="2.53.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error: do not use double quotes when setting a variable otherwise it will contain the quotes too
bin/moodle-docker-compose.cmd
Outdated
SET MOODLE_DOCKER_BROWER_TAG="2.53.1" | ||
) | ||
IF "%MOODLE_DOCKER_BROWSER_NAME%"=="chrome" ( | ||
SET MOODLE_DOCKER_BROWER_TAG="3.5.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error: do not use double quotes when setting a variable otherwise it will contain the quotes too
Too late this night, here is the first diff - we have a regression here for host (mine) and vnc port (yours) too:
Stopped w/ the difference between
My proposal - to be tested - is:
|
I have put your fixes onto a commit on this branch |
OK, will do a second session tonight on top of e4db7bf. Not sure if the fix about localhost and VNC should be separated commits but let the branch be ready for windows too, first 😉. |
Thanks (my problem is that my vms do not have VT support so can't run docker in them). I might see if i can work something out meanwhile |
Oops... a debug echo 😊
Will test and give you diffs, tonight. |
I've finally got a windows install for this.. I came across problems with the old version setting variables which affect future runs. Perhaps we should avoid re-using variables which we expect users to set (in both windows and mac) |
I've successfully run these combinations in the same DOS prompt:
given that we should remove:
Cleaning up the way variables are used could be a separate issue, isn't it? HTH, |
Sorry for arriving late here, just saw #20 some minutes ago... My 2 cents are just to say that I find I mean I'd expect the expansion proposed controls the browser version, not the selenium one. So this is a call to 1) consider if a better name can be used and/or 2) make it 100% clear that it's the selenium version (with whichever browser versions it comes with). Just that, other than that I understand and like the idea/feature 100%. Thanks! |
I tried running the selenium branch last night on Windows 10, with Docker for Windows, using cmd and it failed to compile the containers. I got a message saying it could not find the moodlehq/moodle-php-apache:"7.1" image It works fine on Windows 7 using Docker Toolbox though. I guess the difference here is that Docker will be running on a Linux virtual machine. I will try on Win10 again hopefully tonight, but I'm fairly certain Matteo's latest changes were included at the time. |
Hi @NeillM,
HTH, |
Hi @danpoltawski, TIA, |
I have to say i'm not keen on calling it My intention here is to avoid introducing another variable.. we've already got 8 and I see this quickly getting out of hand.. |
It's clear we need to sort out this windows problem sooner rather than later though. Might be worth splitting the windows fixes in a seperate pull request? |
👍 |
Without this fix the compose stops with an error: >set MOODLE_DOCKER_WWWROOT=C:\Projects\Moodle\moodle-master >set MOODLE_DOCKER_DB=mysql >cd C:\Projects\Moodle\moodle-docker >copy config.docker-template.php %MOODLE_DOCKER_WWWROOT%\config.php >bin\moodle-docker-compose.cmd up -d ERROR: no such image: selenium/standalone-firefox"":2.53.1: invalid reference format Already found while working at moodlehq#20 in refining moodlehq#60.
As far as this will be 2y old in just one week... maybe we should try to get it done, accepting the browser:selenium format... i really don't remember that as a stopper, just a opinion (or at least that's how I see it now, 2 years later) :-) |
Closed by #195, just merged. |
With this patch, you can set MOODLE_DOCKER_BROWSER to options like the
existing options:
firefox
chrome
But also you can specify a tag/version for each, such as:
firefox:2.53.1
chrome:3.5.2
Fixes #20