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

[OS-951, OS-971, OS-958] PoC Make Art chromium selenium web driver #433

Open
wants to merge 727 commits into
base: master
Choose a base branch
from

Conversation

lauraenria
Copy link
Contributor

Use Selenium web driver to test the poc make art in chromium

japonophile and others added 30 commits May 13, 2016 20:43
Changed icon background colour for Kano Apps
I18n support and Japanese translations
Adding rgb+rgba color functions for straightforward color creation
fix gotToPlayground function
implement drawFigure for playground
implement how long the canvas is going to take
take data
add module getTime
improved time testing
add color to console.log
change findelement with elementLocated
automate all test through challenges
delete console.log
add commentes
try to fix Go back home menu end challenges
@lauraenria lauraenria requested a review from tombettany May 3, 2019 11:36
@tombettany tombettany changed the title Os 951/poc make art chromium selenium web driver [OS-951] PoC Make Art chromium selenium web driver May 3, 2019
@tombettany tombettany changed the title [OS-951] PoC Make Art chromium selenium web driver [OS-951] PoC Make Art chromium selenium web driver May 8, 2019
@tombettany tombettany changed the title [OS-951] PoC Make Art chromium selenium web driver [OS-951] PoC Make Art chromium selenium web driver May 8, 2019
@tombettany tombettany changed the title [OS-951] PoC Make Art chromium selenium web driver [OS-951, OS-971] PoC Make Art chromium selenium web driver May 8, 2019
@tombettany tombettany changed the title [OS-951, OS-971] PoC Make Art chromium selenium web driver [OS-951, OS-971, OS-958] PoC Make Art chromium selenium web driver May 8, 2019
Copy link
Member

@tombettany tombettany left a comment

Choose a reason for hiding this comment

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

You should reorganise this a little to have something like:

...
tests/
    auto/
        testChallenges.js
        ...
    benchmark/
        launchBenchmarks.js
        ...
    manual/
        results/
            chromium.md
            webengine.md
            ...
    utils/
        navigation.js
        ...

Also, extract the Chromium PoC code, we can't merge in the selenium stuff with it.

module.exports = {
start : (el) => {
startTime = process.hrtime();
console.log(`startTime ${el} =>`.green, `${startTime[0]} s ${startTime[1]} ns`.yellow)
Copy link
Member

Choose a reason for hiding this comment

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

Console log slows down the running of the code and interferes with the benchmark. We don't really care what time it started, we only really care about how long it takes which is printed in the end() function. If you want to log when we start timing then simply move this above the process.hrtime() and change the message to be a generic "Timer started" or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I delete it


module.exports = {
myServer: myServer,
main: main
Copy link
Member

Choose a reason for hiding this comment

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

Not really the main, maybe export this as driver or something that lets us know that this is actually the driver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it as driver.

@@ -0,0 +1,20 @@
const { Builder } = require('selenium-webdriver');

let myServer = 'http://172.16.254.110:3000'
Copy link
Member

Choose a reason for hiding this comment

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

Hard-coded IP address. Either change this to http://127.0.0.1:3000 which will always point to the computer that you are on or don't set it and have it read from a configuration file.

Copy link
Contributor Author

@lauraenria lauraenria May 8, 2019

Choose a reason for hiding this comment

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

I am going to use http://127.0.0.1:3000 and probably give a look from a config file.

let myServer = 'http://172.16.254.110:3000'
const chrome = require('selenium-webdriver/chrome')
const chrome_options = new chrome.Options(); // you will need to setup Chroumium
chrome_options.setChromeBinaryPath('/usr/bin/chromium-browser');
Copy link
Member

Choose a reason for hiding this comment

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

Right now this is only for Chromium but it might be nice in the future to have the user able to choose which browser they wish to use when the run the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

.setChromeOptions(chrome_options)

const driver_chr = driver_builder.build();
return driver_chr;
Copy link
Member

Choose a reason for hiding this comment

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

Could just

return driver_builder.build();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was because initially there was even a Firefox option
let driver_fx = new webdriver.Builder() .forBrowser('firefox') .build();

for (let i = maxNumber; i >= 10; i = i - 10) {
maxNumber = i
let col = randomColor(colorsArray)
resultDraw = `color ${col}\n${figure} ${maxNumber}`
Copy link
Member

Choose a reason for hiding this comment

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

Huh?

Are we simply trying to count down from 200 in 10s? If so, this whole maxNumber bit is unnecessary:

for (let i = maxNumber; i >= 10; i -= 10) {
    let col = randomColor(colorsArray)
    resultDraw = `color ${col}\n${figure} ${i}`
    array.push(resultDraw)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

README.md Outdated

Follow this [Setting up your own test automation environment](https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Your_own_automation_environment)

- run `npm install selenium-webdriver` inside your local repo
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be needed - npm install should grab it as it is a dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, i modified the text

];

function randomColor(colorsArray) {
return colorsArray[Math.floor(colorsArray.length * Math.random())]
Copy link
Member

Choose a reason for hiding this comment

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

Looks like 2 space indents have been used for all this code, make it 4 to match the rest of the codebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done it


function drawFigure(colorsArray = colors, figure = 'circle') {
let maxNumber = 200;
let resultDraw = ''
Copy link
Member

Choose a reason for hiding this comment

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

Inconsistency ending lines with ; and not bothering, choose one and stick with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done it

@@ -17,14 +16,16 @@ def __init__(self, load_path='', make=False, play=False):
else:
url = base_url.format(path='')

self._index = url
self._index = 'chromium --app={url} --start-fullscreen'.format(url=url)
Copy link
Member

Choose a reason for hiding this comment

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

Split this out of this PR, we can't merge the tests and benchmarks in with this bit. It could be that you've pulled a commit from your other branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done it

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.