-
Notifications
You must be signed in to change notification settings - Fork 70
Adding docker + protractor docs to cookbook. #9
Conversation
jags14385
commented
Sep 15, 2016
- Documentation.
@cnishina Resubmitted it with cla. |
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.
See comments. This should have a package.json that downloads protractor
as a dependency.
----------------------------------------------------------------------------- | ||
``` shell | ||
docker run -d --link selenium-hub:hub selenium/node-chrome:latest | ||
docker run -d --link selenium-hub:hub selenium/node-firefox:latest |
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.
If this example is just showing the latest node-chrome
, let's take out the node-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.
Removed it.
|
||
Install [RealVNC](https://www.realvnc.com) viewer. | ||
|
||
One can add a chrome-debug-node which has a vnc server set up to the selenium grid and see the tests running . |
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.
Discuss that in the debug, you should not use node-chrome
and should use node-chrome-debug
instead to use VNC Viewer when starting up your selenium grid
Also, this is noted as chrome-debug-node
which should be changed to node-chrome-debug
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.
Edited it.
docker-compose up -d --remove-orphans | ||
docker-compose scale chromenode=5 | ||
|
||
./node_modules/.bin/protractor docker-protractor-grid-conf.js |
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.
This should have a package.json
to download protractor. In your package.json
, run this script as:
"scripts": {
"pretest": "./run-tests.sh",
"test": "protractor docker-protractor-grid-conf.js"
}
Then the user can just run npm test
. Also, maybe we should rename the "run-tests.sh" to "docker-setup.sh"
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.
Added it as part of the "test" script in the package.json
``` | ||
|
||
``` shell | ||
chmod +x run-tests.sh |
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.
You could chmod +x
the file and add the permissions to the pull request then the user will not need to make it executable.
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.
gave it +x permissions.
@@ -0,0 +1,12 @@ | |||
docker -v |
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.
Let's rename the file to "docker-setup.sh" and give it "chmod +x" permissions. See comments above.
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.
True. Renamed it.
docker-compose up -d --remove-orphans | ||
docker-compose scale chromenode=5 | ||
|
||
./node_modules/.bin/protractor docker-selenium-grid-conf.js |
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.
Remove this and add it to package.json as part of the test script.
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.
Referenced the above as a shell script file and "npm install" as part of the "pretest" script.
Would still keep it in the .md file so that the reader knows what to do.
@cnishina What do you think ?
@cnishina Done with the code changes. Let me know , if this looks good . |
"name": "protractor-docker-setup", | ||
"dependencies": { | ||
"protractor": "4.0.8", | ||
"chromedriver": "2.24.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.
Chromedriver is not required if we are using docker.
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.
Done.
@@ -0,0 +1,19 @@ | |||
exports.config = { | |||
framework: 'jasmine2' | |||
, specs: ['specs/*.js'] |
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.
Could we have a spec file that does a really simple test? It could just make a request for angularjs.org and check to see if the title matches.
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.
Will add.
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.
Done.
, capabilities: { | ||
browserName: 'chrome' | ||
, shardTestFiles: true | ||
, maxInstances: 5 |
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.
I think we should remove shardTestFiles and maxInstances. These are not necessarily docker specific. However, in you feel like it is, you should add a comment about why these values are set.
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.
Did not think about it, will have to run the tests too check it out .
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.
@cnishina We need so that we can run the 5 spec files in parallel at the same time. Without all the tests run on one single node. Checked it. Will mention it.
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.
Done.
@@ -0,0 +1,147 @@ | |||
Running Protractor Tests on Docker |
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.
This file is really awesome and should be available to read when the user navigates to this folder on github. This should be changed to README.md.
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.
Agreed.
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.
Done
, getPageTimeout: 180000 | ||
, allScriptsTimeout: 180000 | ||
, onPrepare: function () { | ||
browser.manage().window().maximize(); |
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.
Add comment about why onPrepare
we need to maximize the window. This could be necessary if a user is running debug and wants to view the website... also add a comment about implicitylyWait
.
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.
Done.
@cnishina Added with the changes asked. Thanks for being patient. |
This is a great addition. Thank you for the PR! |