Modified how Travis CI is used with the project. #716

Merged
merged 14 commits into from Jan 11, 2017

Projects

None yet

3 participants

@aaltat
Contributor
aaltat commented Dec 17, 2016

Travis does not allow, for securirty reasons, PR buils to access
encrypted data. Project stores the Sacuce Labs credentials in
encrypted format in Travis settings and therefore builds from PR can
not use the browsers from Sauce Labs. Changed CI configuration and
run_tests.py to use Trvis Chrome browser for PR buils. Also configured
Travis corn to test from master periodically and to use different
browsers from the Sauce Labs.

Did take Python argparse in use and therefore passing arbitary
pybot and rebot arguments is not anymore possible. But now it is
possible also to use Sauce Labs from when running test from
local computer.

@aaltat aaltat Modified how Travis CI is used with the project.
Travis does not allow, for securirty reasons, PR buils to access
encrypted data. Project stores the Sacuce Labs credentials in
encrypted format in Travis settings and therefore builds from PR can
not use the browsers from Sauce Labs. Changed CI configuration and
run_tests.py to use Trvis Chrome browser for PR buils. Also configured
Travis corn to test from master periodically and to use different
browsers from the Sauce Labs.

Did take Python argparse in use and therefore passing arbitary
pybot and rebot arguments is not anymore possible. But now it is
possible also to use Sauce Labs from when running test from
local computer.
25bd96e
@aaltat
Contributor
aaltat commented Dec 17, 2016

Perhaps for time being we should test selenium 3 with Python 2. Because we are not yet Python 3 compatible and therefore the build for selenium 3 fails.

@HelioGuilherme66 HelioGuilherme66 self-requested a review Dec 19, 2016
@HelioGuilherme66

The major issue is with run_tests.py.
I'll do more testing on it.
If possible amend the text for PR there are some typos.

test/run_tests.py
Examples:
run_tests.py python chrome
- run_tests.py jython c:\\Python35\\python.exe --test "Click element"
+ run_tests.py jython firefox --suite list
@HelioGuilherme66
HelioGuilherme66 Dec 19, 2016 Contributor

There was a change in the argument passing (mentioned on doc), but the actual code does not look like it has been changed.
Example should be:
run_tests.py --interpreter jython firefox --suite list

There are more fixes like on some typos. I'll stop the review here, but will test this file run_tests.py for better review.

@aaltat
aaltat Dec 20, 2016 Contributor

Done

