Modified how Travis CI is used with the project. by aaltat · Pull Request #716 · robotframework/SeleniumLibrary

aaltat · 2016-12-17T21:38:45Z

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.

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 · 2016-12-17T21:48:45Z

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

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.

HelioGuilherme66 · 2016-12-19T14:02:38Z

        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 '


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

HelioGuilherme66 · 2016-12-19T14:15:04Z

    packages:
      - google-chrome-stable
+before_install:
+  - wget http://chromedriver.storage.googleapis.com/2.26/chromedriver_linux64.zip


CDVERSION=`curl http://chromedriver.storage.googleapis.com/LATEST_RELEASE` wget http://chromedriver.storage.googleapis.com/$CDVERSION/chromedriver_linux64.zip

HelioGuilherme66 · 2016-12-19T14:18:36Z

+            'Can not run test with browser "{}" from SauceLabs\n'
            'SauceLabs can be used only when running with corn and from '
-            'Selenium2Library master banch'.format(browser)
+            'Selenium2Library master banch, but your event type '


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

HelioGuilherme66 · 2016-12-19T14:27:09Z


  run_tests.py python chrome
-  run_tests.py jython c:\\Python35\\python.exe --test "Click element"
+  run_tests.py jython firefox --suite list


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.

HelioGuilherme66 · 2016-12-19T17:16:56Z

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 · 2016-12-19T17:26:49Z

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

HelioGuilherme66 · 2016-12-19T18:36:19Z

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

aaltat · 2016-12-20T20:50:22Z

Updated run_tests.py to support other browsers that chrome locally

HelioGuilherme66 · 2016-12-21T00:28:17Z

-  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


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

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

HelioGuilherme66 · 2016-12-21T00:30:39Z

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.

pekkaklarck · 2016-12-21T06:22:55Z

+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)


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.

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

pekkaklarck · 2016-12-21T06:26:19Z

+  --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.


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.

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.

pekkaklarck · 2016-12-21T06:27:51Z



-def acceptance_tests(interpreter, browser, options):
+def acceptance_tests(interpreter, browser, suite, sauceusername, saucekey):


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.

pekkaklarck · 2016-12-21T06:30:57Z

+        options.extend(get_sauce_conf(browser, sauceusername, saucekey))
+    command = runner
+    if suite:
+        command += ['-s', suite]


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

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.

pekkaklarck · 2016-12-21T06:33:23Z

-    log_start(command)
+    if sauceusername and saucekey:
+        options.extend(get_sauce_conf(browser, sauceusername, saucekey))
+    command = runner


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

command = runner + (['--suite', suite] if suite else [])

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.

pekkaklarck · 2016-12-21T06:34:33Z



-def log_start(command_list):
+def log_start(command_list, sauceusername, saucekey):


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

pekkaklarck · 2016-12-21T06:35:55Z

-def process_output(browser, cli_options):
+def get_sauce_conf(browser, sauceusername, saucekey):
+    if browser == 'chrome' and env.TRAVIS:
+        conf = []


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

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

pekkaklarck · 2016-12-21T06:37:17Z

    robotstatuschecker.process_output(output, verbose=False)
    options = [opt.format(browser=browser) for opt in REBOT_OPTIONS]
+    if suite:
+        options += ['-s', suite]


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

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

pekkaklarck · 2016-12-21T06:39:00Z

+    else:
+        username = sauceusername
+        key = saucekey
+    return username, key


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?

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

pekkaklarck · 2016-12-21T06:39:43Z

-        sys.exit(__doc__)
+    parser = argparse.ArgumentParser(description='Library test runner')
+    parser.add_argument(
+        '--interpreter',


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

pekkaklarck · 2016-12-21T06:41:30Z

+        help='Selects the test suites by name.'
+    )
+    parser.add_argument(
+        '--scusername',


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

pekkaklarck · 2016-12-21T06:41:58Z

+        help='Username to order browser from SaucuLabs'
+    )
+    parser.add_argument(
+        '--sckey',


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

pekkaklarck · 2016-12-21T06:43:01Z

+            '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)


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

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

pekkaklarck · 2016-12-21T06:44:06Z

+        default='python',
+        help='Interpreter used run the test'
+    )
+    parser.add_argument('browser', help='Browser used in testing')


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.

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

pekkaklarck · 2016-12-21T06:51:15Z


-def execute_tests(interpreter, browser, cli_options):
+def execute_tests(interpreter, browser, suite, sauceusername, saucekey):
    runner = [interpreter, '-m', 'robot.run']


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.

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

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 · 2016-12-21T07:33:29Z

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.

Not it is possible to pass Robot Framework command line arguments as last argument to run_tests.py command.

aaltat · 2016-12-25T21:11:01Z

Did not yet do py -3 because of lack of time. But will do it later.
Added note about the README to the issue Change the Travis to run differently the acceptance test #713
Would like to remove the env.pywith separate PR.

aaltat · 2016-12-28T23:40:30Z

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 · 2017-01-02T21:07:28Z

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 · 2017-01-02T22:56:25Z

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

aaltat · 2017-01-06T18:39:31Z

@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.

pekkaklarck · 2017-01-06T18:53:52Z

 Unit and Acceptance Tests
 -------------------------
+To run tests give command::
+	python test/run_tests.py chomre


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?

pekkaklarck · 2017-01-06T18:54:42Z

-To run just the unit tests, run::
-
-	python test/run_unit_tests.py
+More details in test/README.rst


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

pekkaklarck · 2017-01-06T18:55:13Z

+Before running the test, install the dependencies::
+
+    pip install requirements.txt
+    pip install requirements-dev.txt


Shouldn't there be -r too?

Yes and done

pekkaklarck · 2017-01-06T18:56:00Z

+
+To run unit and acceptance tests, run::
+
+    python test/run_tests.py ff|ie|chrome


Perhaps just test/run_tests.py <browser>?

pekkaklarck · 2017-01-06T18:57:30Z

+    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


I'd remove python and py -3 from the beginning altogether.

Using --interpreter hasn't been explained.

pekkaklarck · 2017-01-06T18:58:08Z

+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


There probably should be an empty row after ::.

pekkaklarck · 2017-01-06T18:59:47Z

+            'run_tests.py --interpreter jython firefox --suite javascript\n'
+            'python run_tests.py chrome --sauceusername your_username '
+            '--saucekey account_key --suite javascript\n'
+            ' '


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

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 · 2017-01-06T19:01:13Z

+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.


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?

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

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.

Also the argperser is also using the module doc string.

aaltat · 2017-01-09T22:02:12Z

Done

HelioGuilherme66 · 2017-01-09T23:09:24Z

+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.


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

HelioGuilherme66 · 2017-01-09T23:12:44Z

+--------------------------------------
+
+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


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

HelioGuilherme66 · 2017-01-09T23:17:25Z

-  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


Same correction as in comment

aaltat added 3 commits December 17, 2016 23:49

Changed selenium 3 run from Python 3 to Python 2.

5ec33ef

Changed chrome browser always to run from Travis

4cf8106

Downloading chrome driver and putting it to path

22b4c55

aaltat mentioned this pull request Dec 18, 2016

Change the Travis to run differently the acceptance test #713

Closed

Allowing Sauce Labs to choose chrome version and the OS

adf2b06

HelioGuilherme66 self-requested a review December 19, 2016 13:54

HelioGuilherme66 suggested changes Dec 19, 2016

View reviewed changes

Updated based on review

a175b0b

HelioGuilherme66 reviewed Dec 21, 2016

View reviewed changes

HelioGuilherme66 approved these changes Dec 21, 2016

View reviewed changes

HelioGuilherme66 mentioned this pull request Dec 21, 2016

Makes unit tests runner more compatible between operating systems #693

Closed

pekkaklarck reviewed Dec 21, 2016

View reviewed changes

Enhanced documation and Robot Framework command line args

5edfe7d

Not it is possible to pass Robot Framework command line arguments as last argument to run_tests.py command.

aaltat mentioned this pull request Dec 29, 2016

Adds Travis configuration for Python 3.4 and 3.5. #657

Closed

HelioGuilherme66 mentioned this pull request Dec 30, 2016

List of PRs to merge for Python 3.x compatibility #658

Closed

8 of 9 tasks

Added support for "py -2" for windows and test/README.rst

60b2cc3

Removed env.py

377adf6

Spell checks performed

50fc8da

pekkaklarck approved these changes Jan 6, 2017

View reviewed changes

Fixed documentation based on comments

2475ba3

pekkaklarck reviewed Jan 8, 2017

View reviewed changes

aaltat added 2 commits January 9, 2017 22:43

Added doc string to the module

6bdbbb0

Also the argperser is also using the module doc string.

Fixed based on comments

fe5e179

HelioGuilherme66 reviewed Jan 9, 2017

View reviewed changes

Fixed based on comments

f19b194

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

aaltat deleted the fix_ci branch January 24, 2017 22:12



		def acceptance_tests(interpreter, browser, options):
		def acceptance_tests(interpreter, browser, suite, sauceusername, saucekey):



		def log_start(command_list):
		def log_start(command_list, sauceusername, saucekey):


		To run unit and acceptance tests, run::

		python test/run_tests.py ff\|ie\|chrome

Conversation

aaltat commented Dec 17, 2016

Uh oh!

aaltat commented Dec 17, 2016

Uh oh!

HelioGuilherme66 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HelioGuilherme66 commented Dec 19, 2016

Uh oh!

aaltat commented Dec 19, 2016

Uh oh!

HelioGuilherme66 commented Dec 19, 2016

Uh oh!

aaltat commented Dec 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HelioGuilherme66 commented Dec 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pekkaklarck Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pekkaklarck Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pekkaklarck Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaltat Dec 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pekkaklarck Dec 21, 2016 •

edited

Loading

pekkaklarck Dec 21, 2016 •

edited

Loading

pekkaklarck Dec 21, 2016 •

edited

Loading

aaltat Dec 25, 2016 •

edited

Loading