test/run_tests.py
print(
- 'Can not run test with browser {} from SauceLabs\n'
+ 'Can not run test with browser "{}" from SauceLabs\n'
'SauceLabs can be used only when running with corn and from '
@HelioGuilherme66
HelioGuilherme66 Dec 19, 2016 Contributor

Typo. it should be:
'SauceLabs can be used only when running with cron and from '

@aaltat
aaltat Dec 20, 2016 Contributor

Done

test/run_tests.py
'SauceLabs can be used only when running with corn and from '
- 'Selenium2Library master banch'.format(browser)
+ 'Selenium2Library master banch, but your event type '
@HelioGuilherme66
HelioGuilherme66 Dec 19, 2016 Contributor

Typo. it should be:
'Selenium2Library master branch, but your event type '

@aaltat
aaltat Dec 20, 2016 Contributor

Done

.travis.yml
@@ -7,6 +7,11 @@ addons:
- google-chrome
packages:
- google-chrome-stable
+before_install:
+ - wget http://chromedriver.storage.googleapis.com/2.26/chromedriver_linux64.zip
@HelioGuilherme66
HelioGuilherme66 Dec 19, 2016 Contributor
CDVERSION=`curl http://chromedriver.storage.googleapis.com/LATEST_RELEASE`
wget http://chromedriver.storage.googleapis.com/$CDVERSION/chromedriver_linux64.zip
@aaltat
aaltat Dec 20, 2016 Contributor

Done

@HelioGuilherme66
Contributor

The way it is coded, we can never run tests locally with Firefox, it aborts execution by not being allowed to run in Sauce Labs.

@aaltat
Contributor
aaltat commented Dec 19, 2016

Ah, true. I know how to fix it. Thanks for pointing it out

@HelioGuilherme66
Contributor

I now created a PR in @aaltat branch with my proposals 1.

@aaltat
Contributor
aaltat commented Dec 20, 2016

Updated run_tests.py to support other browsers that chrome locally

test/run_tests.py
- run_tests.py jython firefox --suite list
- run_tests.py python chrome --scusername your_username --sckey account_key
+ run_tests.py chrome
+ run_tests.py --suite list --interpreter jython firefox
@HelioGuilherme66
HelioGuilherme66 Dec 21, 2016 Contributor

Better to have --suite at end because is a list.
run_tests.py --interpreter jython firefox --suite list

@aaltat
aaltat Dec 25, 2016 Contributor

The list was a bad example, because it refers to list.robot file and not to Python list. Changed to example to javascript

@HelioGuilherme66
Contributor

I still think you could merge my PR in your fork. There are some other minor improvements.
If you merge this one and #720 we will have green builds on Travis.

test/env.py
+TRAVIS_EVENT_TYPE = os.environ.get("TRAVIS_EVENT_TYPE", None)
+TRAVIS_JOB_NUMBER = os.environ.get("TRAVIS_JOB_NUMBER", "localtunnel")
+SAUCE_USERNAME = os.environ.get("SAUCE_USERNAME", None)
+SAUCE_ACCESS_KEY = os.environ.get("SAUCE_ACCESS_KEY", None)
@pekkaklarck
pekkaklarck Dec 21, 2016 Member

If these are only used by run_tests.py, we could consider moving all this there and could probably remove the whole env.py. Not really related to this PR, though.

@aaltat
aaltat Dec 25, 2016 Contributor

I agree. But the PR is getting big and perhaps doing it in a separate PR is better and cleaner.

test/run_tests.py
+ --suite: Selects the test suites by name.
+ --scusername: Username to access Sauce Labs to order browsers when
+ running test from local computer.
+ --sckey: Access key for Sauce Labs account.
@pekkaklarck
pekkaklarck Dec 21, 2016 edited Member

Doesn't argparse provide help text? No need to have it here in that case. The whole module docstring could be just a simple description of the script, possibly with few examples, that could then be passed to argparse as the general description. More comprehensive docs could then be moved to README.(rst|md) in this directory.

@aaltat
aaltat Dec 21, 2016 Contributor

Yes it does and it's actually quite good help system. Dividing this with readme file and argparse is good idea. At least the argparse help must be enhanced.

@aaltat
aaltat Dec 25, 2016 Contributor

Done

test/run_tests.py
@@ -69,12 +85,12 @@ def unit_tests():
sys.exit(failures)
-def acceptance_tests(interpreter, browser, options):
+def acceptance_tests(interpreter, browser, suite, sauceusername, saucekey):
@pekkaklarck
pekkaklarck Dec 21, 2016 edited Member

As a style comment, I'd prefer sauce_username and sauce_key as argument names. If they have default values like None, it would be good to add them to the signature.

@aaltat
aaltat Dec 25, 2016 Contributor

Done

test/run_tests.py
+ options.extend(get_sauce_conf(browser, sauceusername, saucekey))
+ command = runner
+ if suite:
+ command += ['-s', suite]
@pekkaklarck
pekkaklarck Dec 21, 2016 edited Member

'--suite' would be a little more explicit than '-s'.

@aaltat
aaltat Dec 25, 2016 edited Contributor

Moved to use https://docs.python.org/2.7/library/argparse.html#partial-parsing which allows to keep the old functional to give random Robot Framework arguments from command line. Therefore this comment is not anymore valid one.

test/run_tests.py
- log_start(command)
+ if sauceusername and saucekey:
+ options.extend(get_sauce_conf(browser, sauceusername, saucekey))
+ command = runner
@pekkaklarck
pekkaklarck Dec 21, 2016 Member

This looks strange. Could either just rename runner to command originally or use something like

command = runner + (['--suite', suite] if suite else [])
@aaltat
aaltat Dec 25, 2016 Contributor

Moved to use https://docs.python.org/2.7/library/argparse.html#partial-parsing which allows to keep the old functional to give random Robot Framework arguments from command line. Therefore this comment is not anymore valid one.

test/run_tests.py
-def log_start(command_list):
+def log_start(command_list, sauceusername, saucekey):
@pekkaklarck
pekkaklarck Dec 21, 2016 Member

Could just have the signature as command_list, *hidden and then later for item in hidden.

@aaltat
aaltat Dec 25, 2016 Contributor

Done

test/run_tests.py
-def process_output(browser, cli_options):
+def get_sauce_conf(browser, sauceusername, saucekey):
+ if browser == 'chrome' and env.TRAVIS:
+ conf = []
@pekkaklarck
pekkaklarck Dec 21, 2016 Member

Why not just return []? Would avoid the big else block below.

@aaltat
aaltat Dec 25, 2016 Contributor

Hmm, I am usually on favor for single return statement for each method. But perhaps this is clearer.

test/run_tests.py
print('Verifying results...')
output = os.path.join(env.RESULTS_DIR, 'output.xml')
robotstatuschecker.process_output(output, verbose=False)
options = [opt.format(browser=browser) for opt in REBOT_OPTIONS]
+ if suite:
+ options += ['-s', suite]
@pekkaklarck
pekkaklarck Dec 21, 2016 Member

This shouldn't be needed at all. Enough to filter based on suite when starting execution.

@aaltat
aaltat Dec 25, 2016 Contributor

This might still valid, but the code has changed quite a look. Please take a new look and send comment if something is needed.

test/run_tests.py
+ else:
+ username = sauceusername
+ key = saucekey
+ return username, key
@pekkaklarck
pekkaklarck Dec 21, 2016 Member

Getting configs from env and from the command line is really confusing. Also, wouldn't it be better for command line options to have higher precedence if given?

@aaltat
aaltat Dec 25, 2016 Contributor

Agree. Made a temporally change and will be fixed better when removing env.py file in separate fix.

test/run_tests.py
- sys.exit(__doc__)
+ parser = argparse.ArgumentParser(description='Library test runner')
+ parser.add_argument(
+ '--interpreter',
@pekkaklarck
pekkaklarck Dec 21, 2016 Member

Should add a short option like -I for such a commonly needed option.

@aaltat
aaltat Dec 25, 2016 Contributor

Done

test/run_tests.py
+ help='Selects the test suites by name.'
+ )
+ parser.add_argument(
+ '--scusername',
@pekkaklarck
pekkaklarck Dec 21, 2016 Member

--sauceusername would be a lot better name. Could add a short option like -U if needed.

@aaltat
aaltat Dec 25, 2016 Contributor

Done

test/run_tests.py
+ help='Username to order browser from SaucuLabs'
+ )
+ parser.add_argument(
+ '--sckey',
@pekkaklarck
pekkaklarck Dec 21, 2016 Member

--saucekey optionally with a short option like '-K` would be better.

@aaltat
aaltat Dec 25, 2016 Contributor

Done

test/run_tests.py
+ 'Can not run test with browser "{}" from SauceLabs\n'
+ 'SauceLabs can be used only when running with cron and from '
+ 'Selenium2Library master branch, but your event type '
+ 'was "{}"'.format(browser, env.TRAVIS_EVENT_TYPE)
@pekkaklarck
pekkaklarck Dec 21, 2016 Member

Apparently only Chrome is supported here? Error should mention that too.

@aaltat
aaltat Dec 25, 2016 Contributor

Yes, when running tests with Travis against PR, then it is only possible to use Chrome. Improved text.

test/run_tests.py
+ default='python',
+ help='Interpreter used run the test'
+ )
+ parser.add_argument('browser', help='Browser used in testing')
@pekkaklarck
pekkaklarck Dec 21, 2016 Member

If we only can run tests on Travis with Chrome, having that as the default browser would probably be a good idea. Could then make this an option like --browser with a short option -B.

@aaltat
aaltat Dec 25, 2016 Contributor

I kind a liked to this way. For me it makes the travis matrix easier to read. Also it saves few letter for typing, now it can be used with:
run_tests.py chrome
instead of:
run_tests.py -B chrome

test/run_tests.py
@@ -96,68 +112,110 @@ def http_server():
serverlog.close()
-def execute_tests(interpreter, browser, cli_options):
+def execute_tests(interpreter, browser, suite, sauceusername, saucekey):
runner = [interpreter, '-m', 'robot.run']
@pekkaklarck
pekkaklarck Dec 21, 2016 Member

We should support also interpreter with spaces like py -3 that's very handy on Windows. I guess that can already be given from the command line like --interpreter "py -3" but this code won't handle it. Could just use interpreter.split() to split the command. shlex.split(interpreter) would be safer but I doubt we need it here.

@aaltat
aaltat Dec 25, 2016 Contributor

Not done. I agree that this is useful but did not have time to code this in today.

@aaltat
aaltat Jan 2, 2017 Contributor

Done

@pekkaklarck

Code changes look good to me in general. I added some comments mainly related to the style. Few things I'd do related to this that aren't necessarily in the scope of this PR:

  • Get rid off the env.py script entirely. Would be much cleaner to handle all configuration in runner scripts themselves. I doubt there's much acceptance and unit test runners share anyway.

  • Add README.(rst|md) into the test directory.

  • Handle interpreter like py -3 (added separate comment about this).

@aaltat
Contributor
aaltat commented Dec 21, 2016

I am busy with Xmas preparation, so making the changes will go after Xmas.

Thanks from the comments (in a good way and not in a Finnish sartastic way) and happy holidays to all.

@aaltat aaltat Enhanced documation and Robot Framework command line args
Not it is possible to pass Robot Framework command line
arguments as last argument to run_tests.py command.
5edfe7d
@aaltat
Contributor
aaltat commented Dec 25, 2016
  • Did not yet do py -3 because of lack of time. But will do it later.
  • Added note about the README to the issue #713
  • Would like to remove the env.pywith separate PR.
@aaltat
Contributor
aaltat commented Dec 28, 2016

But as @HelioGuilherme66 pointed out and was reminded by this robotframework/robotframework#2502, should we minimum supported Python 3 version by Robot Framework (Python 3.3), should we use something else or should we always live with the latest Python 3 available from Travis? Which, according to Travis documentation is is Python 3.5 (there is some support for Python 3.6 nightly builds, according the documentation)?

@aaltat
Contributor
aaltat commented Jan 2, 2017 edited

Added test/README.rst and fixed --interpreter "py -2"problem.

BUILD.rst needs more work but perhaps in separate PR like the env.py file.

@aaltat
Contributor
aaltat commented Jan 2, 2017

Decided to remove env.py because it was simple operation.

@aaltat
Contributor
aaltat commented Jan 6, 2017

@pekkaklarck would you have time to take a look?

@pekkaklarck

Looks good to me in general. I mainly briefly looked at the docs, though. Check my comments and merge when you think this is ready.

BUILD.rst
@@ -26,44 +26,10 @@ test/
Unit and Acceptance Tests
-------------------------
+To run tests give command::
+ python test/run_tests.py chomre
@pekkaklarck
pekkaklarck Jan 6, 2017 Member
  • chomre -> chrome
  • If run_tests.py is executable, could just use test/run_tests.py chrome
  • Should it rather be something generic like <browser> instead of chrome?
@aaltat
aaltat Jan 7, 2017 Contributor

Done

BUILD.rst
-To run just the unit tests, run::
-
- python test/run_unit_tests.py
+More details in test/README.rst
@pekkaklarck
pekkaklarck Jan 6, 2017 Member

Formatted like <test/README.rst>_ this would create a link to the README file when rendered as HTML on GitHub.

@aaltat
aaltat Jan 7, 2017 Contributor

Done

test/README.rst
+Before running the test, install the dependencies::
+
+ pip install requirements.txt
+ pip install requirements-dev.txt
@pekkaklarck
pekkaklarck Jan 6, 2017 Member

Shouldn't there be -r too?

@aaltat
aaltat Jan 7, 2017 Contributor

Yes and done

test/README.rst
+
+To run unit and acceptance tests, run::
+
+ python test/run_tests.py ff|ie|chrome
@pekkaklarck
pekkaklarck Jan 6, 2017 Member

Perhaps just test/run_tests.py <browser>?

@aaltat
aaltat Jan 7, 2017 Contributor

Done

test/README.rst
+It is possible to pass Robot Framework command line arguments to the test
+execution as a last arguments to the `run_tests.py` command. Example arguments
+like --test, --suite, --include and --exclude may be used to configure
+acceptance test execution.
@pekkaklarck
pekkaklarck Jan 6, 2017 Member

Do all RF's options work or can you only use some? Earlier versions had a restriction to use only some but is that fixed?

@aaltat
aaltat Jan 7, 2017 Contributor

In this version all goes, but that might not be always a good idea. Enhanced documentation to recommend few arguments.

test/README.rst
+ python run_tests.py chrome
+ python run_tests.py --interpreter jython firefox --suite javascript
+ python run_tests.py chrome --sauceusername your_username --saucekey account_key --suite javascript
+ py -3 run_tests.py --interpreter "py -2" chrome --suite javascript
@pekkaklarck
pekkaklarck Jan 6, 2017 Member
  • I'd remove python and py -3 from the beginning altogether.
  • Using --interpreter hasn't been explained.
@aaltat
aaltat Jan 7, 2017 Contributor

Done

test/README.rst
+reach the acceptance test web server. The acceptance test uses tunnel with
+name `localtunnel` and therefore when establishing the Sauce Connect tunnel
+use the following command::
+ bin/sc -u YOUR_USERNAME -k YOUR_ACCESS_KEY -i localtunnel
@pekkaklarck
pekkaklarck Jan 6, 2017 Member

There probably should be an empty row after ::.

@aaltat
aaltat Jan 7, 2017 Contributor

Done

test/run_tests.py
+ 'run_tests.py --interpreter jython firefox --suite javascript\n'
+ 'python run_tests.py chrome --sauceusername your_username '
+ '--saucekey account_key --suite javascript\n'
+ ' '
@pekkaklarck
pekkaklarck Jan 6, 2017 Member

Should definitely format this using Python's triplequoted strings. Alternatively could use __doc__ here.

@aaltat
aaltat Jan 7, 2017 Contributor

The argparse is making some things hard to present, but perhaps this is more clearer way. Although I do not like the back slash as line continuation, but it is the lesser evil this time.

@pekkaklarck

I'd add module docstring to run_tests.py and then pass description and epilog to argparse as __doc__.spilitlines()[0] and '\n'.join(__doc__.splitlines()[2:]), respectively. Scripts having docstring is generally nice and it would be handy to reuse it with argparse.

@aaltat aaltat Added doc string to the module
Also the argperser is also using the module doc string.
6bdbbb0
@pekkaklarck

I wouldn't split a line like this. Either just leave the line long or make it shorter by shortening values. Perhaps something like:

run_tests.py ie --sauceusername <user> --saucekey <key> --suite javascript
@pekkaklarck

Empty line missing before import. #nitpick

@aaltat
Contributor
aaltat commented Jan 9, 2017

Done

test/README.rst
+argument.
+
+Starting from version 2.0 onwards the Selenium2Library is tested by using
+Python 2 and 3. Other interpreters are not test by the development team.
@HelioGuilherme66
HelioGuilherme66 Jan 9, 2017 Contributor

Verb and not name. It should be: "Other interpreters are not tested by the development team."

@aaltat
aaltat Jan 10, 2017 Contributor

Done

test/README.rst
+--------------------------------------
+
+It is possible to pass Robot Framework command line arguments to the test
+execution as a last arguments to the `run_tests.py` command. It is recommended
@HelioGuilherme66
HelioGuilherme66 Jan 9, 2017 Contributor

Remove single article. It should be: "... execution as a last arguments to the run_tests.py command."

@aaltat
aaltat Jan 10, 2017 Contributor

Done

test/run_tests.py
- browser: Any browser supported by the library (e.g. `chrome`, `firefox`)
- options: Additional command line options passed to Robot Framework
+It is possible to pass Robot Framework command line arguments to the test
+execution as a last arguments to the `run_tests.py` command. It is
@HelioGuilherme66
HelioGuilherme66 Jan 9, 2017 Contributor

Same correction as in comment

@aaltat
aaltat Jan 10, 2017 Contributor

Done

@aaltat aaltat merged commit 0b545d1 into robotframework:master Jan 11, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment