All of lore.kernel.org
 help / color / mirror / Atom feed
* [Fuego] [PATCH 00/16] Fuego Release Test
@ 2018-03-29  0:08 Guilherme Campos Camargo
  2018-03-29  0:08 ` [Fuego] [PATCH 01/16] Add fuego-release Functional test Guilherme Campos Camargo
                   ` (16 more replies)
  0 siblings, 17 replies; 40+ messages in thread
From: Guilherme Campos Camargo @ 2018-03-29  0:08 UTC (permalink / raw)
  To: fuego

Hello, everyone

This series of patches contains the initial implementation of the Fuego
Release Test (Functional.fuegotest). This test has been created to allow
Fuego to test any version of Fuego, given its git repository and branch.

The test clones the repositories of fuego and fuego-core that are
configured on the test specs.json, installs fuego and runs that version
of fuego as the docker container fuego-release-container.

The test uses Pexpect, to execute/verify commands in the
fuego-release-container shell, and uses SeleniumHQ to interact with the
Jenkins web interface, checking if the web interface responds as
expected.

A few wrappers have been implemented on top of Pexpect and SeleniumHQ in
order to facilitate the inclusion of new tests.

The default specs.json is currently configured to clone and test fuego
and fuego-core from the master branch of the official repository. What
can be changed through the specs.json file.

# Running

Currently this test requires a modified version of Fuego to be executed,
given that it needs to install some dependencies and needs to map the
dockerd socket from the host to the fuego container.

The modified version can be found in two different branches on
Profusion's fuego fork.

 1 - Branch fuego-test: Just a few commits that are necessary for making
 this test work, applied on top of fuego/next. We plan to try to
 integrate these commits into fuego/next in the next few days.

 2 - Branch fuego-base-image: A more complex change on fuego, that makes
 the necessary changes for allowing it to be shipped as a docker image
 through dockerhub.

The steps for each one of the versions above are given below:

## Building the image (from the branch fuego-test)

To run the test, execute the following commands:

```
git clone --branch fuego-test https://bitbucket.org/profusionmobi/fuego-core.git
git clone --branch fuego-test https://bitbucket.org/profusionmobi/fuego.git
cd fuego
./install fuego-to-test-fuego
./fuego-host-scripts/docker-create-container.sh fuego-to-test-fuego fuego-to-test-fuego-container
./fuego-host-scripts/docker-start-container.sh fuego-to-test-fuego-container
```

Then, add the fuego-test board and the Functional.fuegotest and start
the test through Jenkins (localhost:8080/fuego/)

```
ftc add-nodes fuego-test
ftc add-jobs -b fuego-test -t Functional.fuegotest
```

## Using the modified Fuego Base Image from Dockerhub
(fuego-base-image):

You can also use the fuego base image that's being developed in
Profusion's fuego-base-image branch in our fork:
https://bitbucket.org/profusionmobi/fuego/branch/fuego-base-image

The image is already available on dockerhub and can be
downloaded/executed with:

```
docker pull fuegotest/fuego
docker run -it \
  -p 8080:8080 \
  -v $(pwd)/host_fuego_home:/var/fuego_home \
  -e JENKINS_UID=$(id -u) \
  -e JENKINS_GID=$(id -g) \
  -v /var/run/docker.sock:/var/run/docker.sock \
  fuegotest/fuego:latest
```

Wait for the shell to be available and add fuego-test board and
Functional.fuegotest as explained in the last section.

```
ftc add-nodes fuego-test
ftc add-jobs -b fuego-test -t Functional.fuegotest
```

Thanks

Guilherme Campos Camargo (16):
  Add fuego-release Functional test
  Mount fuego-rw/ro/core into the fuego-under-test container
  Increase wait_for_jenkins timeout from 10 to 60s
  Print fuego repo/branch information during test build
  Properly check install return code and abort in case of failure
  Allow the user to keep the container running after the test
  Add Back() Selenium Command
  Add ClickLink selenium command
  Include add-jobs, add-views and build-jobs tests
  Write ok/fail on test report
  Properly quit Selenium driver when SeleniumSession is deleted
  Minor style/PEP8 fixes
  Move Selenium implicitly_wait() to SeleniumSession start
  Use a fixed language for Selenium Chrome-WebDriver
  Improve Click and Check methods to allow multiple locators
  Add test that starts a job through the UI

 engine/tests/Functional.fuegotest/fuego_test.sh |  36 ++
 engine/tests/Functional.fuegotest/spec.json     |  11 +
 engine/tests/Functional.fuegotest/test_run.py   | 591 ++++++++++++++++++++++++
 3 files changed, 638 insertions(+)
 create mode 100755 engine/tests/Functional.fuegotest/fuego_test.sh
 create mode 100644 engine/tests/Functional.fuegotest/spec.json
 create mode 100755 engine/tests/Functional.fuegotest/test_run.py

-- 
2.16.2


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [Fuego] [PATCH 01/16] Add fuego-release Functional test
  2018-03-29  0:08 [Fuego] [PATCH 00/16] Fuego Release Test Guilherme Campos Camargo
@ 2018-03-29  0:08 ` Guilherme Campos Camargo
  2018-03-30 22:17   ` Tim.Bird
  2018-03-29  0:08 ` [Fuego] [PATCH 02/16] Mount fuego-rw/ro/core into the fuego-under-test container Guilherme Campos Camargo
                   ` (15 subsequent siblings)
  16 siblings, 1 reply; 40+ messages in thread
From: Guilherme Campos Camargo @ 2018-03-29  0:08 UTC (permalink / raw)
  To: fuego

Allows Fuego to test new Fuego releases.

The fuego and fuego-core repositores and branches to be tested may be
configured on the specs.json file. Their defaults are:

"FUEGOREPO":"https://bitbucket.org/tbird20d/fuego",
"FUEGOBRANCH":"master",
"FUEGOCOREREPO":"https://bitbucket.org/tbird20d/fuego-core",
"FUEGOCOREBRANCH":"master"

Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
---
 engine/tests/Functional.fuegotest/fuego_test.sh |  29 ++
 engine/tests/Functional.fuegotest/spec.json     |  11 +
 engine/tests/Functional.fuegotest/test_run.py   | 427 ++++++++++++++++++++++++
 3 files changed, 467 insertions(+)
 create mode 100755 engine/tests/Functional.fuegotest/fuego_test.sh
 create mode 100644 engine/tests/Functional.fuegotest/spec.json
 create mode 100755 engine/tests/Functional.fuegotest/test_run.py

diff --git a/engine/tests/Functional.fuegotest/fuego_test.sh b/engine/tests/Functional.fuegotest/fuego_test.sh
new file mode 100755
index 0000000..192c15b
--- /dev/null
+++ b/engine/tests/Functional.fuegotest/fuego_test.sh
@@ -0,0 +1,29 @@
+#!/bin/bash
+
+set -e
+
+readonly fuego_release_dir=fuego-release
+
+function test_build {
+    if [ -d ${fuego_release_dir} ]; then
+        rm -r ${fuego_release_dir}
+    fi
+    git clone --quiet --depth 1 --single-branch \
+        --branch "${FUNCTIONAL_FUEGOTEST_FUEGOBRANCH}" \
+        "${FUNCTIONAL_FUEGOTEST_FUEGOREPO}" \
+        "${fuego_release_dir}/fuego"
+    git clone --quiet --depth 1 --single-branch \
+        --branch "${FUNCTIONAL_FUEGOTEST_FUEGOCOREBRANCH}" \
+        "${FUNCTIONAL_FUEGOTEST_FUEGOCOREREPO}" \
+        "${fuego_release_dir}/fuego-core"
+    cd -
+}
+
+function test_run {
+    sudo -n ${TEST_HOME}/test_run.py "${fuego_release_dir}/fuego"
+    report "echo ok 1 minimal test on target"
+}
+
+function test_processing {
+    log_compare "$TESTDIR" "1" "^ok" "p"
+}
diff --git a/engine/tests/Functional.fuegotest/spec.json b/engine/tests/Functional.fuegotest/spec.json
new file mode 100644
index 0000000..cc345ba
--- /dev/null
+++ b/engine/tests/Functional.fuegotest/spec.json
@@ -0,0 +1,11 @@
+{
+    "testName": "Functional.fuegotest",
+    "specs": {
+        "default": {
+            "FUEGOREPO":"https://bitbucket.org/tbird20d/fuego",
+            "FUEGOBRANCH":"master",
+            "FUEGOCOREREPO":"https://bitbucket.org/tbird20d/fuego-core",
+            "FUEGOCOREBRANCH":"master"
+        }
+    }
+}
diff --git a/engine/tests/Functional.fuegotest/test_run.py b/engine/tests/Functional.fuegotest/test_run.py
new file mode 100755
index 0000000..96fcfb7
--- /dev/null
+++ b/engine/tests/Functional.fuegotest/test_run.py
@@ -0,0 +1,427 @@
+#!/usr/bin/env python3
+import argparse
+import http
+import http.client
+import logging
+import os
+import re
+import subprocess
+import sys
+import time
+
+import docker
+import pexpect
+import requests
+from selenium import webdriver
+
+LOGGER = logging.getLogger('test_run')
+STREAM_HANDLER = logging.StreamHandler()
+STREAM_HANDLER.setFormatter(
+    logging.Formatter('%(name)s:%(levelname)s: %(message)s'))
+LOGGER.setLevel(logging.DEBUG)
+LOGGER.addHandler(STREAM_HANDLER)
+
+
+def loop_until_timeout(func, timeout=10, num_tries=5):
+    LOGGER.debug('Running %s()', func.__name__)
+
+    for _ in range(num_tries):
+        LOGGER.debug('  Try number %s...', _ + 1)
+        if func():
+            LOGGER.debug('  Success')
+            return True
+        time.sleep(timeout/num_tries)
+    LOGGER.debug('  Failure')
+
+    return False
+
+
+class SeleniumCommand:
+    def exec(self, selenium_ctx):
+        self.driver = selenium_ctx.driver
+        self.driver.refresh()
+        self.driver.implicitly_wait(3)
+        LOGGER.debug('Executing Selenium Command \'%s\'',
+                     self.__class__.__name__)
+
+
+class Visit(SeleniumCommand):
+    def __init__(self, url, timeout=10, expected_result=200):
+        self.url = url
+        self.timeout = timeout
+        self.expected_result = expected_result
+
+    def exec(self, selenium_ctx):
+        super().exec(selenium_ctx)
+
+        LOGGER.debug('  Visiting \'%s\'', self.url)
+        self.driver.get(self.url)
+
+        r = requests.get(self.url)
+        if r.status_code != self.expected_result:
+            LOGGER.debug('  HTTP Status Code \'%s\' is different '
+                         'from the expected \'%s\'', r.status_cod, self.url)
+            return False
+
+        LOGGER.debug('  HTTP Status Code is same as expected \'%s\'',
+                     r.status_code)
+        return True
+
+
+class CheckText(SeleniumCommand):
+    def __init__(self, _id, text, expected_result=True):
+        self._id = _id
+        self.text = text
+        self.expected_result = expected_result
+
+    def exec(self, selenium_ctx):
+        super().exec(selenium_ctx)
+
+        try:
+            text = self.driver.find_element_by_id(self._id).text
+        except Exception:  # TODO: Use proper Exception
+            return False
+
+        LOGGER.debug(
+            '  Searching for \'%s\' in \'id:%s\'', self.text, self._id)
+
+        result = True
+        if self.text not in text:
+            LOGGER.error(
+                '  \'%s\' not found in id \'%s\' with text \'%s\'', self.text,
+                self._id, text)
+            result = False
+
+        LOGGER.debug('  \'%s\' was found', self.text)
+
+        return result == self.expected_result
+
+
+class ShExpect():
+    BASH_PATTERN = 'test_run_pr1:#'
+    COMMAND_RESULT_PATTERN = re.compile('^([0-9]+)', re.M)
+    OUTPUT_VARIABLE = 'cmd_output'
+    COMMAND_OUTPUT_DELIM = ':test_run_cmd_out:'
+    COMMAND_OUTPUT_PATTERN = re.compile(
+        r'^{0}(.*){0}\s+{1}'.format(
+            COMMAND_OUTPUT_DELIM, BASH_PATTERN), re.M | re.S)
+
+    def __init__(self, cmd, expected_output=None, expected_result=0):
+        self.cmd = cmd
+        self.expected_result = expected_result
+        self.expected_output = expected_output
+
+    def exec(self, pexpect_ctx):
+        self.client = pexpect_ctx.client
+
+        LOGGER.debug('Executing command \'%s\'', self.cmd)
+        try:
+            self.client.sendline('{}=$({} 2>&1)'.format(
+                self.OUTPUT_VARIABLE, self.cmd))
+            self.client.expect(self.BASH_PATTERN)
+
+            self.client.sendline('echo $?')
+            self.client.expect(self.COMMAND_RESULT_PATTERN)
+            result = int(self.client.match.group(1))
+
+            self.client.sendline(
+                'echo "{0}${{{1}}}{0}"'.format(
+                    self.COMMAND_OUTPUT_DELIM,
+                    self.OUTPUT_VARIABLE))
+            self.client.expect(self.COMMAND_OUTPUT_PATTERN)
+            out = self.client.match.group(1)
+
+            if result != self.expected_result:
+                LOGGER.error('The command \'%s\' returned the code \'%d\', '
+                             'but the expected code is \'%d\''
+                             '\nCommand output: \'%s\'',
+                             self.cmd, result, self.expected_result, out)
+                return False
+            if self.expected_output is not None and \
+                    re.search(self.expected_output, out) is None:
+                LOGGER.error('Wrong output for command \'%s\'. '
+                             'Expected \'%s\'\nReceived \'%s\'',
+                             self.cmd, self.expected_output, out)
+                return False
+        except pexpect.exceptions.TIMEOUT:
+            LOGGER.error('Timeout for command \'%s\'', self.cmd)
+            return False
+        except pexpect.exceptions.EOF:
+            LOGGER.error('Lost connection with docker. Aborting')
+            return False
+        return True
+
+
+class FuegoContainer:
+    def __init__(self, install_script, image_name, container_name,
+                 jenkins_port):
+        self.install_script = install_script
+        self.image_name = image_name
+        self.container_name = container_name
+        self.jenkins_port = jenkins_port
+
+        self.docker_client = docker.APIClient()
+        self.container = self.setup_docker()
+
+    def __del__(self):
+        if self.container:
+            LOGGER.debug('Removing Container')
+            self.container.remove(force=True)
+
+    def stop(self):
+        self.container.remove(force=True)
+        self.container = None
+
+    def setup_docker(self):
+        cmd = './{} {}'.format(self.install_script, self.image_name)
+        LOGGER.debug('Running \'%s\' to install the docker image. '
+                     'This may take a while....', cmd)
+        status = subprocess.call(cmd, shell=True)
+        if status != 0:
+            return None
+        docker_client = docker.from_env()
+        containers = docker_client.containers.list(
+            all=True, filters={'name': self.container_name})
+        if containers:
+            LOGGER.debug(
+                'Erasing the container \'%s\', so a new one can be created',
+                self.container_name)
+            containers[0].remove(force=True)
+
+        container = docker_client.containers.create(
+            self.image_name,
+            stdin_open=True, tty=True, network_mode='bridge',
+            name=self.container_name, command='/bin/bash')
+        LOGGER.debug('Container \'%s\' created', self.container_name)
+        return container
+
+    def is_running(self):
+        try:
+            container_status = self.docker_client.\
+                inspect_container(self.container_name)['State']['Running']
+        except KeyError:
+            return False
+
+        return container_status
+
+    def get_ip(self):
+        container_addr = None
+
+        if not self.is_running():
+            return None
+
+        def fetch_ip():
+            nonlocal container_addr
+            try:
+                container_addr = self.docker_client.\
+                    inspect_container(
+                        self.container_name)['NetworkSettings']['IPAddress']
+            except KeyError:
+                return False
+
+            return False if container_addr is None else True
+
+        if not loop_until_timeout(fetch_ip, timeout=10):
+            LOGGER.error('Could not fetch the container IP address')
+            return None
+
+        return container_addr
+
+    def get_url(self):
+        container_addr = self.get_ip()
+
+        if container_addr:
+            return 'http://{}:{}/fuego/'.\
+                format(container_addr, self.jenkins_port)
+        else:
+            return None
+
+
+class PexpectContainerSession():
+    def __init__(self, container, start_script, timeout):
+        self.container = container
+        self.start_script = start_script
+        self.timeout = timeout
+
+    def start(self):
+        LOGGER.debug(
+            'Starting container \'%s\'', self.container.container_name)
+        self.client = pexpect.spawnu(
+            '{} {}'.format(
+                self.start_script, self.container.container_name),
+            echo=False, timeout=self.timeout)
+
+        PexpectContainerSession.set_ps1(self.client)
+
+        if not self.wait_for_jenkins():
+            return False
+
+        return True
+
+    def __del__(self):
+        self.client.terminate(force=True)
+
+    @staticmethod
+    def set_ps1(client):
+        client.sendline('export PS1="{}"'.format(ShExpect.BASH_PATTERN))
+        client.expect(ShExpect.BASH_PATTERN)
+
+    def wait_for_jenkins(self):
+        def ping_jenkins():
+            try:
+                conn = http.client.HTTPConnection(container_addr,
+                                                  self.container.jenkins_port,
+                                                  timeout=30)
+                conn.request('HEAD', '/fuego/')
+                resp = conn.getresponse()
+                version = resp.getheader('X-Jenkins')
+                status = resp.status
+                conn.close()
+                LOGGER.debug(
+                    '  HTTP Response code: \'%d\' - Jenkins Version: \'%s\'',
+                    status, version)
+                if status == http.client.OK and version is not None:
+                    return True
+            except (ConnectionRefusedError, OSError):
+                return False
+
+            return False
+
+        container_addr = self.container.get_ip()
+        if container_addr is None:
+            return False
+        LOGGER.debug('Trying to reach jenkins at container \'%s\' via '
+                     'the container\'s IP \'%s\' at port \'%d\'',
+                     self.container.container_name,
+                     container_addr, self.container.jenkins_port)
+        if not loop_until_timeout(ping_jenkins, 10):
+            LOGGER.error('Could not connect to jenkins')
+            return False
+
+        return True
+
+
+class SeleniumContainerSession():
+    def __init__(self, container):
+        self.container = container
+        self.driver = None
+        self.root_url = container.get_url()
+
+    def start(self):
+        options = webdriver.ChromeOptions()
+        options.add_argument('headless')
+        options.add_argument('no-sandbox')
+        options.add_argument('window-size=1200x600')
+
+        self.driver = webdriver.Chrome(chrome_options=options)
+        self.driver.get(self.root_url)
+
+        self.driver.get(self.root_url)
+        LOGGER.debug('Started a Selenium Session on %s', self.root_url)
+        return True
+
+
+def main():
+    DEFAULT_TIMEOUT = 120
+    DEFAULT_IMAGE_NAME = 'fuego-release'
+    DEFAULT_CONTAINER_NAME = 'fuego-release-container'
+    DEFAULT_INSTALL_SCRIPT = 'install.sh'
+    DEFAULT_START_SCRIPT = 'fuego-host-scripts/docker-start-container.sh'
+    DEFAULT_JENKINS_PORT = 8080
+
+    def execute_tests(timeout):
+        LOGGER.debug('Starting tests')
+
+        ctx_mapper = {
+            ShExpect: pexpect_session,
+            SeleniumCommand: selenium_session
+        }
+
+        tests_ok = True
+        for cmd in COMMANDS_TO_TEST:
+            for base_class, ctx in ctx_mapper.items():
+                if isinstance(cmd, base_class):
+                    if not cmd.exec(ctx):
+                        tests_ok = False
+                        break
+
+        if tests_ok:
+            LOGGER.debug('Tests finished.')
+
+        return tests_ok
+
+    parser = argparse.ArgumentParser()
+    parser.add_argument('working_dir', help='The working directory', type=str)
+    parser.add_argument('-s', '--install-script',
+                        help='The script that will be used to install the '
+                        'docker image. Defaults to \'{}\''
+                        .format(DEFAULT_INSTALL_SCRIPT),
+                        default=DEFAULT_INSTALL_SCRIPT,
+                        type=str)
+    parser.add_argument('-a', '--start-script',
+                        help='The script used to start the container. '
+                        'Defaults to \'{}\''
+                        .format(DEFAULT_START_SCRIPT),
+                        default=DEFAULT_START_SCRIPT,
+                        type=str)
+    parser.add_argument('-i', '--image-name', default=DEFAULT_IMAGE_NAME,
+                        help='The image name that should be used. '
+                        'Defaults to \'{}\''
+                        .format(DEFAULT_IMAGE_NAME), type=str)
+    parser.add_argument('-c', '--container-name',
+                        default=DEFAULT_CONTAINER_NAME,
+                        help='The container name that should be used for the '
+                        'test. Defaults to \'{}\''
+                        .format(DEFAULT_CONTAINER_NAME),
+                        type=str)
+    parser.add_argument('-t', '--timeout', help='The timeout value for '
+                        'commands. Defaults to {}'
+                        .format(DEFAULT_TIMEOUT),
+                        default=DEFAULT_TIMEOUT, type=int)
+    parser.add_argument('-j', '--jenkins-port',
+                        help='The port where the jenkins is running on the '
+                        'test container. Defaults to {}'
+                        .format(DEFAULT_JENKINS_PORT),
+                        default=DEFAULT_JENKINS_PORT, type=int)
+    args = parser.parse_args()
+
+    LOGGER.debug('Changing working dir to \'%s\'', args.working_dir)
+    os.chdir(args.working_dir)
+
+    container = FuegoContainer(args.install_script, args.image_name,
+                               args.container_name, args.jenkins_port)
+
+    pexpect_session = PexpectContainerSession(container, args.start_script,
+                                              args.timeout)
+    if not pexpect_session.start():
+        return 1
+
+    selenium_session = SeleniumContainerSession(container)
+    if not selenium_session.start():
+        return 1
+
+    COMMANDS_TO_TEST = [
+        ShExpect(
+            'echo $\'hello\n\n\nfrom\n\n\ncontainer\'',
+            r'hello\s+from\s+container'),
+        ShExpect(
+            'cat -ThisOptionDoesNotExists', expected_result=1),
+        ShExpect('ftc add-nodes docker'),
+        ShExpect(
+            'ftc list-nodes', r'Jenkins nodes in this system:\s*docker\s*.*'),
+        ShExpect('ftc add-jobs -b docker -p testplan_docker'),
+        ShExpect(
+            'ftc list-jobs', r'Jenkins jobs in this system:(\s*docker\..*)+'),
+        Visit(url=container.get_url()),
+        CheckText(_id='executors', text='docker'),
+        CheckText(_id='executors', text='master')
+    ]
+
+    if not execute_tests(args.timeout):
+        return 1
+
+    return 0
+
+
+if __name__ == '__main__':
+    sys.exit(main())
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [Fuego] [PATCH 02/16] Mount fuego-rw/ro/core into the fuego-under-test container
  2018-03-29  0:08 [Fuego] [PATCH 00/16] Fuego Release Test Guilherme Campos Camargo
  2018-03-29  0:08 ` [Fuego] [PATCH 01/16] Add fuego-release Functional test Guilherme Campos Camargo
@ 2018-03-29  0:08 ` Guilherme Campos Camargo
  2018-03-30 22:31   ` Tim.Bird
  2018-03-29  0:08 ` [Fuego] [PATCH 03/16] Increase wait_for_jenkins timeout from 10 to 60s Guilherme Campos Camargo
                   ` (14 subsequent siblings)
  16 siblings, 1 reply; 40+ messages in thread
From: Guilherme Campos Camargo @ 2018-03-29  0:08 UTC (permalink / raw)
  To: fuego

This patch implements the necessary logic for mounting fuego-rw,
fuego-ro and fuego-core into fuego.

Given that the dockerd socket is shared between the host and the fuego
container, the mountpoints that are passed to the `docker create` call
need to be provided as full paths relative to the *host* (and not
relative to fuego, as would be expected if the docker daemons were
independent)

Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
---
 engine/tests/Functional.fuegotest/test_run.py | 63 +++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/engine/tests/Functional.fuegotest/test_run.py b/engine/tests/Functional.fuegotest/test_run.py
index 96fcfb7..7eae056 100755
--- a/engine/tests/Functional.fuegotest/test_run.py
+++ b/engine/tests/Functional.fuegotest/test_run.py
@@ -173,12 +173,43 @@ class FuegoContainer:
         self.container = None
 
     def setup_docker(self):
+        def this_container_id():
+            with open('/proc/self/cgroup', 'rt') as f:
+                f_text = f.read()
+
+                if 'docker' not in f_text:
+                    return None
+
+                for c in self.docker_client.containers(quiet=True):
+                    if c['Id'] in f_text:
+                        return c['Id']
+
+                return None
+
+        def map_to_host(mounts, container_id):
+            host_mounts = self.docker_client.\
+                inspect_container(container_id)['Mounts']
+
+            for mount in mounts:
+                LOGGER.debug('  Trying to find %s mountpoint in the host', mount['source'])
+                for host_mount in host_mounts:
+                    if mount['source'].startswith(host_mount['Destination']):
+                        mount['source'] = mount['source'].\
+                            replace(host_mount['Destination'],
+                                    host_mount['Source'], 1)
+                        LOGGER.debug('    Found: %s', mount['source'])
+                        break
+                else:
+                    LOGGER.debug('    Not Found')
+                    mount['source'] = None
+
         cmd = './{} {}'.format(self.install_script, self.image_name)
         LOGGER.debug('Running \'%s\' to install the docker image. '
                      'This may take a while....', cmd)
         status = subprocess.call(cmd, shell=True)
         if status != 0:
             return None
+
         docker_client = docker.from_env()
         containers = docker_client.containers.list(
             all=True, filters={'name': self.container_name})
@@ -188,10 +219,42 @@ class FuegoContainer:
                 self.container_name)
             containers[0].remove(force=True)
 
+        mounts = [
+            {'source': os.path.abspath('./fuego-rw'),
+             'destination':  '/fuego-rw',
+             'readonly': False,
+             },
+            {'source': os.path.abspath('./fuego-ro'),
+             'destination':  '/fuego-ro',
+             'readonly': True,
+             },
+            {'source': os.path.abspath('../fuego-core'),
+             'destination':  '/fuego-core',
+             'readonly': True,
+             },
+        ]
+
+        our_id = this_container_id()
+
+        if our_id:
+            LOGGER.debug('Running inside the Docker container %s', our_id)
+            map_to_host(mounts, our_id)
+        else:
+            LOGGER.debug('Not running inside a Docker container')
+
+        LOGGER.debug('Creating container with the following mountpoints:')
+        LOGGER.debug('  %s', mounts)
+
         container = docker_client.containers.create(
             self.image_name,
             stdin_open=True, tty=True, network_mode='bridge',
+            mounts=[docker.types.Mount(m['destination'],
+                                       m['source'],
+                                       type='bind',
+                                       read_only=m['readonly'])
+                    for m in mounts],
             name=self.container_name, command='/bin/bash')
+
         LOGGER.debug('Container \'%s\' created', self.container_name)
         return container
 
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [Fuego] [PATCH 03/16] Increase wait_for_jenkins timeout from 10 to 60s
  2018-03-29  0:08 [Fuego] [PATCH 00/16] Fuego Release Test Guilherme Campos Camargo
  2018-03-29  0:08 ` [Fuego] [PATCH 01/16] Add fuego-release Functional test Guilherme Campos Camargo
  2018-03-29  0:08 ` [Fuego] [PATCH 02/16] Mount fuego-rw/ro/core into the fuego-under-test container Guilherme Campos Camargo
@ 2018-03-29  0:08 ` Guilherme Campos Camargo
  2018-03-29  0:08 ` [Fuego] [PATCH 04/16] Print fuego repo/branch information during test build Guilherme Campos Camargo
                   ` (13 subsequent siblings)
  16 siblings, 0 replies; 40+ messages in thread
From: Guilherme Campos Camargo @ 2018-03-29  0:08 UTC (permalink / raw)
  To: fuego

Jenkins may take a little longer to be ready to receive commands in some
machines.

Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
---
 engine/tests/Functional.fuegotest/test_run.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/engine/tests/Functional.fuegotest/test_run.py b/engine/tests/Functional.fuegotest/test_run.py
index 7eae056..e3fcb68 100755
--- a/engine/tests/Functional.fuegotest/test_run.py
+++ b/engine/tests/Functional.fuegotest/test_run.py
@@ -357,7 +357,7 @@ class PexpectContainerSession():
                      'the container\'s IP \'%s\' at port \'%d\'',
                      self.container.container_name,
                      container_addr, self.container.jenkins_port)
-        if not loop_until_timeout(ping_jenkins, 10):
+        if not loop_until_timeout(ping_jenkins, timeout=60):
             LOGGER.error('Could not connect to jenkins')
             return False
 
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [Fuego] [PATCH 04/16] Print fuego repo/branch information during test build
  2018-03-29  0:08 [Fuego] [PATCH 00/16] Fuego Release Test Guilherme Campos Camargo
                   ` (2 preceding siblings ...)
  2018-03-29  0:08 ` [Fuego] [PATCH 03/16] Increase wait_for_jenkins timeout from 10 to 60s Guilherme Campos Camargo
@ 2018-03-29  0:08 ` Guilherme Campos Camargo
  2018-03-29  0:08 ` [Fuego] [PATCH 05/16] Properly check install return code and abort in case of failure Guilherme Campos Camargo
                   ` (12 subsequent siblings)
  16 siblings, 0 replies; 40+ messages in thread
From: Guilherme Campos Camargo @ 2018-03-29  0:08 UTC (permalink / raw)
  To: fuego

Also remove --quiet from git clone

Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
---
 engine/tests/Functional.fuegotest/fuego_test.sh | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/engine/tests/Functional.fuegotest/fuego_test.sh b/engine/tests/Functional.fuegotest/fuego_test.sh
index 192c15b..b4755bc 100755
--- a/engine/tests/Functional.fuegotest/fuego_test.sh
+++ b/engine/tests/Functional.fuegotest/fuego_test.sh
@@ -8,11 +8,16 @@ function test_build {
     if [ -d ${fuego_release_dir} ]; then
         rm -r ${fuego_release_dir}
     fi
-    git clone --quiet --depth 1 --single-branch \
+    echo "Cloning fuego from \
+        ${FUNCTIONAL_FUEGOTEST_FUEGOREPO}:${FUNCTIONAL_FUEGOTEST_FUEGOBRANCH}"
+    git clone --depth 1 --single-branch \
         --branch "${FUNCTIONAL_FUEGOTEST_FUEGOBRANCH}" \
         "${FUNCTIONAL_FUEGOTEST_FUEGOREPO}" \
         "${fuego_release_dir}/fuego"
-    git clone --quiet --depth 1 --single-branch \
+
+    echo "Cloning fuego-core from \
+        ${FUNCTIONAL_FUEGOTEST_FUEGOCOREREPO}:${FUNCTIONAL_FUEGOTEST_FUEGOCOREBRANCH}"
+    git clone --depth 1 --single-branch \
         --branch "${FUNCTIONAL_FUEGOTEST_FUEGOCOREBRANCH}" \
         "${FUNCTIONAL_FUEGOTEST_FUEGOCOREREPO}" \
         "${fuego_release_dir}/fuego-core"
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [Fuego] [PATCH 05/16] Properly check install return code and abort in case of failure
  2018-03-29  0:08 [Fuego] [PATCH 00/16] Fuego Release Test Guilherme Campos Camargo
                   ` (3 preceding siblings ...)
  2018-03-29  0:08 ` [Fuego] [PATCH 04/16] Print fuego repo/branch information during test build Guilherme Campos Camargo
@ 2018-03-29  0:08 ` Guilherme Campos Camargo
  2018-03-29  0:08 ` [Fuego] [PATCH 06/16] Allow the user to keep the container running after the test Guilherme Campos Camargo
                   ` (11 subsequent siblings)
  16 siblings, 0 replies; 40+ messages in thread
From: Guilherme Campos Camargo @ 2018-03-29  0:08 UTC (permalink / raw)
  To: fuego

A new method 'install()' has been added to 'FuegoContainer' and the all
the docker_setup steps have been moved from the class __init__() to this
new method.

This will allow the 'main' function to check if install has failed
simply by inspecting its return.

Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
---
 engine/tests/Functional.fuegotest/test_run.py | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/engine/tests/Functional.fuegotest/test_run.py b/engine/tests/Functional.fuegotest/test_run.py
index e3fcb68..d6bec0c 100755
--- a/engine/tests/Functional.fuegotest/test_run.py
+++ b/engine/tests/Functional.fuegotest/test_run.py
@@ -160,14 +160,17 @@ class FuegoContainer:
         self.container_name = container_name
         self.jenkins_port = jenkins_port
 
-        self.docker_client = docker.APIClient()
-        self.container = self.setup_docker()
-
     def __del__(self):
         if self.container:
             LOGGER.debug('Removing Container')
             self.container.remove(force=True)
 
+    def install(self):
+        self.docker_client = docker.APIClient()
+        self.container = self.setup_docker()
+
+        return self.container
+
     def stop(self):
         self.container.remove(force=True)
         self.container = None
@@ -191,7 +194,8 @@ class FuegoContainer:
                 inspect_container(container_id)['Mounts']
 
             for mount in mounts:
-                LOGGER.debug('  Trying to find %s mountpoint in the host', mount['source'])
+                LOGGER.debug('  Trying to find %s mountpoint in the host',
+                             mount['source'])
                 for host_mount in host_mounts:
                     if mount['source'].startswith(host_mount['Destination']):
                         mount['source'] = mount['source'].\
@@ -207,6 +211,9 @@ class FuegoContainer:
         LOGGER.debug('Running \'%s\' to install the docker image. '
                      'This may take a while....', cmd)
         status = subprocess.call(cmd, shell=True)
+
+        LOGGER.debug('Install output code: %s', status)
+
         if status != 0:
             return None
 
@@ -453,6 +460,8 @@ def main():
 
     container = FuegoContainer(args.install_script, args.image_name,
                                args.container_name, args.jenkins_port)
+    if not container.install():
+        return 1
 
     pexpect_session = PexpectContainerSession(container, args.start_script,
                                               args.timeout)
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [Fuego] [PATCH 06/16] Allow the user to keep the container running after the test
  2018-03-29  0:08 [Fuego] [PATCH 00/16] Fuego Release Test Guilherme Campos Camargo
                   ` (4 preceding siblings ...)
  2018-03-29  0:08 ` [Fuego] [PATCH 05/16] Properly check install return code and abort in case of failure Guilherme Campos Camargo
@ 2018-03-29  0:08 ` Guilherme Campos Camargo
  2018-03-30 22:48   ` Tim.Bird
  2018-03-29  0:08 ` [Fuego] [PATCH 07/16] Add Back() Selenium Command Guilherme Campos Camargo
                   ` (10 subsequent siblings)
  16 siblings, 1 reply; 40+ messages in thread
From: Guilherme Campos Camargo @ 2018-03-29  0:08 UTC (permalink / raw)
  To: fuego

Through the `--remove-test-container` command line argument.

Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
---
 engine/tests/Functional.fuegotest/test_run.py | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/engine/tests/Functional.fuegotest/test_run.py b/engine/tests/Functional.fuegotest/test_run.py
index d6bec0c..fa68858 100755
--- a/engine/tests/Functional.fuegotest/test_run.py
+++ b/engine/tests/Functional.fuegotest/test_run.py
@@ -154,16 +154,20 @@ class ShExpect():
 
 class FuegoContainer:
     def __init__(self, install_script, image_name, container_name,
-                 jenkins_port):
+                 jenkins_port, rm_after_test=True):
         self.install_script = install_script
         self.image_name = image_name
         self.container_name = container_name
         self.jenkins_port = jenkins_port
+        self.rm_after_test = rm_after_test
 
     def __del__(self):
         if self.container:
-            LOGGER.debug('Removing Container')
-            self.container.remove(force=True)
+            if self.rm_after_test:
+                LOGGER.debug('Removing Container')
+                self.container.remove(force=True)
+            else:
+                LOGGER.debug('Not Removing the test container')
 
     def install(self):
         self.docker_client = docker.APIClient()
@@ -453,13 +457,18 @@ def main():
                         'test container. Defaults to {}'
                         .format(DEFAULT_JENKINS_PORT),
                         default=DEFAULT_JENKINS_PORT, type=int)
+    parser.add_argument('--no-rm-container',
+                        help='Do not remove the container after tests are '
+                        'complete.',
+                        dest='rm_test_container', default=True, action='store_false')
     args = parser.parse_args()
 
     LOGGER.debug('Changing working dir to \'%s\'', args.working_dir)
     os.chdir(args.working_dir)
 
     container = FuegoContainer(args.install_script, args.image_name,
-                               args.container_name, args.jenkins_port)
+                               args.container_name, args.jenkins_port,
+                               rm_after_test=args.rm_test_container)
     if not container.install():
         return 1
 
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [Fuego] [PATCH 07/16] Add Back() Selenium Command
  2018-03-29  0:08 [Fuego] [PATCH 00/16] Fuego Release Test Guilherme Campos Camargo
                   ` (5 preceding siblings ...)
  2018-03-29  0:08 ` [Fuego] [PATCH 06/16] Allow the user to keep the container running after the test Guilherme Campos Camargo
@ 2018-03-29  0:08 ` Guilherme Campos Camargo
  2018-03-30 22:52   ` Tim.Bird
  2018-03-29  0:08 ` [Fuego] [PATCH 08/16] Add ClickLink selenium command Guilherme Campos Camargo
                   ` (9 subsequent siblings)
  16 siblings, 1 reply; 40+ messages in thread
From: Guilherme Campos Camargo @ 2018-03-29  0:08 UTC (permalink / raw)
  To: fuego

That will go one level back in the browser history.

Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
---
 engine/tests/Functional.fuegotest/test_run.py | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/engine/tests/Functional.fuegotest/test_run.py b/engine/tests/Functional.fuegotest/test_run.py
index fa68858..851074f 100755
--- a/engine/tests/Functional.fuegotest/test_run.py
+++ b/engine/tests/Functional.fuegotest/test_run.py
@@ -97,6 +97,16 @@ class CheckText(SeleniumCommand):
         return result == self.expected_result
 
 
+class Back(SeleniumCommand):
+    def exec(self, selenium_ctx):
+        super().exec(selenium_ctx)
+
+        LOGGER.debug('  Going back')
+        self.driver.back()
+
+        return True
+
+
 class ShExpect():
     BASH_PATTERN = 'test_run_pr1:#'
     COMMAND_RESULT_PATTERN = re.compile('^([0-9]+)', re.M)
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [Fuego] [PATCH 08/16] Add ClickLink selenium command
  2018-03-29  0:08 [Fuego] [PATCH 00/16] Fuego Release Test Guilherme Campos Camargo
                   ` (6 preceding siblings ...)
  2018-03-29  0:08 ` [Fuego] [PATCH 07/16] Add Back() Selenium Command Guilherme Campos Camargo
@ 2018-03-29  0:08 ` Guilherme Campos Camargo
  2018-03-30 22:53   ` Tim.Bird
  2018-03-29  0:08 ` [Fuego] [PATCH 09/16] Include add-jobs, add-views and build-jobs tests Guilherme Campos Camargo
                   ` (8 subsequent siblings)
  16 siblings, 1 reply; 40+ messages in thread
From: Guilherme Campos Camargo @ 2018-03-29  0:08 UTC (permalink / raw)
  To: fuego

Clicks in the link that has the text passed in the argument `linktext`.
Returns False if no link containing `linktext` is found.

Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
---
 engine/tests/Functional.fuegotest/test_run.py | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/engine/tests/Functional.fuegotest/test_run.py b/engine/tests/Functional.fuegotest/test_run.py
index 851074f..e03853b 100755
--- a/engine/tests/Functional.fuegotest/test_run.py
+++ b/engine/tests/Functional.fuegotest/test_run.py
@@ -13,6 +13,7 @@ import docker
 import pexpect
 import requests
 from selenium import webdriver
+import selenium.common.exceptions as selenium_exceptions
 
 LOGGER = logging.getLogger('test_run')
 STREAM_HANDLER = logging.StreamHandler()
@@ -97,6 +98,29 @@ class CheckText(SeleniumCommand):
         return result == self.expected_result
 
 
+class ClickLink(SeleniumCommand):
+    def __init__(self, linktext, expected_result=True):
+        self.linktext = linktext
+        self.expected_result = expected_result
+
+    def exec(self, selenium_ctx):
+        super().exec(selenium_ctx)
+
+        LOGGER.debug(
+            '  Searching for a link with the text \'%s\'', self.linktext)
+
+        try:
+            link = self.driver.find_element_by_partial_link_text(self.linktext)
+            link.click()
+            LOGGER.debug('  Link found and clicked')
+            result = True
+        except selenium_exceptions.NoSuchElementException:
+            LOGGER.error('  Link not found')
+            result = False
+
+        return result == self.expected_result
+
+
 class Back(SeleniumCommand):
     def exec(self, selenium_ctx):
         super().exec(selenium_ctx)
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [Fuego] [PATCH 09/16] Include add-jobs, add-views and build-jobs tests
  2018-03-29  0:08 [Fuego] [PATCH 00/16] Fuego Release Test Guilherme Campos Camargo
                   ` (7 preceding siblings ...)
  2018-03-29  0:08 ` [Fuego] [PATCH 08/16] Add ClickLink selenium command Guilherme Campos Camargo
@ 2018-03-29  0:08 ` Guilherme Campos Camargo
  2018-03-29  0:08 ` [Fuego] [PATCH 10/16] Write ok/fail on test report Guilherme Campos Camargo
                   ` (7 subsequent siblings)
  16 siblings, 0 replies; 40+ messages in thread
From: Guilherme Campos Camargo @ 2018-03-29  0:08 UTC (permalink / raw)
  To: fuego

Those tests are using a combination of ShExpect and SeleniumHQ for
executing the command and checking if the website has responded
properly.

For now, SeleniumHQ is just being used for viewing the results on the
website, and not for sending commands. In other words, build-jobs is
being triggered through the command-line only, and not through the
Jenkins UI.

Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
---
 engine/tests/Functional.fuegotest/test_run.py | 34 +++++++++++++++++----------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/engine/tests/Functional.fuegotest/test_run.py b/engine/tests/Functional.fuegotest/test_run.py
index e03853b..09b3a95 100755
--- a/engine/tests/Functional.fuegotest/test_run.py
+++ b/engine/tests/Functional.fuegotest/test_run.py
@@ -516,20 +516,30 @@ def main():
         return 1
 
     COMMANDS_TO_TEST = [
-        ShExpect(
-            'echo $\'hello\n\n\nfrom\n\n\ncontainer\'',
-            r'hello\s+from\s+container'),
-        ShExpect(
-            'cat -ThisOptionDoesNotExists', expected_result=1),
-        ShExpect('ftc add-nodes docker'),
-        ShExpect(
-            'ftc list-nodes', r'Jenkins nodes in this system:\s*docker\s*.*'),
-        ShExpect('ftc add-jobs -b docker -p testplan_docker'),
-        ShExpect(
-            'ftc list-jobs', r'Jenkins jobs in this system:(\s*docker\..*)+'),
+        # Set Selenium Browser root
         Visit(url=container.get_url()),
+
+        # Add Nodes
+        ShExpect('ftc add-nodes docker'),
+        ShExpect('ftc list-nodes -q', r'.*docker.*'),
+        CheckText(_id='executors', text='master'),
         CheckText(_id='executors', text='docker'),
-        CheckText(_id='executors', text='master')
+
+        # Add Fuego TestPlan
+        ShExpect('ftc add-jobs -b docker -p testplan_fuego_tests'),
+        ShExpect('ftc list-jobs', r'.*docker\.testplan_fuego_tests\.batch.*'),
+
+        ClickLink(linktext='docker'),
+        CheckText(_id='projectstatus', text='docker.testplan_fuego_tests.batch'),
+        Back(),
+
+        # Install Views
+        ShExpect('ftc add-view batch .*.batch'),
+        CheckText(_id='projectstatus-tabBar', text='batch'),
+
+        # Start Tests
+        ShExpect('ftc build-jobs *.*.Functional.fuego_board_check'),
+        CheckText(_id='buildQueue', text='Functional.fuego_board_check'),
     ]
 
     if not execute_tests(args.timeout):
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [Fuego] [PATCH 10/16] Write ok/fail on test report
  2018-03-29  0:08 [Fuego] [PATCH 00/16] Fuego Release Test Guilherme Campos Camargo
                   ` (8 preceding siblings ...)
  2018-03-29  0:08 ` [Fuego] [PATCH 09/16] Include add-jobs, add-views and build-jobs tests Guilherme Campos Camargo
@ 2018-03-29  0:08 ` Guilherme Campos Camargo
  2018-03-30 22:58   ` Tim.Bird
  2018-03-29  0:08 ` [Fuego] [PATCH 11/16] Properly quit Selenium driver when SeleniumSession is deleted Guilherme Campos Camargo
                   ` (6 subsequent siblings)
  16 siblings, 1 reply; 40+ messages in thread
From: Guilherme Campos Camargo @ 2018-03-29  0:08 UTC (permalink / raw)
  To: fuego

Remove the 'minimal test' that had been copied from an example test.

Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
---
 engine/tests/Functional.fuegotest/fuego_test.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/engine/tests/Functional.fuegotest/fuego_test.sh b/engine/tests/Functional.fuegotest/fuego_test.sh
index b4755bc..ee60c81 100755
--- a/engine/tests/Functional.fuegotest/fuego_test.sh
+++ b/engine/tests/Functional.fuegotest/fuego_test.sh
@@ -1,7 +1,5 @@
 #!/bin/bash
 
-set -e
-
 readonly fuego_release_dir=fuego-release
 
 function test_build {
@@ -26,9 +24,13 @@ function test_build {
 
 function test_run {
     sudo -n ${TEST_HOME}/test_run.py "${fuego_release_dir}/fuego"
-    report "echo ok 1 minimal test on target"
+    if [ "${?}" = 0 ]; then
+        report "echo fuego-test: ok"
+    else
+        report "echo fuego-test: fail"
+    fi
 }
 
 function test_processing {
-    log_compare "$TESTDIR" "1" "^ok" "p"
+    log_compare "$TESTDIR" "1" "ok$" "p"
 }
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [Fuego] [PATCH 11/16] Properly quit Selenium driver when SeleniumSession is deleted
  2018-03-29  0:08 [Fuego] [PATCH 00/16] Fuego Release Test Guilherme Campos Camargo
                   ` (9 preceding siblings ...)
  2018-03-29  0:08 ` [Fuego] [PATCH 10/16] Write ok/fail on test report Guilherme Campos Camargo
@ 2018-03-29  0:08 ` Guilherme Campos Camargo
  2018-03-29  0:08 ` [Fuego] [PATCH 12/16] Minor style/PEP8 fixes Guilherme Campos Camargo
                   ` (5 subsequent siblings)
  16 siblings, 0 replies; 40+ messages in thread
From: Guilherme Campos Camargo @ 2018-03-29  0:08 UTC (permalink / raw)
  To: fuego

Call `driver.quit()` when SeleniumContainerSession.__del__() method gets
called.

Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
---
 engine/tests/Functional.fuegotest/test_run.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/engine/tests/Functional.fuegotest/test_run.py b/engine/tests/Functional.fuegotest/test_run.py
index 09b3a95..99bd309 100755
--- a/engine/tests/Functional.fuegotest/test_run.py
+++ b/engine/tests/Functional.fuegotest/test_run.py
@@ -428,6 +428,10 @@ class SeleniumContainerSession():
         LOGGER.debug('Started a Selenium Session on %s', self.root_url)
         return True
 
+    def __del__(self):
+        if self.driver:
+            self.driver.quit()
+
 
 def main():
     DEFAULT_TIMEOUT = 120
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [Fuego] [PATCH 12/16] Minor style/PEP8 fixes
  2018-03-29  0:08 [Fuego] [PATCH 00/16] Fuego Release Test Guilherme Campos Camargo
                   ` (10 preceding siblings ...)
  2018-03-29  0:08 ` [Fuego] [PATCH 11/16] Properly quit Selenium driver when SeleniumSession is deleted Guilherme Campos Camargo
@ 2018-03-29  0:08 ` Guilherme Campos Camargo
  2018-03-29  0:08 ` [Fuego] [PATCH 13/16] Move Selenium implicitly_wait() to SeleniumSession start Guilherme Campos Camargo
                   ` (4 subsequent siblings)
  16 siblings, 0 replies; 40+ messages in thread
From: Guilherme Campos Camargo @ 2018-03-29  0:08 UTC (permalink / raw)
  To: fuego

Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
---
 engine/tests/Functional.fuegotest/test_run.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/engine/tests/Functional.fuegotest/test_run.py b/engine/tests/Functional.fuegotest/test_run.py
index 99bd309..f62ebc1 100755
--- a/engine/tests/Functional.fuegotest/test_run.py
+++ b/engine/tests/Functional.fuegotest/test_run.py
@@ -498,7 +498,8 @@ def main():
     parser.add_argument('--no-rm-container',
                         help='Do not remove the container after tests are '
                         'complete.',
-                        dest='rm_test_container', default=True, action='store_false')
+                        dest='rm_test_container',
+                        default=True, action='store_false')
     args = parser.parse_args()
 
     LOGGER.debug('Changing working dir to \'%s\'', args.working_dir)
@@ -534,7 +535,8 @@ def main():
         ShExpect('ftc list-jobs', r'.*docker\.testplan_fuego_tests\.batch.*'),
 
         ClickLink(linktext='docker'),
-        CheckText(_id='projectstatus', text='docker.testplan_fuego_tests.batch'),
+        CheckText(_id='projectstatus',
+                  text='docker.testplan_fuego_tests.batch'),
         Back(),
 
         # Install Views
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [Fuego] [PATCH 13/16] Move Selenium implicitly_wait() to SeleniumSession start
  2018-03-29  0:08 [Fuego] [PATCH 00/16] Fuego Release Test Guilherme Campos Camargo
                   ` (11 preceding siblings ...)
  2018-03-29  0:08 ` [Fuego] [PATCH 12/16] Minor style/PEP8 fixes Guilherme Campos Camargo
@ 2018-03-29  0:08 ` Guilherme Campos Camargo
  2018-03-29  0:08 ` [Fuego] [PATCH 14/16] Use a fixed language for Selenium Chrome-WebDriver Guilherme Campos Camargo
                   ` (3 subsequent siblings)
  16 siblings, 0 replies; 40+ messages in thread
From: Guilherme Campos Camargo @ 2018-03-29  0:08 UTC (permalink / raw)
  To: fuego

Remove it from SeleniumCommand.exec(), given that "once set, it will be
set for the life of the WebDriver object".

implictly_wait() documentation:
http://selenium-python.readthedocs.io/waits.html#implicit-waits

Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
---
 engine/tests/Functional.fuegotest/test_run.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/engine/tests/Functional.fuegotest/test_run.py b/engine/tests/Functional.fuegotest/test_run.py
index f62ebc1..91d0666 100755
--- a/engine/tests/Functional.fuegotest/test_run.py
+++ b/engine/tests/Functional.fuegotest/test_run.py
@@ -41,7 +41,6 @@ class SeleniumCommand:
     def exec(self, selenium_ctx):
         self.driver = selenium_ctx.driver
         self.driver.refresh()
-        self.driver.implicitly_wait(3)
         LOGGER.debug('Executing Selenium Command \'%s\'',
                      self.__class__.__name__)
 
@@ -422,9 +421,10 @@ class SeleniumContainerSession():
         options.add_argument('window-size=1200x600')
 
         self.driver = webdriver.Chrome(chrome_options=options)
-        self.driver.get(self.root_url)
+        self.driver.implicitly_wait(3)
 
         self.driver.get(self.root_url)
+
         LOGGER.debug('Started a Selenium Session on %s', self.root_url)
         return True
 
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [Fuego] [PATCH 14/16] Use a fixed language for Selenium Chrome-WebDriver
  2018-03-29  0:08 [Fuego] [PATCH 00/16] Fuego Release Test Guilherme Campos Camargo
                   ` (12 preceding siblings ...)
  2018-03-29  0:08 ` [Fuego] [PATCH 13/16] Move Selenium implicitly_wait() to SeleniumSession start Guilherme Campos Camargo
@ 2018-03-29  0:08 ` Guilherme Campos Camargo
  2018-03-29  0:08 ` [Fuego] [PATCH 15/16] Improve Click and Check methods to allow multiple locators Guilherme Campos Camargo
                   ` (2 subsequent siblings)
  16 siblings, 0 replies; 40+ messages in thread
From: Guilherme Campos Camargo @ 2018-03-29  0:08 UTC (permalink / raw)
  To: fuego

This will allow elements to be searched by text (english) disregarding
eventual page translations that may take place depending on the system's
locale.

Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
---
 engine/tests/Functional.fuegotest/test_run.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/engine/tests/Functional.fuegotest/test_run.py b/engine/tests/Functional.fuegotest/test_run.py
index 91d0666..0370b6f 100755
--- a/engine/tests/Functional.fuegotest/test_run.py
+++ b/engine/tests/Functional.fuegotest/test_run.py
@@ -419,6 +419,8 @@ class SeleniumContainerSession():
         options.add_argument('headless')
         options.add_argument('no-sandbox')
         options.add_argument('window-size=1200x600')
+        options.add_experimental_option(
+            'prefs', {'intl.accept_languages': 'en,en_US'})
 
         self.driver = webdriver.Chrome(chrome_options=options)
         self.driver.implicitly_wait(3)
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [Fuego] [PATCH 15/16] Improve Click and Check methods to allow multiple locators
  2018-03-29  0:08 [Fuego] [PATCH 00/16] Fuego Release Test Guilherme Campos Camargo
                   ` (13 preceding siblings ...)
  2018-03-29  0:08 ` [Fuego] [PATCH 14/16] Use a fixed language for Selenium Chrome-WebDriver Guilherme Campos Camargo
@ 2018-03-29  0:08 ` Guilherme Campos Camargo
  2018-03-30 23:03   ` Tim.Bird
  2018-03-29  0:08 ` [Fuego] [PATCH 16/16] Add test that starts a job through the UI Guilherme Campos Camargo
  2018-03-30 21:34 ` [Fuego] [PATCH 00/16] Fuego Release Test Tim.Bird
  16 siblings, 1 reply; 40+ messages in thread
From: Guilherme Campos Camargo @ 2018-03-29  0:08 UTC (permalink / raw)
  To: fuego

Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
---
 engine/tests/Functional.fuegotest/test_run.py | 105 ++++++++++++++++----------
 1 file changed, 65 insertions(+), 40 deletions(-)

diff --git a/engine/tests/Functional.fuegotest/test_run.py b/engine/tests/Functional.fuegotest/test_run.py
index 0370b6f..d433548 100755
--- a/engine/tests/Functional.fuegotest/test_run.py
+++ b/engine/tests/Functional.fuegotest/test_run.py
@@ -13,6 +13,7 @@ import docker
 import pexpect
 import requests
 from selenium import webdriver
+from selenium.webdriver.common.by import By
 import selenium.common.exceptions as selenium_exceptions
 
 LOGGER = logging.getLogger('test_run')
@@ -44,6 +45,46 @@ class SeleniumCommand:
         LOGGER.debug('Executing Selenium Command \'%s\'',
                      self.__class__.__name__)
 
+    @staticmethod
+    def check_element_text(element, text):
+        try:
+            if text in element.text:
+                LOGGER.\
+                    debug('  Text \'%s\' matches element.text \'%s\'',
+                          text, element.text)
+                return True
+            else:
+                LOGGER.\
+                    debug('  Text \'%s\' does not match element.text \'%s\'',
+                          text, element.text)
+                return False
+        except (selenium_exceptions.ElementNotVisibleException,
+                selenium_exceptions.NoSuchAttributeException,):
+            LOGGER.error('  Element has no visible Text')
+            return False
+
+    @staticmethod
+    def click_element(element):
+        try:
+            element.click()
+            LOGGER.debug('  Element clicked')
+            return True
+        except (selenium_exceptions.ElementClickInterceptedException,
+                selenium_exceptions.ElementNotVisibleException,):
+            LOGGER.error('  Element is not clickable')
+            return False
+
+    @staticmethod
+    def find_element(context, locator, pattern):
+        try:
+            element = context.driver.find_element(locator, pattern)
+        except selenium_exceptions.NoSuchElementException:
+            LOGGER.error('  Element not found')
+            return None
+
+        LOGGER.debug('  Element found')
+        return element
+
 
 class Visit(SeleniumCommand):
     def __init__(self, url, timeout=10, expected_result=200):
@@ -69,55 +110,37 @@ class Visit(SeleniumCommand):
 
 
 class CheckText(SeleniumCommand):
-    def __init__(self, _id, text, expected_result=True):
-        self._id = _id
+    def __init__(self, locator, pattern, text='', expected_result=True):
+        self.pattern = pattern
+        self.locator = locator
         self.text = text
         self.expected_result = expected_result
 
     def exec(self, selenium_ctx):
         super().exec(selenium_ctx)
 
-        try:
-            text = self.driver.find_element_by_id(self._id).text
-        except Exception:  # TODO: Use proper Exception
-            return False
-
-        LOGGER.debug(
-            '  Searching for \'%s\' in \'id:%s\'', self.text, self._id)
-
-        result = True
-        if self.text not in text:
-            LOGGER.error(
-                '  \'%s\' not found in id \'%s\' with text \'%s\'', self.text,
-                self._id, text)
-            result = False
-
-        LOGGER.debug('  \'%s\' was found', self.text)
+        element = SeleniumCommand.\
+            find_element(selenium_ctx, self.locator, self.pattern)
+        if element:
+            result = SeleniumCommand.check_element_text(element, self.text)
 
         return result == self.expected_result
 
 
-class ClickLink(SeleniumCommand):
-    def __init__(self, linktext, expected_result=True):
-        self.linktext = linktext
-        self.expected_result = expected_result
+class Click(SeleniumCommand):
+    def __init__(self, locator, pattern):
+        self.pattern = pattern
+        self.locator = locator
 
     def exec(self, selenium_ctx):
         super().exec(selenium_ctx)
 
-        LOGGER.debug(
-            '  Searching for a link with the text \'%s\'', self.linktext)
+        element = SeleniumCommand.\
+            find_element(selenium_ctx, self.locator, self.pattern)
+        if element:
+            return SeleniumCommand.click_element(element)
 
-        try:
-            link = self.driver.find_element_by_partial_link_text(self.linktext)
-            link.click()
-            LOGGER.debug('  Link found and clicked')
-            result = True
-        except selenium_exceptions.NoSuchElementException:
-            LOGGER.error('  Link not found')
-            result = False
-
-        return result == self.expected_result
+        return False
 
 
 class Back(SeleniumCommand):
@@ -529,25 +552,27 @@ def main():
         # Add Nodes
         ShExpect('ftc add-nodes docker'),
         ShExpect('ftc list-nodes -q', r'.*docker.*'),
-        CheckText(_id='executors', text='master'),
-        CheckText(_id='executors', text='docker'),
+        CheckText(By.ID, 'executors', text='master'),
+        CheckText(By.ID, 'executors', text='docker'),
 
         # Add Fuego TestPlan
         ShExpect('ftc add-jobs -b docker -p testplan_fuego_tests'),
         ShExpect('ftc list-jobs', r'.*docker\.testplan_fuego_tests\.batch.*'),
 
-        ClickLink(linktext='docker'),
-        CheckText(_id='projectstatus',
+        Click(By.PARTIAL_LINK_TEXT, 'docker'),
+        CheckText(By.ID, 'projectstatus',
                   text='docker.testplan_fuego_tests.batch'),
         Back(),
 
         # Install Views
         ShExpect('ftc add-view batch .*.batch'),
-        CheckText(_id='projectstatus-tabBar', text='batch'),
+        CheckText(By.ID, 'projectstatus-tabBar',
+                  text='batch'),
 
         # Start Tests
         ShExpect('ftc build-jobs *.*.Functional.fuego_board_check'),
-        CheckText(_id='buildQueue', text='Functional.fuego_board_check'),
+        CheckText(By.ID, 'buildQueue',
+                  text='Functional.fuego_board_check'),
     ]
 
     if not execute_tests(args.timeout):
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [Fuego] [PATCH 16/16] Add test that starts a job through the UI
  2018-03-29  0:08 [Fuego] [PATCH 00/16] Fuego Release Test Guilherme Campos Camargo
                   ` (14 preceding siblings ...)
  2018-03-29  0:08 ` [Fuego] [PATCH 15/16] Improve Click and Check methods to allow multiple locators Guilherme Campos Camargo
@ 2018-03-29  0:08 ` Guilherme Campos Camargo
  2018-03-30 21:34 ` [Fuego] [PATCH 00/16] Fuego Release Test Tim.Bird
  16 siblings, 0 replies; 40+ messages in thread
From: Guilherme Campos Camargo @ 2018-03-29  0:08 UTC (permalink / raw)
  To: fuego

By Clicking on the Schedule Job button, then checking if the job has
been added to the executor's list, through the element XPath.

Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
---
 engine/tests/Functional.fuegotest/test_run.py | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/engine/tests/Functional.fuegotest/test_run.py b/engine/tests/Functional.fuegotest/test_run.py
index d433548..cdf735d 100755
--- a/engine/tests/Functional.fuegotest/test_run.py
+++ b/engine/tests/Functional.fuegotest/test_run.py
@@ -569,10 +569,16 @@ def main():
         CheckText(By.ID, 'projectstatus-tabBar',
                   text='batch'),
 
-        # Start Tests
+        # Start a Test through Command Line
         ShExpect('ftc build-jobs *.*.Functional.fuego_board_check'),
         CheckText(By.ID, 'buildQueue',
                   text='Functional.fuego_board_check'),
+
+        # Start a Test through Jenkins UI (Xpath)
+        Click(By.XPATH,
+              '//img[@title="Schedule a build for docker.default.Functional.hello_world"]'),
+        CheckText(By.ID, 'executors',
+                  text='docker.default.Functional.hello_world'),
     ]
 
     if not execute_tests(args.timeout):
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 00/16] Fuego Release Test
  2018-03-29  0:08 [Fuego] [PATCH 00/16] Fuego Release Test Guilherme Campos Camargo
                   ` (15 preceding siblings ...)
  2018-03-29  0:08 ` [Fuego] [PATCH 16/16] Add test that starts a job through the UI Guilherme Campos Camargo
@ 2018-03-30 21:34 ` Tim.Bird
  2018-04-03  3:16   ` Guilherme Camargo
  16 siblings, 1 reply; 40+ messages in thread
From: Tim.Bird @ 2018-03-30 21:34 UTC (permalink / raw)
  To: guicc, fuego

Thanks Guilherme, for posting the patches.  I'm excited about this work.
See comments inline below.

> -----Original Message-----
> From: Guilherme Campos Camargo
> Hello, everyone
> 
> This series of patches contains the initial implementation of the Fuego
> Release Test (Functional.fuegotest).

Please rename this test to Functional.fuego_release_test.  This fits
the naming for other Fuego-related tests better.

I haven't looked at all the patches yet, but if they're for the fuego-core
repository, and are all in the Functional.fuegotest directory, then
I plan to put them into my master branch.  Since they're self-contained
they shouldn't affect other Fuego tests or infrastructure, or interfere
with any work on the 1.3 release.

I can also put them into my next branch.

> This test has been created to allow
> Fuego to test any version of Fuego, given its git repository and branch.
> 
> The test clones the repositories of fuego and fuego-core that are
> configured on the test specs.json, installs fuego and runs that version
> of fuego as the docker container fuego-release-container.
> 
> The test uses Pexpect, to execute/verify commands in the
> fuego-release-container shell, and uses SeleniumHQ to interact with the
> Jenkins web interface, checking if the web interface responds as
> expected.
This is nice as it adds some additional materials and examples we can
use for other tests.

> 
> A few wrappers have been implemented on top of Pexpect and SeleniumHQ
> in
> order to facilitate the inclusion of new tests.
> 
> The default specs.json is currently configured to clone and test fuego
> and fuego-core from the master branch of the official repository. What
> can be changed through the specs.json file.

Sounds good.  I haven't looked yet, but it would be good to have
specs for 'next', and 'test'.  I use those branches fairly often.

> # Running
> 
> Currently this test requires a modified version of Fuego to be executed,
> given that it needs to install some dependencies and needs to map the
> dockerd socket from the host to the fuego container.
> 
> The modified version can be found in two different branches on
> Profusion's fuego fork.
> 
>  1 - Branch fuego-test: Just a few commits that are necessary for making
>  this test work, applied on top of fuego/next. We plan to try to
>  integrate these commits into fuego/next in the next few days.
> 
>  2 - Branch fuego-base-image: A more complex change on fuego, that makes
>  the necessary changes for allowing it to be shipped as a docker image
>  through dockerhub.
> 
> The steps for each one of the versions above are given below:
> 
> ## Building the image (from the branch fuego-test)
> 
> To run the test, execute the following commands:
> 
> ```
> git clone --branch fuego-test https://bitbucket.org/profusionmobi/fuego-
> core.git
> git clone --branch fuego-test https://bitbucket.org/profusionmobi/fuego.git
> cd fuego
> ./install fuego-to-test-fuego
> ./fuego-host-scripts/docker-create-container.sh fuego-to-test-fuego fuego-
> to-test-fuego-container
> ./fuego-host-scripts/docker-start-container.sh fuego-to-test-fuego-
> container
> ```
> 
> Then, add the fuego-test board and the Functional.fuegotest and start
> the test through Jenkins (localhost:8080/fuego/)
> 
> ```
> ftc add-nodes fuego-test
> ftc add-jobs -b fuego-test -t Functional.fuegotest
> ```
> 
> ## Using the modified Fuego Base Image from Dockerhub
> (fuego-base-image):
> 
> You can also use the fuego base image that's being developed in
> Profusion's fuego-base-image branch in our fork:
> https://bitbucket.org/profusionmobi/fuego/branch/fuego-base-image
> 
> The image is already available on dockerhub and can be
> downloaded/executed with:
> 
> ```
> docker pull fuegotest/fuego
> docker run -it \
>   -p 8080:8080 \
>   -v $(pwd)/host_fuego_home:/var/fuego_home \
>   -e JENKINS_UID=$(id -u) \
>   -e JENKINS_GID=$(id -g) \
>   -v /var/run/docker.sock:/var/run/docker.sock \
>   fuegotest/fuego:latest
> ```
> 
> Wait for the shell to be available and add fuego-test board and
> Functional.fuegotest as explained in the last section.
> 
> ```
> ftc add-nodes fuego-test
> ftc add-jobs -b fuego-test -t Functional.fuegotest
> ```

It's very nice to have detailed instructions for both methods.
Thanks!
 -- Tim

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 01/16] Add fuego-release Functional test
  2018-03-29  0:08 ` [Fuego] [PATCH 01/16] Add fuego-release Functional test Guilherme Campos Camargo
@ 2018-03-30 22:17   ` Tim.Bird
  2018-03-30 22:37     ` Tim.Bird
  2018-04-03  3:17     ` Guilherme Camargo
  0 siblings, 2 replies; 40+ messages in thread
From: Tim.Bird @ 2018-03-30 22:17 UTC (permalink / raw)
  To: guicc, fuego


My strategy is to apply all your patches, but ask for modifications,
that you can provide as patches on top of this set.

If I make these changes, they will break other patches in the
series, which will be obnoxious and time-consuming to fix.

See below inline for comments.
 -- Tim

> -----Original Message-----
> From: Guilherme Campos Camargo
> Allows Fuego to test new Fuego releases.
> 
> The fuego and fuego-core repositores and branches to be tested may be
> configured on the specs.json file. Their defaults are:
> 
> "FUEGOREPO":"https://bitbucket.org/tbird20d/fuego",
> "FUEGOBRANCH":"master",
> "FUEGOCOREREPO":"https://bitbucket.org/tbird20d/fuego-core",
> "FUEGOCOREBRANCH":"master"
> 
Can you please add underscores to these names:
FUEGO_REPO, FUEGO_CORE_BRANCH, etc.
?

It's very nice to be able to customize these in the spec file.

> Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
> ---
>  engine/tests/Functional.fuegotest/fuego_test.sh |  29 ++
>  engine/tests/Functional.fuegotest/spec.json     |  11 +
>  engine/tests/Functional.fuegotest/test_run.py   | 427
> ++++++++++++++++++++++++
>  3 files changed, 467 insertions(+)
>  create mode 100755 engine/tests/Functional.fuegotest/fuego_test.sh
>  create mode 100644 engine/tests/Functional.fuegotest/spec.json
>  create mode 100755 engine/tests/Functional.fuegotest/test_run.py
> 
> diff --git a/engine/tests/Functional.fuegotest/fuego_test.sh
> b/engine/tests/Functional.fuegotest/fuego_test.sh
> new file mode 100755
> index 0000000..192c15b
> --- /dev/null
> +++ b/engine/tests/Functional.fuegotest/fuego_test.sh
> @@ -0,0 +1,29 @@
> +#!/bin/bash
> +
> +set -e
> +
> +readonly fuego_release_dir=fuego-release
> +
> +function test_build {
> +    if [ -d ${fuego_release_dir} ]; then
> +        rm -r ${fuego_release_dir}
> +    fi
> +    git clone --quiet --depth 1 --single-branch \
> +        --branch "${FUNCTIONAL_FUEGOTEST_FUEGOBRANCH}" \
> +        "${FUNCTIONAL_FUEGOTEST_FUEGOREPO}" \
> +        "${fuego_release_dir}/fuego"
> +    git clone --quiet --depth 1 --single-branch \
> +        --branch "${FUNCTIONAL_FUEGOTEST_FUEGOCOREBRANCH}" \
> +        "${FUNCTIONAL_FUEGOTEST_FUEGOCOREREPO}" \
> +        "${fuego_release_dir}/fuego-core"
> +    cd -
> +}
> +
> +function test_run {
> +    sudo -n ${TEST_HOME}/test_run.py "${fuego_release_dir}/fuego"
> +    report "echo ok 1 minimal test on target"
> +}
> +
> +function test_processing {
> +    log_compare "$TESTDIR" "1" "^ok" "p"
> +}
> diff --git a/engine/tests/Functional.fuegotest/spec.json
> b/engine/tests/Functional.fuegotest/spec.json
> new file mode 100644
> index 0000000..cc345ba
> --- /dev/null
> +++ b/engine/tests/Functional.fuegotest/spec.json
> @@ -0,0 +1,11 @@
> +{
> +    "testName": "Functional.fuegotest",
> +    "specs": {
> +        "default": {
> +            "FUEGOREPO":"https://bitbucket.org/tbird20d/fuego",
> +            "FUEGOBRANCH":"master",
> +            "FUEGOCOREREPO":"https://bitbucket.org/tbird20d/fuego-core",
> +            "FUEGOCOREBRANCH":"master"
> +        }
Can you add specs for my 'test' and 'next' branches as well?

> +    }
> +}
> diff --git a/engine/tests/Functional.fuegotest/test_run.py
> b/engine/tests/Functional.fuegotest/test_run.py
> new file mode 100755
> index 0000000..96fcfb7
> --- /dev/null
> +++ b/engine/tests/Functional.fuegotest/test_run.py
> @@ -0,0 +1,427 @@
> +#!/usr/bin/env python3
> +import argparse
> +import http
> +import http.client
> +import logging
> +import os
> +import re
> +import subprocess
> +import sys
> +import time
> +
> +import docker
> +import pexpect
> +import requests
> +from selenium import webdriver
> +
> +LOGGER = logging.getLogger('test_run')
> +STREAM_HANDLER = logging.StreamHandler()
> +STREAM_HANDLER.setFormatter(
> +    logging.Formatter('%(name)s:%(levelname)s: %(message)s'))
> +LOGGER.setLevel(logging.DEBUG)
> +LOGGER.addHandler(STREAM_HANDLER)
> +
> +
> +def loop_until_timeout(func, timeout=10, num_tries=5):
> +    LOGGER.debug('Running %s()', func.__name__)
> +
> +    for _ in range(num_tries):
> +        LOGGER.debug('  Try number %s...', _ + 1)
> +        if func():
> +            LOGGER.debug('  Success')
> +            return True
> +        time.sleep(timeout/num_tries)
> +    LOGGER.debug('  Failure')
> +
> +    return False
> +
> +
> +class SeleniumCommand:
> +    def exec(self, selenium_ctx):
> +        self.driver = selenium_ctx.driver
> +        self.driver.refresh()
> +        self.driver.implicitly_wait(3)
> +        LOGGER.debug('Executing Selenium Command \'%s\'',
> +                     self.__class__.__name__)
> +
> +
> +class Visit(SeleniumCommand):
> +    def __init__(self, url, timeout=10, expected_result=200):
> +        self.url = url
> +        self.timeout = timeout
> +        self.expected_result = expected_result
> +
> +    def exec(self, selenium_ctx):
> +        super().exec(selenium_ctx)
> +
> +        LOGGER.debug('  Visiting \'%s\'', self.url)
> +        self.driver.get(self.url)
> +
> +        r = requests.get(self.url)
> +        if r.status_code != self.expected_result:
> +            LOGGER.debug('  HTTP Status Code \'%s\' is different '
> +                         'from the expected \'%s\'', r.status_cod, self.url)
> +            return False
> +
> +        LOGGER.debug('  HTTP Status Code is same as expected \'%s\'',
> +                     r.status_code)
> +        return True
> +
> +
> +class CheckText(SeleniumCommand):
> +    def __init__(self, _id, text, expected_result=True):
> +        self._id = _id
> +        self.text = text
> +        self.expected_result = expected_result
> +
> +    def exec(self, selenium_ctx):
> +        super().exec(selenium_ctx)
> +
> +        try:
> +            text = self.driver.find_element_by_id(self._id).text
> +        except Exception:  # TODO: Use proper Exception
> +            return False
> +
> +        LOGGER.debug(
> +            '  Searching for \'%s\' in \'id:%s\'', self.text, self._id)
> +
> +        result = True
> +        if self.text not in text:
> +            LOGGER.error(
> +                '  \'%s\' not found in id \'%s\' with text \'%s\'', self.text,
> +                self._id, text)
> +            result = False
> +
> +        LOGGER.debug('  \'%s\' was found', self.text)
> +
> +        return result == self.expected_result
> +
> +
> +class ShExpect():
> +    BASH_PATTERN = 'test_run_pr1:#'
> +    COMMAND_RESULT_PATTERN = re.compile('^([0-9]+)', re.M)
> +    OUTPUT_VARIABLE = 'cmd_output'
> +    COMMAND_OUTPUT_DELIM = ':test_run_cmd_out:'
> +    COMMAND_OUTPUT_PATTERN = re.compile(
> +        r'^{0}(.*){0}\s+{1}'.format(
> +            COMMAND_OUTPUT_DELIM, BASH_PATTERN), re.M | re.S)
> +
> +    def __init__(self, cmd, expected_output=None, expected_result=0):
> +        self.cmd = cmd
> +        self.expected_result = expected_result
> +        self.expected_output = expected_output
> +
> +    def exec(self, pexpect_ctx):
> +        self.client = pexpect_ctx.client
> +
> +        LOGGER.debug('Executing command \'%s\'', self.cmd)
> +        try:
> +            self.client.sendline('{}=$({} 2>&1)'.format(
> +                self.OUTPUT_VARIABLE, self.cmd))
> +            self.client.expect(self.BASH_PATTERN)
> +
> +            self.client.sendline('echo $?')
> +            self.client.expect(self.COMMAND_RESULT_PATTERN)
> +            result = int(self.client.match.group(1))
> +
> +            self.client.sendline(
> +                'echo "{0}${{{1}}}{0}"'.format(
> +                    self.COMMAND_OUTPUT_DELIM,
> +                    self.OUTPUT_VARIABLE))
> +            self.client.expect(self.COMMAND_OUTPUT_PATTERN)
> +            out = self.client.match.group(1)
> +
> +            if result != self.expected_result:
> +                LOGGER.error('The command \'%s\' returned the code \'%d\', '
> +                             'but the expected code is \'%d\''
> +                             '\nCommand output: \'%s\'',
> +                             self.cmd, result, self.expected_result, out)
> +                return False
> +            if self.expected_output is not None and \
> +                    re.search(self.expected_output, out) is None:
> +                LOGGER.error('Wrong output for command \'%s\'. '
> +                             'Expected \'%s\'\nReceived \'%s\'',
> +                             self.cmd, self.expected_output, out)
> +                return False
> +        except pexpect.exceptions.TIMEOUT:
> +            LOGGER.error('Timeout for command \'%s\'', self.cmd)
> +            return False
> +        except pexpect.exceptions.EOF:
> +            LOGGER.error('Lost connection with docker. Aborting')
> +            return False
> +        return True
> +
> +
> +class FuegoContainer:
> +    def __init__(self, install_script, image_name, container_name,
> +                 jenkins_port):
> +        self.install_script = install_script
> +        self.image_name = image_name
> +        self.container_name = container_name
> +        self.jenkins_port = jenkins_port
> +
> +        self.docker_client = docker.APIClient()
> +        self.container = self.setup_docker()
> +
> +    def __del__(self):
> +        if self.container:
> +            LOGGER.debug('Removing Container')
> +            self.container.remove(force=True)
> +
> +    def stop(self):
> +        self.container.remove(force=True)
> +        self.container = None
> +
> +    def setup_docker(self):
> +        cmd = './{} {}'.format(self.install_script, self.image_name)
I'm not sure I like mixing string formatting styles.
I'm not sure which is more prevalent in this code, but the rest
of Fuego uses '%s'.  Please change this to:
cmd = "./%s %s" % (self.install_script, self.image_name)

> +        LOGGER.debug('Running \'%s\' to install the docker image. '
> +                     'This may take a while....', cmd)
> +        status = subprocess.call(cmd, shell=True)
> +        if status != 0:
> +            return None
> +        docker_client = docker.from_env()
> +        containers = docker_client.containers.list(
> +            all=True, filters={'name': self.container_name})
> +        if containers:
> +            LOGGER.debug(
> +                'Erasing the container \'%s\', so a new one can be created',
Please use " (double-quote) on the outside of this string, to avoid having
to escape the internal single-quotes.

> +                self.container_name)
> +            containers[0].remove(force=True)
> +
> +        container = docker_client.containers.create(
> +            self.image_name,
> +            stdin_open=True, tty=True, network_mode='bridge',
> +            name=self.container_name, command='/bin/bash')
> +        LOGGER.debug('Container \'%s\' created', self.container_name)
Please use " (double-quote) on the outside of this string, to avoid having
to escape the internal single-quotes.

> +        return container
> +
> +    def is_running(self):
> +        try:
> +            container_status = self.docker_client.\
> +                inspect_container(self.container_name)['State']['Running']
> +        except KeyError:
> +            return False
> +
> +        return container_status
> +
> +    def get_ip(self):
> +        container_addr = None
> +
> +        if not self.is_running():
> +            return None
> +
> +        def fetch_ip():
> +            nonlocal container_addr
> +            try:
> +                container_addr = self.docker_client.\
> +                    inspect_container(
> +                        self.container_name)['NetworkSettings']['IPAddress']
> +            except KeyError:
> +                return False
> +
> +            return False if container_addr is None else True
I'd rather this was:
if container_addr is None:
	return False
else
	return True
I'm not a big fan of reordering the conditional logic, even to make the
code less verbose.

> +
> +        if not loop_until_timeout(fetch_ip, timeout=10):
> +            LOGGER.error('Could not fetch the container IP address')
> +            return None
> +
> +        return container_addr
> +
> +    def get_url(self):
> +        container_addr = self.get_ip()
> +
> +        if container_addr:
> +            return 'http://{}:{}/fuego/'.\
> +                format(container_addr, self.jenkins_port)
Please use '%s" and the string format operator '%'.

> +        else:
> +            return None
> +
> +
> +class PexpectContainerSession():
> +    def __init__(self, container, start_script, timeout):
> +        self.container = container
> +        self.start_script = start_script
> +        self.timeout = timeout
> +
> +    def start(self):
> +        LOGGER.debug(
> +            'Starting container \'%s\'', self.container.container_name)
Please use " (double-quote) on the outside of this string, to avoid having
to escape the internal single-quotes.

> +        self.client = pexpect.spawnu(
> +            '{} {}'.format(
> +                self.start_script, self.container.container_name),
> +            echo=False, timeout=self.timeout)
> +
> +        PexpectContainerSession.set_ps1(self.client)
> +
> +        if not self.wait_for_jenkins():
> +            return False
> +
> +        return True
> +
> +    def __del__(self):
> +        self.client.terminate(force=True)
> +
> +    @staticmethod
> +    def set_ps1(client):
> +        client.sendline('export PS1="{}"'.format(ShExpect.BASH_PATTERN))
Please use '%s" and the string format operator '%'.

> +        client.expect(ShExpect.BASH_PATTERN)
> +
> +    def wait_for_jenkins(self):
> +        def ping_jenkins():
> +            try:
> +                conn = http.client.HTTPConnection(container_addr,
> +                                                  self.container.jenkins_port,
> +                                                  timeout=30)
> +                conn.request('HEAD', '/fuego/')
> +                resp = conn.getresponse()
> +                version = resp.getheader('X-Jenkins')
> +                status = resp.status
> +                conn.close()
> +                LOGGER.debug(
> +                    '  HTTP Response code: \'%d\' - Jenkins Version: \'%s\'',
> +                    status, version)
> +                if status == http.client.OK and version is not None:
> +                    return True
> +            except (ConnectionRefusedError, OSError):
> +                return False
> +
> +            return False
> +
> +        container_addr = self.container.get_ip()
> +        if container_addr is None:
> +            return False
> +        LOGGER.debug('Trying to reach jenkins at container \'%s\' via '
> +                     'the container\'s IP \'%s\' at port \'%d\'',
Please use " (double-quote) on the outside of this string, to avoid having
to escape the internal single-quotes.

> +                     self.container.container_name,
> +                     container_addr, self.container.jenkins_port)
> +        if not loop_until_timeout(ping_jenkins, 10):
> +            LOGGER.error('Could not connect to jenkins')
> +            return False
> +
> +        return True
> +
> +
> +class SeleniumContainerSession():
> +    def __init__(self, container):
> +        self.container = container
> +        self.driver = None
> +        self.root_url = container.get_url()
> +
> +    def start(self):
> +        options = webdriver.ChromeOptions()
> +        options.add_argument('headless')
> +        options.add_argument('no-sandbox')
> +        options.add_argument('window-size=1200x600')
> +
> +        self.driver = webdriver.Chrome(chrome_options=options)
> +        self.driver.get(self.root_url)
> +
> +        self.driver.get(self.root_url)
> +        LOGGER.debug('Started a Selenium Session on %s', self.root_url)
> +        return True
> +
> +
> +def main():
> +    DEFAULT_TIMEOUT = 120
> +    DEFAULT_IMAGE_NAME = 'fuego-release'
> +    DEFAULT_CONTAINER_NAME = 'fuego-release-container'
> +    DEFAULT_INSTALL_SCRIPT = 'install.sh'
> +    DEFAULT_START_SCRIPT = 'fuego-host-scripts/docker-start-container.sh'
> +    DEFAULT_JENKINS_PORT = 8080
> +
> +    def execute_tests(timeout):
> +        LOGGER.debug('Starting tests')
> +
> +        ctx_mapper = {
> +            ShExpect: pexpect_session,
> +            SeleniumCommand: selenium_session
> +        }
> +
> +        tests_ok = True
> +        for cmd in COMMANDS_TO_TEST:
> +            for base_class, ctx in ctx_mapper.items():
> +                if isinstance(cmd, base_class):
> +                    if not cmd.exec(ctx):
> +                        tests_ok = False
> +                        break
> +
> +        if tests_ok:
> +            LOGGER.debug('Tests finished.')
> +
> +        return tests_ok
> +
> +    parser = argparse.ArgumentParser()
> +    parser.add_argument('working_dir', help='The working directory',
> type=str)
> +    parser.add_argument('-s', '--install-script',
> +                        help='The script that will be used to install the '
> +                        'docker image. Defaults to \'{}\''
> +                        .format(DEFAULT_INSTALL_SCRIPT),
Please use + for strings concatenated at the end.

> +                        default=DEFAULT_INSTALL_SCRIPT,
> +                        type=str)
> +    parser.add_argument('-a', '--start-script',
> +                        help='The script used to start the container. '
Please use " (double-quote) on the outside of this string, to avoid having
to escape the internal single-quotes.

> +                        'Defaults to \'{}\''
Please use + for strings concatenated at the end.

> +                        .format(DEFAULT_START_SCRIPT),
> +                        default=DEFAULT_START_SCRIPT,
> +                        type=str)
> +    parser.add_argument('-i', '--image-name',
> default=DEFAULT_IMAGE_NAME,
> +                        help='The image name that should be used. '
> +                        'Defaults to \'{}\''
> +                        .format(DEFAULT_IMAGE_NAME), type=str)
Please use + for strings concatenated at the end.

> +    parser.add_argument('-c', '--container-name',
> +                        default=DEFAULT_CONTAINER_NAME,
> +                        help='The container name that should be used for the '
> +                        'test. Defaults to \'{}\''
> +                        .format(DEFAULT_CONTAINER_NAME),
Please use + for strings concatenated at the end.

> +                        type=str)
> +    parser.add_argument('-t', '--timeout', help='The timeout value for '
> +                        'commands. Defaults to {}'
> +                        .format(DEFAULT_TIMEOUT),
Please use + for strings concatenated at the end.

> +                        default=DEFAULT_TIMEOUT, type=int)
> +    parser.add_argument('-j', '--jenkins-port',
> +                        help='The port where the jenkins is running on the '
> +                        'test container. Defaults to {}'
> +                        .format(DEFAULT_JENKINS_PORT),
Please use + for strings concatenated at the end.

> +                        default=DEFAULT_JENKINS_PORT, type=int)
> +    args = parser.parse_args()
> +
> +    LOGGER.debug('Changing working dir to \'%s\'', args.working_dir)
Please use " (double-quote) on the outside of this string, to avoid having
to escape the internal single-quotes.

> +    os.chdir(args.working_dir)
> +
> +    container = FuegoContainer(args.install_script, args.image_name,
> +                               args.container_name, args.jenkins_port)
> +
> +    pexpect_session = PexpectContainerSession(container, args.start_script,
> +                                              args.timeout)
> +    if not pexpect_session.start():
> +        return 1
> +
> +    selenium_session = SeleniumContainerSession(container)
> +    if not selenium_session.start():
> +        return 1
> +
> +    COMMANDS_TO_TEST = [
> +        ShExpect(
> +            'echo $\'hello\n\n\nfrom\n\n\ncontainer\'',
Please use " (double-quote) on the outside of this string, to avoid having
to escape the internal single-quotes.  (or vice-versa)

> +            r'hello\s+from\s+container'),
> +        ShExpect(
> +            'cat -ThisOptionDoesNotExists', expected_result=1),
> +        ShExpect('ftc add-nodes docker'),
> +        ShExpect(
> +            'ftc list-nodes', r'Jenkins nodes in this system:\s*docker\s*.*'),
> +        ShExpect('ftc add-jobs -b docker -p testplan_docker'),
> +        ShExpect(
> +            'ftc list-jobs', r'Jenkins jobs in this system:(\s*docker\..*)+'),
> +        Visit(url=container.get_url()),
> +        CheckText(_id='executors', text='docker'),
> +        CheckText(_id='executors', text='master')
> +    ]
> +
> +    if not execute_tests(args.timeout):
> +        return 1
> +
> +    return 0
> +
> +
> +if __name__ == '__main__':
> +    sys.exit(main())
> --
> 2.16.2

Thanks.
 -- Tim

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 02/16] Mount fuego-rw/ro/core into the fuego-under-test container
  2018-03-29  0:08 ` [Fuego] [PATCH 02/16] Mount fuego-rw/ro/core into the fuego-under-test container Guilherme Campos Camargo
@ 2018-03-30 22:31   ` Tim.Bird
  2018-04-03  3:18     ` Guilherme Camargo
  0 siblings, 1 reply; 40+ messages in thread
From: Tim.Bird @ 2018-03-30 22:31 UTC (permalink / raw)
  To: guicc, fuego


One question inline...

> -----Original Message-----
> From Guilherme Campos Camargo
> This patch implements the necessary logic for mounting fuego-rw,
> fuego-ro and fuego-core into fuego.
> 
> Given that the dockerd socket is shared between the host and the fuego
> container, the mountpoints that are passed to the `docker create` call
> need to be provided as full paths relative to the *host* (and not
> relative to fuego, as would be expected if the docker daemons were
> independent)
> 
> Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
> ---
>  engine/tests/Functional.fuegotest/test_run.py | 63
> +++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/engine/tests/Functional.fuegotest/test_run.py
> b/engine/tests/Functional.fuegotest/test_run.py
> index 96fcfb7..7eae056 100755
> --- a/engine/tests/Functional.fuegotest/test_run.py
> +++ b/engine/tests/Functional.fuegotest/test_run.py
> @@ -173,12 +173,43 @@ class FuegoContainer:
>          self.container = None
> 
>      def setup_docker(self):
> +        def this_container_id():
> +            with open('/proc/self/cgroup', 'rt') as f:
> +                f_text = f.read()
> +
> +                if 'docker' not in f_text:
> +                    return None
> +
> +                for c in self.docker_client.containers(quiet=True):
> +                    if c['Id'] in f_text:
> +                        return c['Id']
> +
> +                return None
> +
> +        def map_to_host(mounts, container_id):
> +            host_mounts = self.docker_client.\
> +                inspect_container(container_id)['Mounts']
> +
> +            for mount in mounts:
> +                LOGGER.debug('  Trying to find %s mountpoint in the host',
> mount['source'])
I assume from the usage here that LOGGER.debug does
string formatting?

> +                for host_mount in host_mounts:
> +                    if mount['source'].startswith(host_mount['Destination']):
> +                        mount['source'] = mount['source'].\
> +                            replace(host_mount['Destination'],
> +                                    host_mount['Source'], 1)
> +                        LOGGER.debug('    Found: %s', mount['source'])
> +                        break
> +                else:
> +                    LOGGER.debug('    Not Found')
> +                    mount['source'] = None
> +
>          cmd = './{} {}'.format(self.install_script, self.image_name)
>          LOGGER.debug('Running \'%s\' to install the docker image. '
>                       'This may take a while....', cmd)
>          status = subprocess.call(cmd, shell=True)
>          if status != 0:
>              return None
> +
>          docker_client = docker.from_env()
>          containers = docker_client.containers.list(
>              all=True, filters={'name': self.container_name})
> @@ -188,10 +219,42 @@ class FuegoContainer:
>                  self.container_name)
>              containers[0].remove(force=True)
> 
> +        mounts = [
> +            {'source': os.path.abspath('./fuego-rw'),
> +             'destination':  '/fuego-rw',
> +             'readonly': False,
> +             },
> +            {'source': os.path.abspath('./fuego-ro'),
> +             'destination':  '/fuego-ro',
> +             'readonly': True,
> +             },
> +            {'source': os.path.abspath('../fuego-core'),
> +             'destination':  '/fuego-core',
> +             'readonly': True,
> +             },
> +        ]
> +
> +        our_id = this_container_id()
> +
> +        if our_id:
> +            LOGGER.debug('Running inside the Docker container %s', our_id)
> +            map_to_host(mounts, our_id)
> +        else:
> +            LOGGER.debug('Not running inside a Docker container')
> +
> +        LOGGER.debug('Creating container with the following mountpoints:')
> +        LOGGER.debug('  %s', mounts)
> +
>          container = docker_client.containers.create(
>              self.image_name,
>              stdin_open=True, tty=True, network_mode='bridge',
> +            mounts=[docker.types.Mount(m['destination'],
> +                                       m['source'],
> +                                       type='bind',
> +                                       read_only=m['readonly'])
> +                    for m in mounts],
>              name=self.container_name, command='/bin/bash')
> +
>          LOGGER.debug('Container \'%s\' created', self.container_name)
>          return container
> 
> --
> 2.16.2


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 01/16] Add fuego-release Functional test
  2018-03-30 22:17   ` Tim.Bird
@ 2018-03-30 22:37     ` Tim.Bird
  2018-04-03  3:18       ` Guilherme Camargo
  2018-04-03  3:17     ` Guilherme Camargo
  1 sibling, 1 reply; 40+ messages in thread
From: Tim.Bird @ 2018-03-30 22:37 UTC (permalink / raw)
  To: Tim.Bird, guicc, fuego



> -----Original Message-----
> From: Tim Bird
> My strategy is to apply all your patches, but ask for modifications,
> that you can provide as patches on top of this set.
> 
> If I make these changes, they will break other patches in the
> series, which will be obnoxious and time-consuming to fix.
> 
> See below inline for comments.
>  -- Tim
> 
> > -----Original Message-----
> > From: Guilherme Campos Camargo
> > Allows Fuego to test new Fuego releases.
> >
> > The fuego and fuego-core repositores and branches to be tested may be
> > configured on the specs.json file. Their defaults are:
> >
> > "FUEGOREPO":"https://bitbucket.org/tbird20d/fuego",
> > "FUEGOBRANCH":"master",
> > "FUEGOCOREREPO":"https://bitbucket.org/tbird20d/fuego-core",
> > "FUEGOCOREBRANCH":"master"
> >
> Can you please add underscores to these names:
> FUEGO_REPO, FUEGO_CORE_BRANCH, etc.
> ?
> 
> It's very nice to be able to customize these in the spec file.
> 
> > Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
> > ---
> >  engine/tests/Functional.fuegotest/fuego_test.sh |  29 ++
> >  engine/tests/Functional.fuegotest/spec.json     |  11 +
> >  engine/tests/Functional.fuegotest/test_run.py   | 427
> > ++++++++++++++++++++++++
> >  3 files changed, 467 insertions(+)
> >  create mode 100755 engine/tests/Functional.fuegotest/fuego_test.sh
> >  create mode 100644 engine/tests/Functional.fuegotest/spec.json
> >  create mode 100755 engine/tests/Functional.fuegotest/test_run.py
> >
> > diff --git a/engine/tests/Functional.fuegotest/fuego_test.sh
> > b/engine/tests/Functional.fuegotest/fuego_test.sh
> > new file mode 100755
> > index 0000000..192c15b
> > --- /dev/null
> > +++ b/engine/tests/Functional.fuegotest/fuego_test.sh
> > @@ -0,0 +1,29 @@
> > +#!/bin/bash
> > +
> > +set -e
> > +
> > +readonly fuego_release_dir=fuego-release
> > +
> > +function test_build {
> > +    if [ -d ${fuego_release_dir} ]; then
> > +        rm -r ${fuego_release_dir}
> > +    fi
> > +    git clone --quiet --depth 1 --single-branch \
> > +        --branch "${FUNCTIONAL_FUEGOTEST_FUEGOBRANCH}" \
> > +        "${FUNCTIONAL_FUEGOTEST_FUEGOREPO}" \

I realize now that my suggestion to change the test name and
add underscores is going to make some really long variable names.

e.g. FUNCTIONAL_FUEGO_RELEASE_TEST_FUEGO_CORE_REPO.

Yikes.  Oh well, let me know what you think.  I still think the
name has more clarity, but maybe it could be shortened inside
the test?

> > +        "${fuego_release_dir}/fuego"
> > +    git clone --quiet --depth 1 --single-branch \
> > +        --branch "${FUNCTIONAL_FUEGOTEST_FUEGOCOREBRANCH}" \
> > +        "${FUNCTIONAL_FUEGOTEST_FUEGOCOREREPO}" \
> > +        "${fuego_release_dir}/fuego-core"
> > +    cd -
> > +}
> > +
> > +function test_run {
> > +    sudo -n ${TEST_HOME}/test_run.py "${fuego_release_dir}/fuego"
> > +    report "echo ok 1 minimal test on target"
> > +}
> > +
> > +function test_processing {
> > +    log_compare "$TESTDIR" "1" "^ok" "p"
> > +}
> > diff --git a/engine/tests/Functional.fuegotest/spec.json
> > b/engine/tests/Functional.fuegotest/spec.json
> > new file mode 100644
> > index 0000000..cc345ba
> > --- /dev/null
> > +++ b/engine/tests/Functional.fuegotest/spec.json
> > @@ -0,0 +1,11 @@
> > +{
> > +    "testName": "Functional.fuegotest",
> > +    "specs": {
> > +        "default": {
> > +            "FUEGOREPO":"https://bitbucket.org/tbird20d/fuego",
> > +            "FUEGOBRANCH":"master",
> > +            "FUEGOCOREREPO":"https://bitbucket.org/tbird20d/fuego-core",
> > +            "FUEGOCOREBRANCH":"master"
> > +        }
> Can you add specs for my 'test' and 'next' branches as well?
> 
> > +    }
> > +}
> > diff --git a/engine/tests/Functional.fuegotest/test_run.py
> > b/engine/tests/Functional.fuegotest/test_run.py
> > new file mode 100755
> > index 0000000..96fcfb7
> > --- /dev/null
> > +++ b/engine/tests/Functional.fuegotest/test_run.py
> > @@ -0,0 +1,427 @@
> > +#!/usr/bin/env python3
> > +import argparse
> > +import http
> > +import http.client
> > +import logging
> > +import os
> > +import re
> > +import subprocess
> > +import sys
> > +import time
> > +
> > +import docker
> > +import pexpect
> > +import requests
> > +from selenium import webdriver
> > +
> > +LOGGER = logging.getLogger('test_run')
> > +STREAM_HANDLER = logging.StreamHandler()
> > +STREAM_HANDLER.setFormatter(
> > +    logging.Formatter('%(name)s:%(levelname)s: %(message)s'))
> > +LOGGER.setLevel(logging.DEBUG)
> > +LOGGER.addHandler(STREAM_HANDLER)
> > +
> > +
> > +def loop_until_timeout(func, timeout=10, num_tries=5):
> > +    LOGGER.debug('Running %s()', func.__name__)
> > +
> > +    for _ in range(num_tries):
> > +        LOGGER.debug('  Try number %s...', _ + 1)
> > +        if func():
> > +            LOGGER.debug('  Success')
> > +            return True
> > +        time.sleep(timeout/num_tries)
> > +    LOGGER.debug('  Failure')
> > +
> > +    return False
> > +
> > +
> > +class SeleniumCommand:
> > +    def exec(self, selenium_ctx):
> > +        self.driver = selenium_ctx.driver
> > +        self.driver.refresh()
> > +        self.driver.implicitly_wait(3)
> > +        LOGGER.debug('Executing Selenium Command \'%s\'',
> > +                     self.__class__.__name__)
> > +
> > +
> > +class Visit(SeleniumCommand):
> > +    def __init__(self, url, timeout=10, expected_result=200):
> > +        self.url = url
> > +        self.timeout = timeout
> > +        self.expected_result = expected_result
> > +
> > +    def exec(self, selenium_ctx):
> > +        super().exec(selenium_ctx)
> > +
> > +        LOGGER.debug('  Visiting \'%s\'', self.url)
> > +        self.driver.get(self.url)
> > +
> > +        r = requests.get(self.url)
> > +        if r.status_code != self.expected_result:
> > +            LOGGER.debug('  HTTP Status Code \'%s\' is different '
> > +                         'from the expected \'%s\'', r.status_cod, self.url)
> > +            return False
> > +
> > +        LOGGER.debug('  HTTP Status Code is same as expected \'%s\'',
> > +                     r.status_code)
> > +        return True
> > +
> > +
> > +class CheckText(SeleniumCommand):
> > +    def __init__(self, _id, text, expected_result=True):
> > +        self._id = _id
> > +        self.text = text
> > +        self.expected_result = expected_result
> > +
> > +    def exec(self, selenium_ctx):
> > +        super().exec(selenium_ctx)
> > +
> > +        try:
> > +            text = self.driver.find_element_by_id(self._id).text
> > +        except Exception:  # TODO: Use proper Exception
> > +            return False
> > +
> > +        LOGGER.debug(
> > +            '  Searching for \'%s\' in \'id:%s\'', self.text, self._id)
> > +
> > +        result = True
> > +        if self.text not in text:
> > +            LOGGER.error(
> > +                '  \'%s\' not found in id \'%s\' with text \'%s\'', self.text,
> > +                self._id, text)
> > +            result = False
> > +
> > +        LOGGER.debug('  \'%s\' was found', self.text)
> > +
> > +        return result == self.expected_result
> > +
> > +
> > +class ShExpect():
> > +    BASH_PATTERN = 'test_run_pr1:#'
> > +    COMMAND_RESULT_PATTERN = re.compile('^([0-9]+)', re.M)
> > +    OUTPUT_VARIABLE = 'cmd_output'
> > +    COMMAND_OUTPUT_DELIM = ':test_run_cmd_out:'
> > +    COMMAND_OUTPUT_PATTERN = re.compile(
> > +        r'^{0}(.*){0}\s+{1}'.format(
> > +            COMMAND_OUTPUT_DELIM, BASH_PATTERN), re.M | re.S)
> > +
> > +    def __init__(self, cmd, expected_output=None, expected_result=0):
> > +        self.cmd = cmd
> > +        self.expected_result = expected_result
> > +        self.expected_output = expected_output
> > +
> > +    def exec(self, pexpect_ctx):
> > +        self.client = pexpect_ctx.client
> > +
> > +        LOGGER.debug('Executing command \'%s\'', self.cmd)
> > +        try:
> > +            self.client.sendline('{}=$({} 2>&1)'.format(
> > +                self.OUTPUT_VARIABLE, self.cmd))
> > +            self.client.expect(self.BASH_PATTERN)
> > +
> > +            self.client.sendline('echo $?')
> > +            self.client.expect(self.COMMAND_RESULT_PATTERN)
> > +            result = int(self.client.match.group(1))
> > +
> > +            self.client.sendline(
> > +                'echo "{0}${{{1}}}{0}"'.format(
> > +                    self.COMMAND_OUTPUT_DELIM,
> > +                    self.OUTPUT_VARIABLE))
> > +            self.client.expect(self.COMMAND_OUTPUT_PATTERN)
> > +            out = self.client.match.group(1)
> > +
> > +            if result != self.expected_result:
> > +                LOGGER.error('The command \'%s\' returned the code \'%d\', '
> > +                             'but the expected code is \'%d\''
> > +                             '\nCommand output: \'%s\'',
> > +                             self.cmd, result, self.expected_result, out)
> > +                return False
> > +            if self.expected_output is not None and \
> > +                    re.search(self.expected_output, out) is None:
> > +                LOGGER.error('Wrong output for command \'%s\'. '
> > +                             'Expected \'%s\'\nReceived \'%s\'',
> > +                             self.cmd, self.expected_output, out)
> > +                return False
> > +        except pexpect.exceptions.TIMEOUT:
> > +            LOGGER.error('Timeout for command \'%s\'', self.cmd)
> > +            return False
> > +        except pexpect.exceptions.EOF:
> > +            LOGGER.error('Lost connection with docker. Aborting')
> > +            return False
> > +        return True
> > +
> > +
> > +class FuegoContainer:
> > +    def __init__(self, install_script, image_name, container_name,
> > +                 jenkins_port):
> > +        self.install_script = install_script
> > +        self.image_name = image_name
> > +        self.container_name = container_name
> > +        self.jenkins_port = jenkins_port
> > +
> > +        self.docker_client = docker.APIClient()
> > +        self.container = self.setup_docker()
> > +
> > +    def __del__(self):
> > +        if self.container:
> > +            LOGGER.debug('Removing Container')
> > +            self.container.remove(force=True)
> > +
> > +    def stop(self):
> > +        self.container.remove(force=True)
> > +        self.container = None
> > +
> > +    def setup_docker(self):
> > +        cmd = './{} {}'.format(self.install_script, self.image_name)
> I'm not sure I like mixing string formatting styles.
> I'm not sure which is more prevalent in this code, but the rest
> of Fuego uses '%s'.  Please change this to:
> cmd = "./%s %s" % (self.install_script, self.image_name)
> 
> > +        LOGGER.debug('Running \'%s\' to install the docker image. '
> > +                     'This may take a while....', cmd)
> > +        status = subprocess.call(cmd, shell=True)
> > +        if status != 0:
> > +            return None
> > +        docker_client = docker.from_env()
> > +        containers = docker_client.containers.list(
> > +            all=True, filters={'name': self.container_name})
> > +        if containers:
> > +            LOGGER.debug(
> > +                'Erasing the container \'%s\', so a new one can be created',
> Please use " (double-quote) on the outside of this string, to avoid having
> to escape the internal single-quotes.
> 
> > +                self.container_name)
> > +            containers[0].remove(force=True)
> > +
> > +        container = docker_client.containers.create(
> > +            self.image_name,
> > +            stdin_open=True, tty=True, network_mode='bridge',
> > +            name=self.container_name, command='/bin/bash')
> > +        LOGGER.debug('Container \'%s\' created', self.container_name)
> Please use " (double-quote) on the outside of this string, to avoid having
> to escape the internal single-quotes.
> 
> > +        return container
> > +
> > +    def is_running(self):
> > +        try:
> > +            container_status = self.docker_client.\
> > +                inspect_container(self.container_name)['State']['Running']
> > +        except KeyError:
> > +            return False
> > +
> > +        return container_status
> > +
> > +    def get_ip(self):
> > +        container_addr = None
> > +
> > +        if not self.is_running():
> > +            return None
> > +
> > +        def fetch_ip():
> > +            nonlocal container_addr
> > +            try:
> > +                container_addr = self.docker_client.\
> > +                    inspect_container(
> > +                        self.container_name)['NetworkSettings']['IPAddress']
> > +            except KeyError:
> > +                return False
> > +
> > +            return False if container_addr is None else True
> I'd rather this was:
> if container_addr is None:
> 	return False
> else
> 	return True
> I'm not a big fan of reordering the conditional logic, even to make the
> code less verbose.
> 
> > +
> > +        if not loop_until_timeout(fetch_ip, timeout=10):
> > +            LOGGER.error('Could not fetch the container IP address')
> > +            return None
> > +
> > +        return container_addr
> > +
> > +    def get_url(self):
> > +        container_addr = self.get_ip()
> > +
> > +        if container_addr:
> > +            return 'http://{}:{}/fuego/'.\
> > +                format(container_addr, self.jenkins_port)
> Please use '%s" and the string format operator '%'.
> 
> > +        else:
> > +            return None
> > +
> > +
> > +class PexpectContainerSession():
> > +    def __init__(self, container, start_script, timeout):
> > +        self.container = container
> > +        self.start_script = start_script
> > +        self.timeout = timeout
> > +
> > +    def start(self):
> > +        LOGGER.debug(
> > +            'Starting container \'%s\'', self.container.container_name)
> Please use " (double-quote) on the outside of this string, to avoid having
> to escape the internal single-quotes.
> 
> > +        self.client = pexpect.spawnu(
> > +            '{} {}'.format(
> > +                self.start_script, self.container.container_name),
> > +            echo=False, timeout=self.timeout)
> > +
> > +        PexpectContainerSession.set_ps1(self.client)
> > +
> > +        if not self.wait_for_jenkins():
> > +            return False
> > +
> > +        return True
> > +
> > +    def __del__(self):
> > +        self.client.terminate(force=True)
> > +
> > +    @staticmethod
> > +    def set_ps1(client):
> > +        client.sendline('export PS1="{}"'.format(ShExpect.BASH_PATTERN))
> Please use '%s" and the string format operator '%'.
> 
> > +        client.expect(ShExpect.BASH_PATTERN)
> > +
> > +    def wait_for_jenkins(self):
> > +        def ping_jenkins():
> > +            try:
> > +                conn = http.client.HTTPConnection(container_addr,
> > +                                                  self.container.jenkins_port,
> > +                                                  timeout=30)
> > +                conn.request('HEAD', '/fuego/')
> > +                resp = conn.getresponse()
> > +                version = resp.getheader('X-Jenkins')
> > +                status = resp.status
> > +                conn.close()
> > +                LOGGER.debug(
> > +                    '  HTTP Response code: \'%d\' - Jenkins Version: \'%s\'',
> > +                    status, version)
> > +                if status == http.client.OK and version is not None:
> > +                    return True
> > +            except (ConnectionRefusedError, OSError):
> > +                return False
> > +
> > +            return False
> > +
> > +        container_addr = self.container.get_ip()
> > +        if container_addr is None:
> > +            return False
> > +        LOGGER.debug('Trying to reach jenkins at container \'%s\' via '
> > +                     'the container\'s IP \'%s\' at port \'%d\'',
> Please use " (double-quote) on the outside of this string, to avoid having
> to escape the internal single-quotes.
> 
> > +                     self.container.container_name,
> > +                     container_addr, self.container.jenkins_port)
> > +        if not loop_until_timeout(ping_jenkins, 10):
> > +            LOGGER.error('Could not connect to jenkins')
> > +            return False
> > +
> > +        return True
> > +
> > +
> > +class SeleniumContainerSession():
> > +    def __init__(self, container):
> > +        self.container = container
> > +        self.driver = None
> > +        self.root_url = container.get_url()
> > +
> > +    def start(self):
> > +        options = webdriver.ChromeOptions()
> > +        options.add_argument('headless')
> > +        options.add_argument('no-sandbox')
> > +        options.add_argument('window-size=1200x600')
> > +
> > +        self.driver = webdriver.Chrome(chrome_options=options)
> > +        self.driver.get(self.root_url)
> > +
> > +        self.driver.get(self.root_url)
> > +        LOGGER.debug('Started a Selenium Session on %s', self.root_url)
> > +        return True
> > +
> > +
> > +def main():
> > +    DEFAULT_TIMEOUT = 120
> > +    DEFAULT_IMAGE_NAME = 'fuego-release'
> > +    DEFAULT_CONTAINER_NAME = 'fuego-release-container'
> > +    DEFAULT_INSTALL_SCRIPT = 'install.sh'
> > +    DEFAULT_START_SCRIPT = 'fuego-host-scripts/docker-start-
> container.sh'
> > +    DEFAULT_JENKINS_PORT = 8080
> > +
> > +    def execute_tests(timeout):
> > +        LOGGER.debug('Starting tests')
> > +
> > +        ctx_mapper = {
> > +            ShExpect: pexpect_session,
> > +            SeleniumCommand: selenium_session
> > +        }
> > +
> > +        tests_ok = True
> > +        for cmd in COMMANDS_TO_TEST:
> > +            for base_class, ctx in ctx_mapper.items():
> > +                if isinstance(cmd, base_class):
> > +                    if not cmd.exec(ctx):
> > +                        tests_ok = False
> > +                        break
> > +
> > +        if tests_ok:
> > +            LOGGER.debug('Tests finished.')
> > +
> > +        return tests_ok
> > +
> > +    parser = argparse.ArgumentParser()
> > +    parser.add_argument('working_dir', help='The working directory',
> > type=str)
> > +    parser.add_argument('-s', '--install-script',
> > +                        help='The script that will be used to install the '
> > +                        'docker image. Defaults to \'{}\''
> > +                        .format(DEFAULT_INSTALL_SCRIPT),
> Please use + for strings concatenated at the end.
> 
> > +                        default=DEFAULT_INSTALL_SCRIPT,
> > +                        type=str)
> > +    parser.add_argument('-a', '--start-script',
> > +                        help='The script used to start the container. '
> Please use " (double-quote) on the outside of this string, to avoid having
> to escape the internal single-quotes.
> 
> > +                        'Defaults to \'{}\''
> Please use + for strings concatenated at the end.
> 
> > +                        .format(DEFAULT_START_SCRIPT),
> > +                        default=DEFAULT_START_SCRIPT,
> > +                        type=str)
> > +    parser.add_argument('-i', '--image-name',
> > default=DEFAULT_IMAGE_NAME,
> > +                        help='The image name that should be used. '
> > +                        'Defaults to \'{}\''
> > +                        .format(DEFAULT_IMAGE_NAME), type=str)
> Please use + for strings concatenated at the end.
> 
> > +    parser.add_argument('-c', '--container-name',
> > +                        default=DEFAULT_CONTAINER_NAME,
> > +                        help='The container name that should be used for the '
> > +                        'test. Defaults to \'{}\''
> > +                        .format(DEFAULT_CONTAINER_NAME),
> Please use + for strings concatenated at the end.
> 
> > +                        type=str)
> > +    parser.add_argument('-t', '--timeout', help='The timeout value for '
> > +                        'commands. Defaults to {}'
> > +                        .format(DEFAULT_TIMEOUT),
> Please use + for strings concatenated at the end.
> 
> > +                        default=DEFAULT_TIMEOUT, type=int)
> > +    parser.add_argument('-j', '--jenkins-port',
> > +                        help='The port where the jenkins is running on the '
> > +                        'test container. Defaults to {}'
> > +                        .format(DEFAULT_JENKINS_PORT),
> Please use + for strings concatenated at the end.
> 
> > +                        default=DEFAULT_JENKINS_PORT, type=int)
> > +    args = parser.parse_args()
> > +
> > +    LOGGER.debug('Changing working dir to \'%s\'', args.working_dir)
> Please use " (double-quote) on the outside of this string, to avoid having
> to escape the internal single-quotes.
> 
> > +    os.chdir(args.working_dir)
> > +
> > +    container = FuegoContainer(args.install_script, args.image_name,
> > +                               args.container_name, args.jenkins_port)
> > +
> > +    pexpect_session = PexpectContainerSession(container,
> args.start_script,
> > +                                              args.timeout)
> > +    if not pexpect_session.start():
> > +        return 1
> > +
> > +    selenium_session = SeleniumContainerSession(container)
> > +    if not selenium_session.start():
> > +        return 1
> > +
> > +    COMMANDS_TO_TEST = [
> > +        ShExpect(
> > +            'echo $\'hello\n\n\nfrom\n\n\ncontainer\'',
> Please use " (double-quote) on the outside of this string, to avoid having
> to escape the internal single-quotes.  (or vice-versa)
> 
> > +            r'hello\s+from\s+container'),
> > +        ShExpect(
> > +            'cat -ThisOptionDoesNotExists', expected_result=1),
> > +        ShExpect('ftc add-nodes docker'),
> > +        ShExpect(
> > +            'ftc list-nodes', r'Jenkins nodes in this system:\s*docker\s*.*'),
> > +        ShExpect('ftc add-jobs -b docker -p testplan_docker'),
> > +        ShExpect(
> > +            'ftc list-jobs', r'Jenkins jobs in this system:(\s*docker\..*)+'),
> > +        Visit(url=container.get_url()),
> > +        CheckText(_id='executors', text='docker'),
> > +        CheckText(_id='executors', text='master')
> > +    ]
> > +
> > +    if not execute_tests(args.timeout):
> > +        return 1
> > +
> > +    return 0
> > +
> > +
> > +if __name__ == '__main__':
> > +    sys.exit(main())
> > --
> > 2.16.2
> 
> Thanks.
>  -- Tim
> _______________________________________________
> Fuego mailing list
> Fuego@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/fuego

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 06/16] Allow the user to keep the container running after the test
  2018-03-29  0:08 ` [Fuego] [PATCH 06/16] Allow the user to keep the container running after the test Guilherme Campos Camargo
@ 2018-03-30 22:48   ` Tim.Bird
  2018-04-03  3:20     ` Guilherme Camargo
  0 siblings, 1 reply; 40+ messages in thread
From: Tim.Bird @ 2018-03-30 22:48 UTC (permalink / raw)
  To: guicc, fuego



> -----Original Message-----
> From: Guilherme Campos Camargo
> 
> Through the `--remove-test-container` command line argument.

Don't split a sentence in the commit description line and the
body of the commit description.  Also, the argument appears
to be '--no-rm-container'.  Description here was not helpful.

Also, there are extra spaces between 'running' and 'after' in the commit description.
Note sure what happened there.

> 
> Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
> ---
>  engine/tests/Functional.fuegotest/test_run.py | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/engine/tests/Functional.fuegotest/test_run.py
> b/engine/tests/Functional.fuegotest/test_run.py
> index d6bec0c..fa68858 100755
> --- a/engine/tests/Functional.fuegotest/test_run.py
> +++ b/engine/tests/Functional.fuegotest/test_run.py
> @@ -154,16 +154,20 @@ class ShExpect():
> 
>  class FuegoContainer:
>      def __init__(self, install_script, image_name, container_name,
> -                 jenkins_port):
> +                 jenkins_port, rm_after_test=True):
>          self.install_script = install_script
>          self.image_name = image_name
>          self.container_name = container_name
>          self.jenkins_port = jenkins_port
> +        self.rm_after_test = rm_after_test
> 
>      def __del__(self):
>          if self.container:
> -            LOGGER.debug('Removing Container')
> -            self.container.remove(force=True)
> +            if self.rm_after_test:
> +                LOGGER.debug('Removing Container')
> +                self.container.remove(force=True)
> +            else:
> +                LOGGER.debug('Not Removing the test container')
> 
>      def install(self):
>          self.docker_client = docker.APIClient()
> @@ -453,13 +457,18 @@ def main():
>                          'test container. Defaults to {}'
>                          .format(DEFAULT_JENKINS_PORT),
>                          default=DEFAULT_JENKINS_PORT, type=int)
> +    parser.add_argument('--no-rm-container',
> +                        help='Do not remove the container after tests are '
> +                        'complete.',
> +                        dest='rm_test_container', default=True, action='store_false')
>      args = parser.parse_args()
> 
>      LOGGER.debug('Changing working dir to \'%s\'', args.working_dir)
>      os.chdir(args.working_dir)
> 
>      container = FuegoContainer(args.install_script, args.image_name,
> -                               args.container_name, args.jenkins_port)
> +                               args.container_name, args.jenkins_port,
> +                               rm_after_test=args.rm_test_container)
>      if not container.install():
>          return 1
> 
> --
> 2.16.2
> 
> _______________________________________________
> Fuego mailing list
> Fuego@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/fuego

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 07/16] Add Back() Selenium Command
  2018-03-29  0:08 ` [Fuego] [PATCH 07/16] Add Back() Selenium Command Guilherme Campos Camargo
@ 2018-03-30 22:52   ` Tim.Bird
  2018-04-03  3:21     ` Guilherme Camargo
  0 siblings, 1 reply; 40+ messages in thread
From: Tim.Bird @ 2018-03-30 22:52 UTC (permalink / raw)
  To: guicc, fuego



> -----Original Message-----
> From: Guilherme Campos
> Camargo
> Sent: Wednesday, March 28, 2018 5:08 PM
> To: fuego@lists.linuxfoundation.org
> Subject: [Fuego] [PATCH 07/16] Add Back() Selenium Command
> 
> That will go one level back in the browser history.

Please don't split a sentence between the commit summary and commit description.

Patch content is OK.
 -- Tim

> 
> Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
> ---
>  engine/tests/Functional.fuegotest/test_run.py | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/engine/tests/Functional.fuegotest/test_run.py
> b/engine/tests/Functional.fuegotest/test_run.py
> index fa68858..851074f 100755
> --- a/engine/tests/Functional.fuegotest/test_run.py
> +++ b/engine/tests/Functional.fuegotest/test_run.py
> @@ -97,6 +97,16 @@ class CheckText(SeleniumCommand):
>          return result == self.expected_result
> 
> 
> +class Back(SeleniumCommand):
> +    def exec(self, selenium_ctx):
> +        super().exec(selenium_ctx)
> +
> +        LOGGER.debug('  Going back')
> +        self.driver.back()
> +
> +        return True
> +
> +
>  class ShExpect():
>      BASH_PATTERN = 'test_run_pr1:#'
>      COMMAND_RESULT_PATTERN = re.compile('^([0-9]+)', re.M)
> --
> 2.16.2
> 
> _______________________________________________
> Fuego mailing list
> Fuego@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/fuego

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 08/16] Add ClickLink selenium command
  2018-03-29  0:08 ` [Fuego] [PATCH 08/16] Add ClickLink selenium command Guilherme Campos Camargo
@ 2018-03-30 22:53   ` Tim.Bird
  2018-04-03  3:21     ` Guilherme Camargo
  0 siblings, 1 reply; 40+ messages in thread
From: Tim.Bird @ 2018-03-30 22:53 UTC (permalink / raw)
  To: guicc, fuego



> -----Original Message-----
> From: Guilherme Campos Camargo
> 
> Clicks in the link that has the text passed in the argument `linktext`.
> Returns False if no link containing `linktext` is found.
> 
> Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
> ---
>  engine/tests/Functional.fuegotest/test_run.py | 24
> ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/engine/tests/Functional.fuegotest/test_run.py
> b/engine/tests/Functional.fuegotest/test_run.py
> index 851074f..e03853b 100755
> --- a/engine/tests/Functional.fuegotest/test_run.py
> +++ b/engine/tests/Functional.fuegotest/test_run.py
> @@ -13,6 +13,7 @@ import docker
>  import pexpect
>  import requests
>  from selenium import webdriver
> +import selenium.common.exceptions as selenium_exceptions
> 
>  LOGGER = logging.getLogger('test_run')
>  STREAM_HANDLER = logging.StreamHandler()
> @@ -97,6 +98,29 @@ class CheckText(SeleniumCommand):
>          return result == self.expected_result
> 
> 
> +class ClickLink(SeleniumCommand):
> +    def __init__(self, linktext, expected_result=True):
> +        self.linktext = linktext
> +        self.expected_result = expected_result
> +
> +    def exec(self, selenium_ctx):
> +        super().exec(selenium_ctx)
> +
> +        LOGGER.debug(
> +            '  Searching for a link with the text \'%s\'', self.linktext)
Use " to avoid having to escape ' in the string.
> +
> +        try:
> +            link = self.driver.find_element_by_partial_link_text(self.linktext)
> +            link.click()
> +            LOGGER.debug('  Link found and clicked')
> +            result = True
> +        except selenium_exceptions.NoSuchElementException:
> +            LOGGER.error('  Link not found')
> +            result = False
> +
> +        return result == self.expected_result
> +
> +
>  class Back(SeleniumCommand):
>      def exec(self, selenium_ctx):
>          super().exec(selenium_ctx)
> --
> 2.16.2
> 
> _______________________________________________
> Fuego mailing list
> Fuego@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/fuego

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 10/16] Write ok/fail on test report
  2018-03-29  0:08 ` [Fuego] [PATCH 10/16] Write ok/fail on test report Guilherme Campos Camargo
@ 2018-03-30 22:58   ` Tim.Bird
  2018-04-03  3:21     ` Guilherme Camargo
  0 siblings, 1 reply; 40+ messages in thread
From: Tim.Bird @ 2018-03-30 22:58 UTC (permalink / raw)
  To: guicc, fuego



> -----Original Message-----
> From: Guilherme Campos Camargo
> 
> Remove the 'minimal test' that had been copied from an example test.
> 
> Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
> ---
>  engine/tests/Functional.fuegotest/fuego_test.sh | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/engine/tests/Functional.fuegotest/fuego_test.sh
> b/engine/tests/Functional.fuegotest/fuego_test.sh
> index b4755bc..ee60c81 100755
> --- a/engine/tests/Functional.fuegotest/fuego_test.sh
> +++ b/engine/tests/Functional.fuegotest/fuego_test.sh
> @@ -1,7 +1,5 @@
>  #!/bin/bash
> 
> -set -e
> -
>  readonly fuego_release_dir=fuego-release
> 
>  function test_build {
> @@ -26,9 +24,13 @@ function test_build {
> 
>  function test_run {
>      sudo -n ${TEST_HOME}/test_run.py "${fuego_release_dir}/fuego"
> -    report "echo ok 1 minimal test on target"
> +    if [ "${?}" = 0 ]; then
> +        report "echo fuego-test: ok"
> +    else
> +        report "echo fuego-test: fail"
> +    fi

Please keep the output in TAP format, which should look like this:
    report "echo ok 1 fuego release test"
else
    report "echo not ok 1 fuego release test"

>  }
> 
>  function test_processing {
> -    log_compare "$TESTDIR" "1" "^ok" "p"
> +    log_compare "$TESTDIR" "1" "ok$" "p"
We're trying to stay with TAP format where possible.
This line can be left alone.

 -- Tim


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 15/16] Improve Click and Check methods to allow multiple locators
  2018-03-29  0:08 ` [Fuego] [PATCH 15/16] Improve Click and Check methods to allow multiple locators Guilherme Campos Camargo
@ 2018-03-30 23:03   ` Tim.Bird
  2018-04-03  3:22     ` Guilherme Camargo
  0 siblings, 1 reply; 40+ messages in thread
From: Tim.Bird @ 2018-03-30 23:03 UTC (permalink / raw)
  To: guicc, fuego



> -----Original Message-----
> From: Guilherme Campos Camargo
> Subject: [Fuego] [PATCH 15/16] Improve Click and Check methods to allow
> multiple locators

Need more information here.   In general, there should usually be something
in the description of the commit, even if it just rewords the summary line.
But in this case, more text explaining the methods would have been good.

(but the commit is already applied...)
> 
> Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
> ---
>  engine/tests/Functional.fuegotest/test_run.py | 105 ++++++++++++++++--
> --------
>  1 file changed, 65 insertions(+), 40 deletions(-)
> 
> diff --git a/engine/tests/Functional.fuegotest/test_run.py
> b/engine/tests/Functional.fuegotest/test_run.py
> index 0370b6f..d433548 100755
> --- a/engine/tests/Functional.fuegotest/test_run.py
> +++ b/engine/tests/Functional.fuegotest/test_run.py
> @@ -13,6 +13,7 @@ import docker
>  import pexpect
>  import requests
>  from selenium import webdriver
> +from selenium.webdriver.common.by import By
>  import selenium.common.exceptions as selenium_exceptions
> 
>  LOGGER = logging.getLogger('test_run')
> @@ -44,6 +45,46 @@ class SeleniumCommand:
>          LOGGER.debug('Executing Selenium Command \'%s\'',
>                       self.__class__.__name__)
> 
> +    @staticmethod
> +    def check_element_text(element, text):
> +        try:
> +            if text in element.text:
> +                LOGGER.\
> +                    debug('  Text \'%s\' matches element.text \'%s\'',
please use " to avoid escaping '.

> +                          text, element.text)
> +                return True
> +            else:
> +                LOGGER.\
> +                    debug('  Text \'%s\' does not match element.text \'%s\'',
please use " to avoid escaping '.

> +                          text, element.text)
> +                return False
> +        except (selenium_exceptions.ElementNotVisibleException,
> +                selenium_exceptions.NoSuchAttributeException,):
> +            LOGGER.error('  Element has no visible Text')
> +            return False
> +
> +    @staticmethod
> +    def click_element(element):
> +        try:
> +            element.click()
> +            LOGGER.debug('  Element clicked')
> +            return True
> +        except (selenium_exceptions.ElementClickInterceptedException,
> +                selenium_exceptions.ElementNotVisibleException,):
> +            LOGGER.error('  Element is not clickable')
> +            return False
> +
> +    @staticmethod
> +    def find_element(context, locator, pattern):
> +        try:
> +            element = context.driver.find_element(locator, pattern)
> +        except selenium_exceptions.NoSuchElementException:
> +            LOGGER.error('  Element not found')
> +            return None
> +
> +        LOGGER.debug('  Element found')
> +        return element
> +
> 
>  class Visit(SeleniumCommand):
>      def __init__(self, url, timeout=10, expected_result=200):
> @@ -69,55 +110,37 @@ class Visit(SeleniumCommand):
> 
> 
>  class CheckText(SeleniumCommand):
> -    def __init__(self, _id, text, expected_result=True):
> -        self._id = _id
> +    def __init__(self, locator, pattern, text='', expected_result=True):
> +        self.pattern = pattern
> +        self.locator = locator
>          self.text = text
>          self.expected_result = expected_result
> 
>      def exec(self, selenium_ctx):
>          super().exec(selenium_ctx)
> 
> -        try:
> -            text = self.driver.find_element_by_id(self._id).text
> -        except Exception:  # TODO: Use proper Exception
> -            return False
> -
> -        LOGGER.debug(
> -            '  Searching for \'%s\' in \'id:%s\'', self.text, self._id)
> -
> -        result = True
> -        if self.text not in text:
> -            LOGGER.error(
> -                '  \'%s\' not found in id \'%s\' with text \'%s\'', self.text,
> -                self._id, text)
> -            result = False
> -
> -        LOGGER.debug('  \'%s\' was found', self.text)
> +        element = SeleniumCommand.\
> +            find_element(selenium_ctx, self.locator, self.pattern)
> +        if element:
> +            result = SeleniumCommand.check_element_text(element, self.text)
> 
>          return result == self.expected_result
> 
> 
> -class ClickLink(SeleniumCommand):
> -    def __init__(self, linktext, expected_result=True):
> -        self.linktext = linktext
> -        self.expected_result = expected_result
> +class Click(SeleniumCommand):
> +    def __init__(self, locator, pattern):
> +        self.pattern = pattern
> +        self.locator = locator
> 
>      def exec(self, selenium_ctx):
>          super().exec(selenium_ctx)
> 
> -        LOGGER.debug(
> -            '  Searching for a link with the text \'%s\'', self.linktext)
> +        element = SeleniumCommand.\
> +            find_element(selenium_ctx, self.locator, self.pattern)
> +        if element:
> +            return SeleniumCommand.click_element(element)
> 
> -        try:
> -            link = self.driver.find_element_by_partial_link_text(self.linktext)
> -            link.click()
> -            LOGGER.debug('  Link found and clicked')
> -            result = True
> -        except selenium_exceptions.NoSuchElementException:
> -            LOGGER.error('  Link not found')
> -            result = False
> -
> -        return result == self.expected_result
> +        return False
> 
> 
>  class Back(SeleniumCommand):
> @@ -529,25 +552,27 @@ def main():
>          # Add Nodes
>          ShExpect('ftc add-nodes docker'),
>          ShExpect('ftc list-nodes -q', r'.*docker.*'),
> -        CheckText(_id='executors', text='master'),
> -        CheckText(_id='executors', text='docker'),
> +        CheckText(By.ID, 'executors', text='master'),
> +        CheckText(By.ID, 'executors', text='docker'),
> 
>          # Add Fuego TestPlan
>          ShExpect('ftc add-jobs -b docker -p testplan_fuego_tests'),
>          ShExpect('ftc list-jobs', r'.*docker\.testplan_fuego_tests\.batch.*'),
> 
> -        ClickLink(linktext='docker'),
> -        CheckText(_id='projectstatus',
> +        Click(By.PARTIAL_LINK_TEXT, 'docker'),
> +        CheckText(By.ID, 'projectstatus',
>                    text='docker.testplan_fuego_tests.batch'),
>          Back(),
> 
>          # Install Views
>          ShExpect('ftc add-view batch .*.batch'),
> -        CheckText(_id='projectstatus-tabBar', text='batch'),
> +        CheckText(By.ID, 'projectstatus-tabBar',
> +                  text='batch'),
> 
>          # Start Tests
>          ShExpect('ftc build-jobs *.*.Functional.fuego_board_check'),
> -        CheckText(_id='buildQueue', text='Functional.fuego_board_check'),
> +        CheckText(By.ID, 'buildQueue',
> +                  text='Functional.fuego_board_check'),
>      ]
> 
>      if not execute_tests(args.timeout):
> --
> 2.16.2
> 
> _______________________________________________
> Fuego mailing list
> Fuego@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/fuego

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 00/16] Fuego Release Test
  2018-03-30 21:34 ` [Fuego] [PATCH 00/16] Fuego Release Test Tim.Bird
@ 2018-04-03  3:16   ` Guilherme Camargo
  0 siblings, 0 replies; 40+ messages in thread
From: Guilherme Camargo @ 2018-04-03  3:16 UTC (permalink / raw)
  To: Tim.Bird; +Cc: fuego

On Fri, Mar 30, 2018 at 09:34:36PM +0000, Tim.Bird@sony.com wrote:
> Thanks Guilherme, for posting the patches.  I'm excited about this work.
> See comments inline below.
> 
> > -----Original Message-----
> > From: Guilherme Campos Camargo
> > Hello, everyone
> > 
> > This series of patches contains the initial implementation of the Fuego
> > Release Test (Functional.fuegotest).
> 
> Please rename this test to Functional.fuego_release_test.  This fits
> the naming for other Fuego-related tests better.

Sure, Tim. Will do that, thanks.

> 
> I haven't looked at all the patches yet, but if they're for the fuego-core
> repository, and are all in the Functional.fuegotest directory, then
> I plan to put them into my master branch.  Since they're self-contained
> they shouldn't affect other Fuego tests or infrastructure, or interfere
> with any work on the 1.3 release.
> 

Yes, that's the case. The test is self-contained on fuego-core.

> I can also put them into my next branch.
> 
> > This test has been created to allow
> > Fuego to test any version of Fuego, given its git repository and branch.
> > 
> > The test clones the repositories of fuego and fuego-core that are
> > configured on the test specs.json, installs fuego and runs that version
> > of fuego as the docker container fuego-release-container.
> > 
> > The test uses Pexpect, to execute/verify commands in the
> > fuego-release-container shell, and uses SeleniumHQ to interact with the
> > Jenkins web interface, checking if the web interface responds as
> > expected.
> This is nice as it adds some additional materials and examples we can
> use for other tests.
> 
> > 
> > A few wrappers have been implemented on top of Pexpect and SeleniumHQ
> > in
> > order to facilitate the inclusion of new tests.
> > 
> > The default specs.json is currently configured to clone and test fuego
> > and fuego-core from the master branch of the official repository. What
> > can be changed through the specs.json file.
> 
> Sounds good.  I haven't looked yet, but it would be good to have
> specs for 'next', and 'test'.  I use those branches fairly often.
> 
> > # Running
> > 
> > Currently this test requires a modified version of Fuego to be executed,
> > given that it needs to install some dependencies and needs to map the
> > dockerd socket from the host to the fuego container.
> > 
> > The modified version can be found in two different branches on
> > Profusion's fuego fork.
> > 
> >  1 - Branch fuego-test: Just a few commits that are necessary for making
> >  this test work, applied on top of fuego/next. We plan to try to
> >  integrate these commits into fuego/next in the next few days.
> > 
> >  2 - Branch fuego-base-image: A more complex change on fuego, that makes
> >  the necessary changes for allowing it to be shipped as a docker image
> >  through dockerhub.
> > 
> > The steps for each one of the versions above are given below:
> > 
> > ## Building the image (from the branch fuego-test)
> > 
> > To run the test, execute the following commands:
> > 
> > ```
> > git clone --branch fuego-test https://bitbucket.org/profusionmobi/fuego-
> > core.git
> > git clone --branch fuego-test https://bitbucket.org/profusionmobi/fuego.git
> > cd fuego
> > ./install fuego-to-test-fuego
> > ./fuego-host-scripts/docker-create-container.sh fuego-to-test-fuego fuego-
> > to-test-fuego-container
> > ./fuego-host-scripts/docker-start-container.sh fuego-to-test-fuego-
> > container
> > ```
> > 
> > Then, add the fuego-test board and the Functional.fuegotest and start
> > the test through Jenkins (localhost:8080/fuego/)
> > 
> > ```
> > ftc add-nodes fuego-test
> > ftc add-jobs -b fuego-test -t Functional.fuegotest
> > ```
> > 
> > ## Using the modified Fuego Base Image from Dockerhub
> > (fuego-base-image):
> > 
> > You can also use the fuego base image that's being developed in
> > Profusion's fuego-base-image branch in our fork:
> > https://bitbucket.org/profusionmobi/fuego/branch/fuego-base-image
> > 
> > The image is already available on dockerhub and can be
> > downloaded/executed with:
> > 
> > ```
> > docker pull fuegotest/fuego
> > docker run -it \
> >   -p 8080:8080 \
> >   -v $(pwd)/host_fuego_home:/var/fuego_home \
> >   -e JENKINS_UID=$(id -u) \
> >   -e JENKINS_GID=$(id -g) \
> >   -v /var/run/docker.sock:/var/run/docker.sock \
> >   fuegotest/fuego:latest
> > ```
> > 
> > Wait for the shell to be available and add fuego-test board and
> > Functional.fuegotest as explained in the last section.
> > 
> > ```
> > ftc add-nodes fuego-test
> > ftc add-jobs -b fuego-test -t Functional.fuegotest
> > ```
> 
> It's very nice to have detailed instructions for both methods.
> Thanks!
>  -- Tim

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 01/16] Add fuego-release Functional test
  2018-03-30 22:17   ` Tim.Bird
  2018-03-30 22:37     ` Tim.Bird
@ 2018-04-03  3:17     ` Guilherme Camargo
  2018-04-03 20:22       ` Tim.Bird
  1 sibling, 1 reply; 40+ messages in thread
From: Guilherme Camargo @ 2018-04-03  3:17 UTC (permalink / raw)
  To: Tim.Bird; +Cc: fuego

Hello, Tim. Thanks for the the review of this patch.

I'm implementing most of your comments. Please see a few comments
inline.

Also, regarding the string formatting operations - the newer method
str.format() VS the legacy version % - I think we should consider
keeping the newer method for the following reasons:

   1. Python documentation encourages the use of the new method
   (str.format()) instead of the old method - as we can see here https://docs.python.org/release/3.1.5/library/stdtypes.html#old-string-formatting-operations

   2. There are a few advantages of using this new method for complex
   string formatting, specially when a value is used multiple times in
   the string as in:

   ```
   161 COMMAND_OUTPUT_PATTERN = re.compile(
   162     r'^{0}(.*){0}\s+{1}'.format(
   163         COMMAND_OUTPUT_DELIM, BASH_PATTERN), re.M | re.S)
   ```

   or

   ```
   191             "echo '{0}${{{1}}}{0}'".format(
   192                 self.COMMAND_OUTPUT_DELIM,
   193                 self.OUTPUT_VARIABLE))
   ```

   Also, I checked and we're using the newer method consistently
   throughout the test.

But it's your call, no problem if you still prefer using '%', please
just let me know and I can make the change if that's the case.

On Fri, Mar 30, 2018 at 10:17:23PM +0000, Tim.Bird@sony.com wrote:
> 
> My strategy is to apply all your patches, but ask for modifications,
> that you can provide as patches on top of this set.
> 
> If I make these changes, they will break other patches in the
> series, which will be obnoxious and time-consuming to fix.
> 

I aggree. I'll send patches with the fixes on top of these ones.

> See below inline for comments.
>  -- Tim
> 
> > -----Original Message-----
> > From: Guilherme Campos Camargo
> > Allows Fuego to test new Fuego releases.
> > 
> > The fuego and fuego-core repositores and branches to be tested may be
> > configured on the specs.json file. Their defaults are:
> > 
> > "FUEGOREPO":"https://bitbucket.org/tbird20d/fuego",
> > "FUEGOBRANCH":"master",
> > "FUEGOCOREREPO":"https://bitbucket.org/tbird20d/fuego-core",
> > "FUEGOCOREBRANCH":"master"
> > 
> Can you please add underscores to these names:
> FUEGO_REPO, FUEGO_CORE_BRANCH, etc.
> ?

Sure. I also saw your next e-mail on the thread (replied in there).

> 
> It's very nice to be able to customize these in the spec file.
> 
> > Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
> > ---
> >  engine/tests/Functional.fuegotest/fuego_test.sh |  29 ++
> >  engine/tests/Functional.fuegotest/spec.json     |  11 +
> >  engine/tests/Functional.fuegotest/test_run.py   | 427
> > ++++++++++++++++++++++++
> >  3 files changed, 467 insertions(+)
> >  create mode 100755 engine/tests/Functional.fuegotest/fuego_test.sh
> >  create mode 100644 engine/tests/Functional.fuegotest/spec.json
> >  create mode 100755 engine/tests/Functional.fuegotest/test_run.py
> > 
> > diff --git a/engine/tests/Functional.fuegotest/fuego_test.sh
> > b/engine/tests/Functional.fuegotest/fuego_test.sh
> > new file mode 100755
> > index 0000000..192c15b
> > --- /dev/null
> > +++ b/engine/tests/Functional.fuegotest/fuego_test.sh
> > @@ -0,0 +1,29 @@
> > +#!/bin/bash
> > +
> > +set -e
> > +
> > +readonly fuego_release_dir=fuego-release
> > +
> > +function test_build {
> > +    if [ -d ${fuego_release_dir} ]; then
> > +        rm -r ${fuego_release_dir}
> > +    fi
> > +    git clone --quiet --depth 1 --single-branch \
> > +        --branch "${FUNCTIONAL_FUEGOTEST_FUEGOBRANCH}" \
> > +        "${FUNCTIONAL_FUEGOTEST_FUEGOREPO}" \
> > +        "${fuego_release_dir}/fuego"
> > +    git clone --quiet --depth 1 --single-branch \
> > +        --branch "${FUNCTIONAL_FUEGOTEST_FUEGOCOREBRANCH}" \
> > +        "${FUNCTIONAL_FUEGOTEST_FUEGOCOREREPO}" \
> > +        "${fuego_release_dir}/fuego-core"
> > +    cd -
> > +}
> > +
> > +function test_run {
> > +    sudo -n ${TEST_HOME}/test_run.py "${fuego_release_dir}/fuego"
> > +    report "echo ok 1 minimal test on target"
> > +}
> > +
> > +function test_processing {
> > +    log_compare "$TESTDIR" "1" "^ok" "p"
> > +}
> > diff --git a/engine/tests/Functional.fuegotest/spec.json
> > b/engine/tests/Functional.fuegotest/spec.json
> > new file mode 100644
> > index 0000000..cc345ba
> > --- /dev/null
> > +++ b/engine/tests/Functional.fuegotest/spec.json
> > @@ -0,0 +1,11 @@
> > +{
> > +    "testName": "Functional.fuegotest",
> > +    "specs": {
> > +        "default": {
> > +            "FUEGOREPO":"https://bitbucket.org/tbird20d/fuego",
> > +            "FUEGOBRANCH":"master",
> > +            "FUEGOCOREREPO":"https://bitbucket.org/tbird20d/fuego-core",
> > +            "FUEGOCOREBRANCH":"master"
> > +        }
> Can you add specs for my 'test' and 'next' branches as well?
> 

Yes, I'll do that.

> > +    }
> > +}
> > diff --git a/engine/tests/Functional.fuegotest/test_run.py
> > b/engine/tests/Functional.fuegotest/test_run.py
> > new file mode 100755
> > index 0000000..96fcfb7
> > --- /dev/null
> > +++ b/engine/tests/Functional.fuegotest/test_run.py
> > @@ -0,0 +1,427 @@
> > +#!/usr/bin/env python3
> > +import argparse
> > +import http
> > +import http.client
> > +import logging
> > +import os
> > +import re
> > +import subprocess
> > +import sys
> > +import time
> > +
> > +import docker
> > +import pexpect
> > +import requests
> > +from selenium import webdriver
> > +
> > +LOGGER = logging.getLogger('test_run')
> > +STREAM_HANDLER = logging.StreamHandler()
> > +STREAM_HANDLER.setFormatter(
> > +    logging.Formatter('%(name)s:%(levelname)s: %(message)s'))
> > +LOGGER.setLevel(logging.DEBUG)
> > +LOGGER.addHandler(STREAM_HANDLER)
> > +
> > +
> > +def loop_until_timeout(func, timeout=10, num_tries=5):
> > +    LOGGER.debug('Running %s()', func.__name__)
> > +
> > +    for _ in range(num_tries):
> > +        LOGGER.debug('  Try number %s...', _ + 1)
> > +        if func():
> > +            LOGGER.debug('  Success')
> > +            return True
> > +        time.sleep(timeout/num_tries)
> > +    LOGGER.debug('  Failure')
> > +
> > +    return False
> > +
> > +
> > +class SeleniumCommand:
> > +    def exec(self, selenium_ctx):
> > +        self.driver = selenium_ctx.driver
> > +        self.driver.refresh()
> > +        self.driver.implicitly_wait(3)
> > +        LOGGER.debug('Executing Selenium Command \'%s\'',
> > +                     self.__class__.__name__)
> > +
> > +
> > +class Visit(SeleniumCommand):
> > +    def __init__(self, url, timeout=10, expected_result=200):
> > +        self.url = url
> > +        self.timeout = timeout
> > +        self.expected_result = expected_result
> > +
> > +    def exec(self, selenium_ctx):
> > +        super().exec(selenium_ctx)
> > +
> > +        LOGGER.debug('  Visiting \'%s\'', self.url)
> > +        self.driver.get(self.url)
> > +
> > +        r = requests.get(self.url)
> > +        if r.status_code != self.expected_result:
> > +            LOGGER.debug('  HTTP Status Code \'%s\' is different '
> > +                         'from the expected \'%s\'', r.status_cod, self.url)
> > +            return False
> > +
> > +        LOGGER.debug('  HTTP Status Code is same as expected \'%s\'',
> > +                     r.status_code)
> > +        return True
> > +
> > +
> > +class CheckText(SeleniumCommand):
> > +    def __init__(self, _id, text, expected_result=True):
> > +        self._id = _id
> > +        self.text = text
> > +        self.expected_result = expected_result
> > +
> > +    def exec(self, selenium_ctx):
> > +        super().exec(selenium_ctx)
> > +
> > +        try:
> > +            text = self.driver.find_element_by_id(self._id).text
> > +        except Exception:  # TODO: Use proper Exception
> > +            return False
> > +
> > +        LOGGER.debug(
> > +            '  Searching for \'%s\' in \'id:%s\'', self.text, self._id)
> > +
> > +        result = True
> > +        if self.text not in text:
> > +            LOGGER.error(
> > +                '  \'%s\' not found in id \'%s\' with text \'%s\'', self.text,
> > +                self._id, text)
> > +            result = False
> > +
> > +        LOGGER.debug('  \'%s\' was found', self.text)
> > +
> > +        return result == self.expected_result
> > +
> > +
> > +class ShExpect():
> > +    BASH_PATTERN = 'test_run_pr1:#'
> > +    COMMAND_RESULT_PATTERN = re.compile('^([0-9]+)', re.M)
> > +    OUTPUT_VARIABLE = 'cmd_output'
> > +    COMMAND_OUTPUT_DELIM = ':test_run_cmd_out:'
> > +    COMMAND_OUTPUT_PATTERN = re.compile(
> > +        r'^{0}(.*){0}\s+{1}'.format(
> > +            COMMAND_OUTPUT_DELIM, BASH_PATTERN), re.M | re.S)
> > +
> > +    def __init__(self, cmd, expected_output=None, expected_result=0):
> > +        self.cmd = cmd
> > +        self.expected_result = expected_result
> > +        self.expected_output = expected_output
> > +
> > +    def exec(self, pexpect_ctx):
> > +        self.client = pexpect_ctx.client
> > +
> > +        LOGGER.debug('Executing command \'%s\'', self.cmd)
> > +        try:
> > +            self.client.sendline('{}=$({} 2>&1)'.format(
> > +                self.OUTPUT_VARIABLE, self.cmd))
> > +            self.client.expect(self.BASH_PATTERN)
> > +
> > +            self.client.sendline('echo $?')
> > +            self.client.expect(self.COMMAND_RESULT_PATTERN)
> > +            result = int(self.client.match.group(1))
> > +
> > +            self.client.sendline(
> > +                'echo "{0}${{{1}}}{0}"'.format(
> > +                    self.COMMAND_OUTPUT_DELIM,
> > +                    self.OUTPUT_VARIABLE))
> > +            self.client.expect(self.COMMAND_OUTPUT_PATTERN)
> > +            out = self.client.match.group(1)
> > +
> > +            if result != self.expected_result:
> > +                LOGGER.error('The command \'%s\' returned the code \'%d\', '
> > +                             'but the expected code is \'%d\''
> > +                             '\nCommand output: \'%s\'',
> > +                             self.cmd, result, self.expected_result, out)
> > +                return False
> > +            if self.expected_output is not None and \
> > +                    re.search(self.expected_output, out) is None:
> > +                LOGGER.error('Wrong output for command \'%s\'. '
> > +                             'Expected \'%s\'\nReceived \'%s\'',
> > +                             self.cmd, self.expected_output, out)
> > +                return False
> > +        except pexpect.exceptions.TIMEOUT:
> > +            LOGGER.error('Timeout for command \'%s\'', self.cmd)
> > +            return False
> > +        except pexpect.exceptions.EOF:
> > +            LOGGER.error('Lost connection with docker. Aborting')
> > +            return False
> > +        return True
> > +
> > +
> > +class FuegoContainer:
> > +    def __init__(self, install_script, image_name, container_name,
> > +                 jenkins_port):
> > +        self.install_script = install_script
> > +        self.image_name = image_name
> > +        self.container_name = container_name
> > +        self.jenkins_port = jenkins_port
> > +
> > +        self.docker_client = docker.APIClient()
> > +        self.container = self.setup_docker()
> > +
> > +    def __del__(self):
> > +        if self.container:
> > +            LOGGER.debug('Removing Container')
> > +            self.container.remove(force=True)
> > +
> > +    def stop(self):
> > +        self.container.remove(force=True)
> > +        self.container = None
> > +
> > +    def setup_docker(self):
> > +        cmd = './{} {}'.format(self.install_script, self.image_name)
> I'm not sure I like mixing string formatting styles.
> I'm not sure which is more prevalent in this code, but the rest
> of Fuego uses '%s'.  Please change this to:
> cmd = "./%s %s" % (self.install_script, self.image_name)
> 

Would rather keeping str.format() as explained above

> > +        LOGGER.debug('Running \'%s\' to install the docker image. '
> > +                     'This may take a while....', cmd)
> > +        status = subprocess.call(cmd, shell=True)
> > +        if status != 0:
> > +            return None
> > +        docker_client = docker.from_env()
> > +        containers = docker_client.containers.list(
> > +            all=True, filters={'name': self.container_name})
> > +        if containers:
> > +            LOGGER.debug(
> > +                'Erasing the container \'%s\', so a new one can be created',
> Please use " (double-quote) on the outside of this string, to avoid having
> to escape the internal single-quotes.
> 

Sure. I'll use double-quotes in all long strings (that are human
readable) to avoid the need of scaping singlo-quotes.

> > +                self.container_name)
> > +            containers[0].remove(force=True)
> > +
> > +        container = docker_client.containers.create(
> > +            self.image_name,
> > +            stdin_open=True, tty=True, network_mode='bridge',
> > +            name=self.container_name, command='/bin/bash')
> > +        LOGGER.debug('Container \'%s\' created', self.container_name)
> Please use " (double-quote) on the outside of this string, to avoid having
> to escape the internal single-quotes.
> 
> > +        return container
> > +
> > +    def is_running(self):
> > +        try:
> > +            container_status = self.docker_client.\
> > +                inspect_container(self.container_name)['State']['Running']
> > +        except KeyError:
> > +            return False
> > +
> > +        return container_status
> > +
> > +    def get_ip(self):
> > +        container_addr = None
> > +
> > +        if not self.is_running():
> > +            return None
> > +
> > +        def fetch_ip():
> > +            nonlocal container_addr
> > +            try:
> > +                container_addr = self.docker_client.\
> > +                    inspect_container(
> > +                        self.container_name)['NetworkSettings']['IPAddress']
> > +            except KeyError:
> > +                return False
> > +
> > +            return False if container_addr is None else True
> I'd rather this was:
> if container_addr is None:
> 	return False
> else
> 	return True
> I'm not a big fan of reordering the conditional logic, even to make the
> code less verbose.
> 

Done

> > +
> > +        if not loop_until_timeout(fetch_ip, timeout=10):
> > +            LOGGER.error('Could not fetch the container IP address')
> > +            return None
> > +
> > +        return container_addr
> > +
> > +    def get_url(self):
> > +        container_addr = self.get_ip()
> > +
> > +        if container_addr:
> > +            return 'http://{}:{}/fuego/'.\
> > +                format(container_addr, self.jenkins_port)
> Please use '%s" and the string format operator '%'.
> 
> > +        else:
> > +            return None
> > +
> > +
> > +class PexpectContainerSession():
> > +    def __init__(self, container, start_script, timeout):
> > +        self.container = container
> > +        self.start_script = start_script
> > +        self.timeout = timeout
> > +
> > +    def start(self):
> > +        LOGGER.debug(
> > +            'Starting container \'%s\'', self.container.container_name)
> Please use " (double-quote) on the outside of this string, to avoid having
> to escape the internal single-quotes.
> 
> > +        self.client = pexpect.spawnu(
> > +            '{} {}'.format(
> > +                self.start_script, self.container.container_name),
> > +            echo=False, timeout=self.timeout)
> > +
> > +        PexpectContainerSession.set_ps1(self.client)
> > +
> > +        if not self.wait_for_jenkins():
> > +            return False
> > +
> > +        return True
> > +
> > +    def __del__(self):
> > +        self.client.terminate(force=True)
> > +
> > +    @staticmethod
> > +    def set_ps1(client):
> > +        client.sendline('export PS1="{}"'.format(ShExpect.BASH_PATTERN))
> Please use '%s" and the string format operator '%'.
> 
> > +        client.expect(ShExpect.BASH_PATTERN)
> > +
> > +    def wait_for_jenkins(self):
> > +        def ping_jenkins():
> > +            try:
> > +                conn = http.client.HTTPConnection(container_addr,
> > +                                                  self.container.jenkins_port,
> > +                                                  timeout=30)
> > +                conn.request('HEAD', '/fuego/')
> > +                resp = conn.getresponse()
> > +                version = resp.getheader('X-Jenkins')
> > +                status = resp.status
> > +                conn.close()
> > +                LOGGER.debug(
> > +                    '  HTTP Response code: \'%d\' - Jenkins Version: \'%s\'',
> > +                    status, version)
> > +                if status == http.client.OK and version is not None:
> > +                    return True
> > +            except (ConnectionRefusedError, OSError):
> > +                return False
> > +
> > +            return False
> > +
> > +        container_addr = self.container.get_ip()
> > +        if container_addr is None:
> > +            return False
> > +        LOGGER.debug('Trying to reach jenkins at container \'%s\' via '
> > +                     'the container\'s IP \'%s\' at port \'%d\'',
> Please use " (double-quote) on the outside of this string, to avoid having
> to escape the internal single-quotes.
> 
> > +                     self.container.container_name,
> > +                     container_addr, self.container.jenkins_port)
> > +        if not loop_until_timeout(ping_jenkins, 10):
> > +            LOGGER.error('Could not connect to jenkins')
> > +            return False
> > +
> > +        return True
> > +
> > +
> > +class SeleniumContainerSession():
> > +    def __init__(self, container):
> > +        self.container = container
> > +        self.driver = None
> > +        self.root_url = container.get_url()
> > +
> > +    def start(self):
> > +        options = webdriver.ChromeOptions()
> > +        options.add_argument('headless')
> > +        options.add_argument('no-sandbox')
> > +        options.add_argument('window-size=1200x600')
> > +
> > +        self.driver = webdriver.Chrome(chrome_options=options)
> > +        self.driver.get(self.root_url)
> > +
> > +        self.driver.get(self.root_url)
> > +        LOGGER.debug('Started a Selenium Session on %s', self.root_url)
> > +        return True
> > +
> > +
> > +def main():
> > +    DEFAULT_TIMEOUT = 120
> > +    DEFAULT_IMAGE_NAME = 'fuego-release'
> > +    DEFAULT_CONTAINER_NAME = 'fuego-release-container'
> > +    DEFAULT_INSTALL_SCRIPT = 'install.sh'
> > +    DEFAULT_START_SCRIPT = 'fuego-host-scripts/docker-start-container.sh'
> > +    DEFAULT_JENKINS_PORT = 8080
> > +
> > +    def execute_tests(timeout):
> > +        LOGGER.debug('Starting tests')
> > +
> > +        ctx_mapper = {
> > +            ShExpect: pexpect_session,
> > +            SeleniumCommand: selenium_session
> > +        }
> > +
> > +        tests_ok = True
> > +        for cmd in COMMANDS_TO_TEST:
> > +            for base_class, ctx in ctx_mapper.items():
> > +                if isinstance(cmd, base_class):
> > +                    if not cmd.exec(ctx):
> > +                        tests_ok = False
> > +                        break
> > +
> > +        if tests_ok:
> > +            LOGGER.debug('Tests finished.')
> > +
> > +        return tests_ok
> > +
> > +    parser = argparse.ArgumentParser()
> > +    parser.add_argument('working_dir', help='The working directory',
> > type=str)
> > +    parser.add_argument('-s', '--install-script',
> > +                        help='The script that will be used to install the '
> > +                        'docker image. Defaults to \'{}\''
> > +                        .format(DEFAULT_INSTALL_SCRIPT),
> Please use + for strings concatenated at the end.
> 

No problem. Will use the '+' operator.

> > +                        default=DEFAULT_INSTALL_SCRIPT,
> > +                        type=str)
> > +    parser.add_argument('-a', '--start-script',
> > +                        help='The script used to start the container. '
> Please use " (double-quote) on the outside of this string, to avoid having
> to escape the internal single-quotes.
> 
> > +                        'Defaults to \'{}\''
> Please use + for strings concatenated at the end.
> 
> > +                        .format(DEFAULT_START_SCRIPT),
> > +                        default=DEFAULT_START_SCRIPT,
> > +                        type=str)
> > +    parser.add_argument('-i', '--image-name',
> > default=DEFAULT_IMAGE_NAME,
> > +                        help='The image name that should be used. '
> > +                        'Defaults to \'{}\''
> > +                        .format(DEFAULT_IMAGE_NAME), type=str)
> Please use + for strings concatenated at the end.
> 
> > +    parser.add_argument('-c', '--container-name',
> > +                        default=DEFAULT_CONTAINER_NAME,
> > +                        help='The container name that should be used for the '
> > +                        'test. Defaults to \'{}\''
> > +                        .format(DEFAULT_CONTAINER_NAME),
> Please use + for strings concatenated at the end.
> 
> > +                        type=str)
> > +    parser.add_argument('-t', '--timeout', help='The timeout value for '
> > +                        'commands. Defaults to {}'
> > +                        .format(DEFAULT_TIMEOUT),
> Please use + for strings concatenated at the end.
> 
> > +                        default=DEFAULT_TIMEOUT, type=int)
> > +    parser.add_argument('-j', '--jenkins-port',
> > +                        help='The port where the jenkins is running on the '
> > +                        'test container. Defaults to {}'
> > +                        .format(DEFAULT_JENKINS_PORT),
> Please use + for strings concatenated at the end.
> 
> > +                        default=DEFAULT_JENKINS_PORT, type=int)
> > +    args = parser.parse_args()
> > +
> > +    LOGGER.debug('Changing working dir to \'%s\'', args.working_dir)
> Please use " (double-quote) on the outside of this string, to avoid having
> to escape the internal single-quotes.
> 
> > +    os.chdir(args.working_dir)
> > +
> > +    container = FuegoContainer(args.install_script, args.image_name,
> > +                               args.container_name, args.jenkins_port)
> > +
> > +    pexpect_session = PexpectContainerSession(container, args.start_script,
> > +                                              args.timeout)
> > +    if not pexpect_session.start():
> > +        return 1
> > +
> > +    selenium_session = SeleniumContainerSession(container)
> > +    if not selenium_session.start():
> > +        return 1
> > +
> > +    COMMANDS_TO_TEST = [
> > +        ShExpect(
> > +            'echo $\'hello\n\n\nfrom\n\n\ncontainer\'',
> Please use " (double-quote) on the outside of this string, to avoid having
> to escape the internal single-quotes.  (or vice-versa)
> 
> > +            r'hello\s+from\s+container'),
> > +        ShExpect(
> > +            'cat -ThisOptionDoesNotExists', expected_result=1),
> > +        ShExpect('ftc add-nodes docker'),
> > +        ShExpect(
> > +            'ftc list-nodes', r'Jenkins nodes in this system:\s*docker\s*.*'),
> > +        ShExpect('ftc add-jobs -b docker -p testplan_docker'),
> > +        ShExpect(
> > +            'ftc list-jobs', r'Jenkins jobs in this system:(\s*docker\..*)+'),
> > +        Visit(url=container.get_url()),
> > +        CheckText(_id='executors', text='docker'),
> > +        CheckText(_id='executors', text='master')
> > +    ]
> > +
> > +    if not execute_tests(args.timeout):
> > +        return 1
> > +
> > +    return 0
> > +
> > +
> > +if __name__ == '__main__':
> > +    sys.exit(main())
> > --
> > 2.16.2
> 
> Thanks.
>  -- Tim

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 01/16] Add fuego-release Functional test
  2018-03-30 22:37     ` Tim.Bird
@ 2018-04-03  3:18       ` Guilherme Camargo
  2018-04-03 20:03         ` Tim.Bird
  0 siblings, 1 reply; 40+ messages in thread
From: Guilherme Camargo @ 2018-04-03  3:18 UTC (permalink / raw)
  To: Tim.Bird; +Cc: fuego

On Fri, Mar 30, 2018 at 10:37:21PM +0000, Tim.Bird@sony.com wrote:
> 
> 
> > -----Original Message-----
> > From: Tim Bird
> > My strategy is to apply all your patches, but ask for modifications,
> > that you can provide as patches on top of this set.
> > 
> > If I make these changes, they will break other patches in the
> > series, which will be obnoxious and time-consuming to fix.
> > 
> > See below inline for comments.
> >  -- Tim
> > 
> > > -----Original Message-----
> > > From: Guilherme Campos Camargo
> > > Allows Fuego to test new Fuego releases.
> > >
> > > The fuego and fuego-core repositores and branches to be tested may be
> > > configured on the specs.json file. Their defaults are:
> > >
> > > "FUEGOREPO":"https://bitbucket.org/tbird20d/fuego",
> > > "FUEGOBRANCH":"master",
> > > "FUEGOCOREREPO":"https://bitbucket.org/tbird20d/fuego-core",
> > > "FUEGOCOREBRANCH":"master"
> > >
> > Can you please add underscores to these names:
> > FUEGO_REPO, FUEGO_CORE_BRANCH, etc.
> > ?
> > 
> > It's very nice to be able to customize these in the spec file.
> > 
> > > Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
> > > ---
> > >  engine/tests/Functional.fuegotest/fuego_test.sh |  29 ++
> > >  engine/tests/Functional.fuegotest/spec.json     |  11 +
> > >  engine/tests/Functional.fuegotest/test_run.py   | 427
> > > ++++++++++++++++++++++++
> > >  3 files changed, 467 insertions(+)
> > >  create mode 100755 engine/tests/Functional.fuegotest/fuego_test.sh
> > >  create mode 100644 engine/tests/Functional.fuegotest/spec.json
> > >  create mode 100755 engine/tests/Functional.fuegotest/test_run.py
> > >
> > > diff --git a/engine/tests/Functional.fuegotest/fuego_test.sh
> > > b/engine/tests/Functional.fuegotest/fuego_test.sh
> > > new file mode 100755
> > > index 0000000..192c15b
> > > --- /dev/null
> > > +++ b/engine/tests/Functional.fuegotest/fuego_test.sh
> > > @@ -0,0 +1,29 @@
> > > +#!/bin/bash
> > > +
> > > +set -e
> > > +
> > > +readonly fuego_release_dir=fuego-release
> > > +
> > > +function test_build {
> > > +    if [ -d ${fuego_release_dir} ]; then
> > > +        rm -r ${fuego_release_dir}
> > > +    fi
> > > +    git clone --quiet --depth 1 --single-branch \
> > > +        --branch "${FUNCTIONAL_FUEGOTEST_FUEGOBRANCH}" \
> > > +        "${FUNCTIONAL_FUEGOTEST_FUEGOREPO}" \
> 
> I realize now that my suggestion to change the test name and
> add underscores is going to make some really long variable names.
> 
> e.g. FUNCTIONAL_FUEGO_RELEASE_TEST_FUEGO_CORE_REPO.
> 
> Yikes.  Oh well, let me know what you think.  I still think the
> name has more clarity, but maybe it could be shortened inside
> the test?
> 

I agree that the names will become more readable specially on the specs
json by using the underlines and I like the change on the test name. I
don't think that having these huge variables is a big problem, because,
as you said, we can just create an alias for them in the fuego_test.sh

That's what I will do, then.

Also, do we really need the to have the test name as a prefix for specs
variables? If not, we might consider changing that in the future, by
exporting the variables with the same names that they have on the
spec.json.

> > > +        "${fuego_release_dir}/fuego"
> > > +    git clone --quiet --depth 1 --single-branch \
> > > +        --branch "${FUNCTIONAL_FUEGOTEST_FUEGOCOREBRANCH}" \
> > > +        "${FUNCTIONAL_FUEGOTEST_FUEGOCOREREPO}" \
> > > +        "${fuego_release_dir}/fuego-core"
> > > +    cd -
> > > +}
> > > +
> > > +function test_run {
> > > +    sudo -n ${TEST_HOME}/test_run.py "${fuego_release_dir}/fuego"
> > > +    report "echo ok 1 minimal test on target"
> > > +}
> > > +
> > > +function test_processing {
> > > +    log_compare "$TESTDIR" "1" "^ok" "p"
> > > +}
> > > diff --git a/engine/tests/Functional.fuegotest/spec.json
> > > b/engine/tests/Functional.fuegotest/spec.json
> > > new file mode 100644
> > > index 0000000..cc345ba
> > > --- /dev/null
> > > +++ b/engine/tests/Functional.fuegotest/spec.json
> > > @@ -0,0 +1,11 @@
> > > +{
> > > +    "testName": "Functional.fuegotest",
> > > +    "specs": {
> > > +        "default": {
> > > +            "FUEGOREPO":"https://bitbucket.org/tbird20d/fuego",
> > > +            "FUEGOBRANCH":"master",
> > > +            "FUEGOCOREREPO":"https://bitbucket.org/tbird20d/fuego-core",
> > > +            "FUEGOCOREBRANCH":"master"
> > > +        }
> > Can you add specs for my 'test' and 'next' branches as well?
> > 
> > > +    }
> > > +}
> > > diff --git a/engine/tests/Functional.fuegotest/test_run.py
> > > b/engine/tests/Functional.fuegotest/test_run.py
> > > new file mode 100755
> > > index 0000000..96fcfb7
> > > --- /dev/null
> > > +++ b/engine/tests/Functional.fuegotest/test_run.py
> > > @@ -0,0 +1,427 @@
> > > +#!/usr/bin/env python3
> > > +import argparse
> > > +import http
> > > +import http.client
> > > +import logging
> > > +import os
> > > +import re
> > > +import subprocess
> > > +import sys
> > > +import time
> > > +
> > > +import docker
> > > +import pexpect
> > > +import requests
> > > +from selenium import webdriver
> > > +
> > > +LOGGER = logging.getLogger('test_run')
> > > +STREAM_HANDLER = logging.StreamHandler()
> > > +STREAM_HANDLER.setFormatter(
> > > +    logging.Formatter('%(name)s:%(levelname)s: %(message)s'))
> > > +LOGGER.setLevel(logging.DEBUG)
> > > +LOGGER.addHandler(STREAM_HANDLER)
> > > +
> > > +
> > > +def loop_until_timeout(func, timeout=10, num_tries=5):
> > > +    LOGGER.debug('Running %s()', func.__name__)
> > > +
> > > +    for _ in range(num_tries):
> > > +        LOGGER.debug('  Try number %s...', _ + 1)
> > > +        if func():
> > > +            LOGGER.debug('  Success')
> > > +            return True
> > > +        time.sleep(timeout/num_tries)
> > > +    LOGGER.debug('  Failure')
> > > +
> > > +    return False
> > > +
> > > +
> > > +class SeleniumCommand:
> > > +    def exec(self, selenium_ctx):
> > > +        self.driver = selenium_ctx.driver
> > > +        self.driver.refresh()
> > > +        self.driver.implicitly_wait(3)
> > > +        LOGGER.debug('Executing Selenium Command \'%s\'',
> > > +                     self.__class__.__name__)
> > > +
> > > +
> > > +class Visit(SeleniumCommand):
> > > +    def __init__(self, url, timeout=10, expected_result=200):
> > > +        self.url = url
> > > +        self.timeout = timeout
> > > +        self.expected_result = expected_result
> > > +
> > > +    def exec(self, selenium_ctx):
> > > +        super().exec(selenium_ctx)
> > > +
> > > +        LOGGER.debug('  Visiting \'%s\'', self.url)
> > > +        self.driver.get(self.url)
> > > +
> > > +        r = requests.get(self.url)
> > > +        if r.status_code != self.expected_result:
> > > +            LOGGER.debug('  HTTP Status Code \'%s\' is different '
> > > +                         'from the expected \'%s\'', r.status_cod, self.url)
> > > +            return False
> > > +
> > > +        LOGGER.debug('  HTTP Status Code is same as expected \'%s\'',
> > > +                     r.status_code)
> > > +        return True
> > > +
> > > +
> > > +class CheckText(SeleniumCommand):
> > > +    def __init__(self, _id, text, expected_result=True):
> > > +        self._id = _id
> > > +        self.text = text
> > > +        self.expected_result = expected_result
> > > +
> > > +    def exec(self, selenium_ctx):
> > > +        super().exec(selenium_ctx)
> > > +
> > > +        try:
> > > +            text = self.driver.find_element_by_id(self._id).text
> > > +        except Exception:  # TODO: Use proper Exception
> > > +            return False
> > > +
> > > +        LOGGER.debug(
> > > +            '  Searching for \'%s\' in \'id:%s\'', self.text, self._id)
> > > +
> > > +        result = True
> > > +        if self.text not in text:
> > > +            LOGGER.error(
> > > +                '  \'%s\' not found in id \'%s\' with text \'%s\'', self.text,
> > > +                self._id, text)
> > > +            result = False
> > > +
> > > +        LOGGER.debug('  \'%s\' was found', self.text)
> > > +
> > > +        return result == self.expected_result
> > > +
> > > +
> > > +class ShExpect():
> > > +    BASH_PATTERN = 'test_run_pr1:#'
> > > +    COMMAND_RESULT_PATTERN = re.compile('^([0-9]+)', re.M)
> > > +    OUTPUT_VARIABLE = 'cmd_output'
> > > +    COMMAND_OUTPUT_DELIM = ':test_run_cmd_out:'
> > > +    COMMAND_OUTPUT_PATTERN = re.compile(
> > > +        r'^{0}(.*){0}\s+{1}'.format(
> > > +            COMMAND_OUTPUT_DELIM, BASH_PATTERN), re.M | re.S)
> > > +
> > > +    def __init__(self, cmd, expected_output=None, expected_result=0):
> > > +        self.cmd = cmd
> > > +        self.expected_result = expected_result
> > > +        self.expected_output = expected_output
> > > +
> > > +    def exec(self, pexpect_ctx):
> > > +        self.client = pexpect_ctx.client
> > > +
> > > +        LOGGER.debug('Executing command \'%s\'', self.cmd)
> > > +        try:
> > > +            self.client.sendline('{}=$({} 2>&1)'.format(
> > > +                self.OUTPUT_VARIABLE, self.cmd))
> > > +            self.client.expect(self.BASH_PATTERN)
> > > +
> > > +            self.client.sendline('echo $?')
> > > +            self.client.expect(self.COMMAND_RESULT_PATTERN)
> > > +            result = int(self.client.match.group(1))
> > > +
> > > +            self.client.sendline(
> > > +                'echo "{0}${{{1}}}{0}"'.format(
> > > +                    self.COMMAND_OUTPUT_DELIM,
> > > +                    self.OUTPUT_VARIABLE))
> > > +            self.client.expect(self.COMMAND_OUTPUT_PATTERN)
> > > +            out = self.client.match.group(1)
> > > +
> > > +            if result != self.expected_result:
> > > +                LOGGER.error('The command \'%s\' returned the code \'%d\', '
> > > +                             'but the expected code is \'%d\''
> > > +                             '\nCommand output: \'%s\'',
> > > +                             self.cmd, result, self.expected_result, out)
> > > +                return False
> > > +            if self.expected_output is not None and \
> > > +                    re.search(self.expected_output, out) is None:
> > > +                LOGGER.error('Wrong output for command \'%s\'. '
> > > +                             'Expected \'%s\'\nReceived \'%s\'',
> > > +                             self.cmd, self.expected_output, out)
> > > +                return False
> > > +        except pexpect.exceptions.TIMEOUT:
> > > +            LOGGER.error('Timeout for command \'%s\'', self.cmd)
> > > +            return False
> > > +        except pexpect.exceptions.EOF:
> > > +            LOGGER.error('Lost connection with docker. Aborting')
> > > +            return False
> > > +        return True
> > > +
> > > +
> > > +class FuegoContainer:
> > > +    def __init__(self, install_script, image_name, container_name,
> > > +                 jenkins_port):
> > > +        self.install_script = install_script
> > > +        self.image_name = image_name
> > > +        self.container_name = container_name
> > > +        self.jenkins_port = jenkins_port
> > > +
> > > +        self.docker_client = docker.APIClient()
> > > +        self.container = self.setup_docker()
> > > +
> > > +    def __del__(self):
> > > +        if self.container:
> > > +            LOGGER.debug('Removing Container')
> > > +            self.container.remove(force=True)
> > > +
> > > +    def stop(self):
> > > +        self.container.remove(force=True)
> > > +        self.container = None
> > > +
> > > +    def setup_docker(self):
> > > +        cmd = './{} {}'.format(self.install_script, self.image_name)
> > I'm not sure I like mixing string formatting styles.
> > I'm not sure which is more prevalent in this code, but the rest
> > of Fuego uses '%s'.  Please change this to:
> > cmd = "./%s %s" % (self.install_script, self.image_name)
> > 
> > > +        LOGGER.debug('Running \'%s\' to install the docker image. '
> > > +                     'This may take a while....', cmd)
> > > +        status = subprocess.call(cmd, shell=True)
> > > +        if status != 0:
> > > +            return None
> > > +        docker_client = docker.from_env()
> > > +        containers = docker_client.containers.list(
> > > +            all=True, filters={'name': self.container_name})
> > > +        if containers:
> > > +            LOGGER.debug(
> > > +                'Erasing the container \'%s\', so a new one can be created',
> > Please use " (double-quote) on the outside of this string, to avoid having
> > to escape the internal single-quotes.
> > 
> > > +                self.container_name)
> > > +            containers[0].remove(force=True)
> > > +
> > > +        container = docker_client.containers.create(
> > > +            self.image_name,
> > > +            stdin_open=True, tty=True, network_mode='bridge',
> > > +            name=self.container_name, command='/bin/bash')
> > > +        LOGGER.debug('Container \'%s\' created', self.container_name)
> > Please use " (double-quote) on the outside of this string, to avoid having
> > to escape the internal single-quotes.
> > 
> > > +        return container
> > > +
> > > +    def is_running(self):
> > > +        try:
> > > +            container_status = self.docker_client.\
> > > +                inspect_container(self.container_name)['State']['Running']
> > > +        except KeyError:
> > > +            return False
> > > +
> > > +        return container_status
> > > +
> > > +    def get_ip(self):
> > > +        container_addr = None
> > > +
> > > +        if not self.is_running():
> > > +            return None
> > > +
> > > +        def fetch_ip():
> > > +            nonlocal container_addr
> > > +            try:
> > > +                container_addr = self.docker_client.\
> > > +                    inspect_container(
> > > +                        self.container_name)['NetworkSettings']['IPAddress']
> > > +            except KeyError:
> > > +                return False
> > > +
> > > +            return False if container_addr is None else True
> > I'd rather this was:
> > if container_addr is None:
> > 	return False
> > else
> > 	return True
> > I'm not a big fan of reordering the conditional logic, even to make the
> > code less verbose.
> > 
> > > +
> > > +        if not loop_until_timeout(fetch_ip, timeout=10):
> > > +            LOGGER.error('Could not fetch the container IP address')
> > > +            return None
> > > +
> > > +        return container_addr
> > > +
> > > +    def get_url(self):
> > > +        container_addr = self.get_ip()
> > > +
> > > +        if container_addr:
> > > +            return 'http://{}:{}/fuego/'.\
> > > +                format(container_addr, self.jenkins_port)
> > Please use '%s" and the string format operator '%'.
> > 
> > > +        else:
> > > +            return None
> > > +
> > > +
> > > +class PexpectContainerSession():
> > > +    def __init__(self, container, start_script, timeout):
> > > +        self.container = container
> > > +        self.start_script = start_script
> > > +        self.timeout = timeout
> > > +
> > > +    def start(self):
> > > +        LOGGER.debug(
> > > +            'Starting container \'%s\'', self.container.container_name)
> > Please use " (double-quote) on the outside of this string, to avoid having
> > to escape the internal single-quotes.
> > 
> > > +        self.client = pexpect.spawnu(
> > > +            '{} {}'.format(
> > > +                self.start_script, self.container.container_name),
> > > +            echo=False, timeout=self.timeout)
> > > +
> > > +        PexpectContainerSession.set_ps1(self.client)
> > > +
> > > +        if not self.wait_for_jenkins():
> > > +            return False
> > > +
> > > +        return True
> > > +
> > > +    def __del__(self):
> > > +        self.client.terminate(force=True)
> > > +
> > > +    @staticmethod
> > > +    def set_ps1(client):
> > > +        client.sendline('export PS1="{}"'.format(ShExpect.BASH_PATTERN))
> > Please use '%s" and the string format operator '%'.
> > 
> > > +        client.expect(ShExpect.BASH_PATTERN)
> > > +
> > > +    def wait_for_jenkins(self):
> > > +        def ping_jenkins():
> > > +            try:
> > > +                conn = http.client.HTTPConnection(container_addr,
> > > +                                                  self.container.jenkins_port,
> > > +                                                  timeout=30)
> > > +                conn.request('HEAD', '/fuego/')
> > > +                resp = conn.getresponse()
> > > +                version = resp.getheader('X-Jenkins')
> > > +                status = resp.status
> > > +                conn.close()
> > > +                LOGGER.debug(
> > > +                    '  HTTP Response code: \'%d\' - Jenkins Version: \'%s\'',
> > > +                    status, version)
> > > +                if status == http.client.OK and version is not None:
> > > +                    return True
> > > +            except (ConnectionRefusedError, OSError):
> > > +                return False
> > > +
> > > +            return False
> > > +
> > > +        container_addr = self.container.get_ip()
> > > +        if container_addr is None:
> > > +            return False
> > > +        LOGGER.debug('Trying to reach jenkins at container \'%s\' via '
> > > +                     'the container\'s IP \'%s\' at port \'%d\'',
> > Please use " (double-quote) on the outside of this string, to avoid having
> > to escape the internal single-quotes.
> > 
> > > +                     self.container.container_name,
> > > +                     container_addr, self.container.jenkins_port)
> > > +        if not loop_until_timeout(ping_jenkins, 10):
> > > +            LOGGER.error('Could not connect to jenkins')
> > > +            return False
> > > +
> > > +        return True
> > > +
> > > +
> > > +class SeleniumContainerSession():
> > > +    def __init__(self, container):
> > > +        self.container = container
> > > +        self.driver = None
> > > +        self.root_url = container.get_url()
> > > +
> > > +    def start(self):
> > > +        options = webdriver.ChromeOptions()
> > > +        options.add_argument('headless')
> > > +        options.add_argument('no-sandbox')
> > > +        options.add_argument('window-size=1200x600')
> > > +
> > > +        self.driver = webdriver.Chrome(chrome_options=options)
> > > +        self.driver.get(self.root_url)
> > > +
> > > +        self.driver.get(self.root_url)
> > > +        LOGGER.debug('Started a Selenium Session on %s', self.root_url)
> > > +        return True
> > > +
> > > +
> > > +def main():
> > > +    DEFAULT_TIMEOUT = 120
> > > +    DEFAULT_IMAGE_NAME = 'fuego-release'
> > > +    DEFAULT_CONTAINER_NAME = 'fuego-release-container'
> > > +    DEFAULT_INSTALL_SCRIPT = 'install.sh'
> > > +    DEFAULT_START_SCRIPT = 'fuego-host-scripts/docker-start-
> > container.sh'
> > > +    DEFAULT_JENKINS_PORT = 8080
> > > +
> > > +    def execute_tests(timeout):
> > > +        LOGGER.debug('Starting tests')
> > > +
> > > +        ctx_mapper = {
> > > +            ShExpect: pexpect_session,
> > > +            SeleniumCommand: selenium_session
> > > +        }
> > > +
> > > +        tests_ok = True
> > > +        for cmd in COMMANDS_TO_TEST:
> > > +            for base_class, ctx in ctx_mapper.items():
> > > +                if isinstance(cmd, base_class):
> > > +                    if not cmd.exec(ctx):
> > > +                        tests_ok = False
> > > +                        break
> > > +
> > > +        if tests_ok:
> > > +            LOGGER.debug('Tests finished.')
> > > +
> > > +        return tests_ok
> > > +
> > > +    parser = argparse.ArgumentParser()
> > > +    parser.add_argument('working_dir', help='The working directory',
> > > type=str)
> > > +    parser.add_argument('-s', '--install-script',
> > > +                        help='The script that will be used to install the '
> > > +                        'docker image. Defaults to \'{}\''
> > > +                        .format(DEFAULT_INSTALL_SCRIPT),
> > Please use + for strings concatenated at the end.
> > 
> > > +                        default=DEFAULT_INSTALL_SCRIPT,
> > > +                        type=str)
> > > +    parser.add_argument('-a', '--start-script',
> > > +                        help='The script used to start the container. '
> > Please use " (double-quote) on the outside of this string, to avoid having
> > to escape the internal single-quotes.
> > 
> > > +                        'Defaults to \'{}\''
> > Please use + for strings concatenated at the end.
> > 
> > > +                        .format(DEFAULT_START_SCRIPT),
> > > +                        default=DEFAULT_START_SCRIPT,
> > > +                        type=str)
> > > +    parser.add_argument('-i', '--image-name',
> > > default=DEFAULT_IMAGE_NAME,
> > > +                        help='The image name that should be used. '
> > > +                        'Defaults to \'{}\''
> > > +                        .format(DEFAULT_IMAGE_NAME), type=str)
> > Please use + for strings concatenated at the end.
> > 
> > > +    parser.add_argument('-c', '--container-name',
> > > +                        default=DEFAULT_CONTAINER_NAME,
> > > +                        help='The container name that should be used for the '
> > > +                        'test. Defaults to \'{}\''
> > > +                        .format(DEFAULT_CONTAINER_NAME),
> > Please use + for strings concatenated at the end.
> > 
> > > +                        type=str)
> > > +    parser.add_argument('-t', '--timeout', help='The timeout value for '
> > > +                        'commands. Defaults to {}'
> > > +                        .format(DEFAULT_TIMEOUT),
> > Please use + for strings concatenated at the end.
> > 
> > > +                        default=DEFAULT_TIMEOUT, type=int)
> > > +    parser.add_argument('-j', '--jenkins-port',
> > > +                        help='The port where the jenkins is running on the '
> > > +                        'test container. Defaults to {}'
> > > +                        .format(DEFAULT_JENKINS_PORT),
> > Please use + for strings concatenated at the end.
> > 
> > > +                        default=DEFAULT_JENKINS_PORT, type=int)
> > > +    args = parser.parse_args()
> > > +
> > > +    LOGGER.debug('Changing working dir to \'%s\'', args.working_dir)
> > Please use " (double-quote) on the outside of this string, to avoid having
> > to escape the internal single-quotes.
> > 
> > > +    os.chdir(args.working_dir)
> > > +
> > > +    container = FuegoContainer(args.install_script, args.image_name,
> > > +                               args.container_name, args.jenkins_port)
> > > +
> > > +    pexpect_session = PexpectContainerSession(container,
> > args.start_script,
> > > +                                              args.timeout)
> > > +    if not pexpect_session.start():
> > > +        return 1
> > > +
> > > +    selenium_session = SeleniumContainerSession(container)
> > > +    if not selenium_session.start():
> > > +        return 1
> > > +
> > > +    COMMANDS_TO_TEST = [
> > > +        ShExpect(
> > > +            'echo $\'hello\n\n\nfrom\n\n\ncontainer\'',
> > Please use " (double-quote) on the outside of this string, to avoid having
> > to escape the internal single-quotes.  (or vice-versa)
> > 
> > > +            r'hello\s+from\s+container'),
> > > +        ShExpect(
> > > +            'cat -ThisOptionDoesNotExists', expected_result=1),
> > > +        ShExpect('ftc add-nodes docker'),
> > > +        ShExpect(
> > > +            'ftc list-nodes', r'Jenkins nodes in this system:\s*docker\s*.*'),
> > > +        ShExpect('ftc add-jobs -b docker -p testplan_docker'),
> > > +        ShExpect(
> > > +            'ftc list-jobs', r'Jenkins jobs in this system:(\s*docker\..*)+'),
> > > +        Visit(url=container.get_url()),
> > > +        CheckText(_id='executors', text='docker'),
> > > +        CheckText(_id='executors', text='master')
> > > +    ]
> > > +
> > > +    if not execute_tests(args.timeout):
> > > +        return 1
> > > +
> > > +    return 0
> > > +
> > > +
> > > +if __name__ == '__main__':
> > > +    sys.exit(main())
> > > --
> > > 2.16.2
> > 
> > Thanks.
> >  -- Tim
> > _______________________________________________
> > Fuego mailing list
> > Fuego@lists.linuxfoundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/fuego

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 02/16] Mount fuego-rw/ro/core into the fuego-under-test container
  2018-03-30 22:31   ` Tim.Bird
@ 2018-04-03  3:18     ` Guilherme Camargo
  0 siblings, 0 replies; 40+ messages in thread
From: Guilherme Camargo @ 2018-04-03  3:18 UTC (permalink / raw)
  To: Tim.Bird; +Cc: fuego

On Fri, Mar 30, 2018 at 10:31:42PM +0000, Tim.Bird@sony.com wrote:
> 
> One question inline...
> 
> > -----Original Message-----
> > From Guilherme Campos Camargo
> > This patch implements the necessary logic for mounting fuego-rw,
> > fuego-ro and fuego-core into fuego.
> > 
> > Given that the dockerd socket is shared between the host and the fuego
> > container, the mountpoints that are passed to the `docker create` call
> > need to be provided as full paths relative to the *host* (and not
> > relative to fuego, as would be expected if the docker daemons were
> > independent)
> > 
> > Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
> > ---
> >  engine/tests/Functional.fuegotest/test_run.py | 63
> > +++++++++++++++++++++++++++
> >  1 file changed, 63 insertions(+)
> > 
> > diff --git a/engine/tests/Functional.fuegotest/test_run.py
> > b/engine/tests/Functional.fuegotest/test_run.py
> > index 96fcfb7..7eae056 100755
> > --- a/engine/tests/Functional.fuegotest/test_run.py
> > +++ b/engine/tests/Functional.fuegotest/test_run.py
> > @@ -173,12 +173,43 @@ class FuegoContainer:
> >          self.container = None
> > 
> >      def setup_docker(self):
> > +        def this_container_id():
> > +            with open('/proc/self/cgroup', 'rt') as f:
> > +                f_text = f.read()
> > +
> > +                if 'docker' not in f_text:
> > +                    return None
> > +
> > +                for c in self.docker_client.containers(quiet=True):
> > +                    if c['Id'] in f_text:
> > +                        return c['Id']
> > +
> > +                return None
> > +
> > +        def map_to_host(mounts, container_id):
> > +            host_mounts = self.docker_client.\
> > +                inspect_container(container_id)['Mounts']
> > +
> > +            for mount in mounts:
> > +                LOGGER.debug('  Trying to find %s mountpoint in the host',
> > mount['source'])
> I assume from the usage here that LOGGER.debug does
> string formatting?
> 

Yes, Tim. It does. You can see that we have set the formatting settings
on the first lines of the script.

```
19 LOGGER = logging.getLogger('test_run')
20 STREAM_HANDLER = logging.StreamHandler()
21 STREAM_HANDLER.setFormatter(
22    logging.Formatter("%(name)s:%(levelname)s: %(message)s"))
23 LOGGER.setLevel(logging.DEBUG)
24 LOGGER.addHandler(STREAM_HANDLER)

> > +                for host_mount in host_mounts:
> > +                    if mount['source'].startswith(host_mount['Destination']):
> > +                        mount['source'] = mount['source'].\
> > +                            replace(host_mount['Destination'],
> > +                                    host_mount['Source'], 1)
> > +                        LOGGER.debug('    Found: %s', mount['source'])
> > +                        break
> > +                else:
> > +                    LOGGER.debug('    Not Found')
> > +                    mount['source'] = None
> > +
> >          cmd = './{} {}'.format(self.install_script, self.image_name)
> >          LOGGER.debug('Running \'%s\' to install the docker image. '
> >                       'This may take a while....', cmd)
> >          status = subprocess.call(cmd, shell=True)
> >          if status != 0:
> >              return None
> > +
> >          docker_client = docker.from_env()
> >          containers = docker_client.containers.list(
> >              all=True, filters={'name': self.container_name})
> > @@ -188,10 +219,42 @@ class FuegoContainer:
> >                  self.container_name)
> >              containers[0].remove(force=True)
> > 
> > +        mounts = [
> > +            {'source': os.path.abspath('./fuego-rw'),
> > +             'destination':  '/fuego-rw',
> > +             'readonly': False,
> > +             },
> > +            {'source': os.path.abspath('./fuego-ro'),
> > +             'destination':  '/fuego-ro',
> > +             'readonly': True,
> > +             },
> > +            {'source': os.path.abspath('../fuego-core'),
> > +             'destination':  '/fuego-core',
> > +             'readonly': True,
> > +             },
> > +        ]
> > +
> > +        our_id = this_container_id()
> > +
> > +        if our_id:
> > +            LOGGER.debug('Running inside the Docker container %s', our_id)
> > +            map_to_host(mounts, our_id)
> > +        else:
> > +            LOGGER.debug('Not running inside a Docker container')
> > +
> > +        LOGGER.debug('Creating container with the following mountpoints:')
> > +        LOGGER.debug('  %s', mounts)
> > +
> >          container = docker_client.containers.create(
> >              self.image_name,
> >              stdin_open=True, tty=True, network_mode='bridge',
> > +            mounts=[docker.types.Mount(m['destination'],
> > +                                       m['source'],
> > +                                       type='bind',
> > +                                       read_only=m['readonly'])
> > +                    for m in mounts],
> >              name=self.container_name, command='/bin/bash')
> > +
> >          LOGGER.debug('Container \'%s\' created', self.container_name)
> >          return container
> > 
> > --
> > 2.16.2
> 

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 06/16] Allow the user to keep the container running after the test
  2018-03-30 22:48   ` Tim.Bird
@ 2018-04-03  3:20     ` Guilherme Camargo
  2018-04-03 20:08       ` Tim.Bird
  0 siblings, 1 reply; 40+ messages in thread
From: Guilherme Camargo @ 2018-04-03  3:20 UTC (permalink / raw)
  To: Tim.Bird; +Cc: fuego

On Fri, Mar 30, 2018 at 10:48:06PM +0000, Tim.Bird@sony.com wrote:
> 
> 
> > -----Original Message-----
> > From: Guilherme Campos Camargo
> > 
> > Through the `--remove-test-container` command line argument.
> 
> Don't split a sentence in the commit description line and the
> body of the commit description.  Also, the argument appears
> to be '--no-rm-container'.  Description here was not helpful.
> 
> Also, there are extra spaces between 'running' and 'after' in the commit description.
> Note sure what happened there.
> 

Thanks, Tim. Sorry for that.

Should I send a fixup commit fixing that commit message? This would
require a force push on master, so I'm not sure if you would want that.

> > 
> > Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
> > ---
> >  engine/tests/Functional.fuegotest/test_run.py | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/engine/tests/Functional.fuegotest/test_run.py
> > b/engine/tests/Functional.fuegotest/test_run.py
> > index d6bec0c..fa68858 100755
> > --- a/engine/tests/Functional.fuegotest/test_run.py
> > +++ b/engine/tests/Functional.fuegotest/test_run.py
> > @@ -154,16 +154,20 @@ class ShExpect():
> > 
> >  class FuegoContainer:
> >      def __init__(self, install_script, image_name, container_name,
> > -                 jenkins_port):
> > +                 jenkins_port, rm_after_test=True):
> >          self.install_script = install_script
> >          self.image_name = image_name
> >          self.container_name = container_name
> >          self.jenkins_port = jenkins_port
> > +        self.rm_after_test = rm_after_test
> > 
> >      def __del__(self):
> >          if self.container:
> > -            LOGGER.debug('Removing Container')
> > -            self.container.remove(force=True)
> > +            if self.rm_after_test:
> > +                LOGGER.debug('Removing Container')
> > +                self.container.remove(force=True)
> > +            else:
> > +                LOGGER.debug('Not Removing the test container')
> > 
> >      def install(self):
> >          self.docker_client = docker.APIClient()
> > @@ -453,13 +457,18 @@ def main():
> >                          'test container. Defaults to {}'
> >                          .format(DEFAULT_JENKINS_PORT),
> >                          default=DEFAULT_JENKINS_PORT, type=int)
> > +    parser.add_argument('--no-rm-container',
> > +                        help='Do not remove the container after tests are '
> > +                        'complete.',
> > +                        dest='rm_test_container', default=True, action='store_false')
> >      args = parser.parse_args()
> > 
> >      LOGGER.debug('Changing working dir to \'%s\'', args.working_dir)
> >      os.chdir(args.working_dir)
> > 
> >      container = FuegoContainer(args.install_script, args.image_name,
> > -                               args.container_name, args.jenkins_port)
> > +                               args.container_name, args.jenkins_port,
> > +                               rm_after_test=args.rm_test_container)
> >      if not container.install():
> >          return 1
> > 
> > --
> > 2.16.2
> > 
> > _______________________________________________
> > Fuego mailing list
> > Fuego@lists.linuxfoundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/fuego

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 07/16] Add Back() Selenium Command
  2018-03-30 22:52   ` Tim.Bird
@ 2018-04-03  3:21     ` Guilherme Camargo
  2018-04-03 20:09       ` Tim.Bird
  0 siblings, 1 reply; 40+ messages in thread
From: Guilherme Camargo @ 2018-04-03  3:21 UTC (permalink / raw)
  To: Tim.Bird; +Cc: fuego

On Fri, Mar 30, 2018 at 10:52:13PM +0000, Tim.Bird@sony.com wrote:
> 
> 
> > -----Original Message-----
> > From: Guilherme Campos
> > Camargo
> > Sent: Wednesday, March 28, 2018 5:08 PM
> > To: fuego@lists.linuxfoundation.org
> > Subject: [Fuego] [PATCH 07/16] Add Back() Selenium Command
> > 
> > That will go one level back in the browser history.
> 
> Please don't split a sentence between the commit summary and commit description.
> 
> Patch content is OK.
>  -- Tim
> 

Thanks, Tim. I can fix that commit message in a fixup commit, as said on
the previous patch. Would you like me to do it?


> > 
> > Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
> > ---
> >  engine/tests/Functional.fuegotest/test_run.py | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/engine/tests/Functional.fuegotest/test_run.py
> > b/engine/tests/Functional.fuegotest/test_run.py
> > index fa68858..851074f 100755
> > --- a/engine/tests/Functional.fuegotest/test_run.py
> > +++ b/engine/tests/Functional.fuegotest/test_run.py
> > @@ -97,6 +97,16 @@ class CheckText(SeleniumCommand):
> >          return result == self.expected_result
> > 
> > 
> > +class Back(SeleniumCommand):
> > +    def exec(self, selenium_ctx):
> > +        super().exec(selenium_ctx)
> > +
> > +        LOGGER.debug('  Going back')
> > +        self.driver.back()
> > +
> > +        return True
> > +
> > +
> >  class ShExpect():
> >      BASH_PATTERN = 'test_run_pr1:#'
> >      COMMAND_RESULT_PATTERN = re.compile('^([0-9]+)', re.M)
> > --
> > 2.16.2
> > 
> > _______________________________________________
> > Fuego mailing list
> > Fuego@lists.linuxfoundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/fuego

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 08/16] Add ClickLink selenium command
  2018-03-30 22:53   ` Tim.Bird
@ 2018-04-03  3:21     ` Guilherme Camargo
  0 siblings, 0 replies; 40+ messages in thread
From: Guilherme Camargo @ 2018-04-03  3:21 UTC (permalink / raw)
  To: Tim.Bird; +Cc: fuego

On Fri, Mar 30, 2018 at 10:53:38PM +0000, Tim.Bird@sony.com wrote:
> 
> 
> > -----Original Message-----
> > From: Guilherme Campos Camargo
> > 
> > Clicks in the link that has the text passed in the argument `linktext`.
> > Returns False if no link containing `linktext` is found.
> > 
> > Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
> > ---
> >  engine/tests/Functional.fuegotest/test_run.py | 24
> > ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/engine/tests/Functional.fuegotest/test_run.py
> > b/engine/tests/Functional.fuegotest/test_run.py
> > index 851074f..e03853b 100755
> > --- a/engine/tests/Functional.fuegotest/test_run.py
> > +++ b/engine/tests/Functional.fuegotest/test_run.py
> > @@ -13,6 +13,7 @@ import docker
> >  import pexpect
> >  import requests
> >  from selenium import webdriver
> > +import selenium.common.exceptions as selenium_exceptions
> > 
> >  LOGGER = logging.getLogger('test_run')
> >  STREAM_HANDLER = logging.StreamHandler()
> > @@ -97,6 +98,29 @@ class CheckText(SeleniumCommand):
> >          return result == self.expected_result
> > 
> > 
> > +class ClickLink(SeleniumCommand):
> > +    def __init__(self, linktext, expected_result=True):
> > +        self.linktext = linktext
> > +        self.expected_result = expected_result
> > +
> > +    def exec(self, selenium_ctx):
> > +        super().exec(selenium_ctx)
> > +
> > +        LOGGER.debug(
> > +            '  Searching for a link with the text \'%s\'', self.linktext)
> Use " to avoid having to escape ' in the string.

Fixed. Thanks.

> > +
> > +        try:
> > +            link = self.driver.find_element_by_partial_link_text(self.linktext)
> > +            link.click()
> > +            LOGGER.debug('  Link found and clicked')
> > +            result = True
> > +        except selenium_exceptions.NoSuchElementException:
> > +            LOGGER.error('  Link not found')
> > +            result = False
> > +
> > +        return result == self.expected_result
> > +
> > +
> >  class Back(SeleniumCommand):
> >      def exec(self, selenium_ctx):
> >          super().exec(selenium_ctx)
> > --
> > 2.16.2
> > 
> > _______________________________________________
> > Fuego mailing list
> > Fuego@lists.linuxfoundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/fuego

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 10/16] Write ok/fail on test report
  2018-03-30 22:58   ` Tim.Bird
@ 2018-04-03  3:21     ` Guilherme Camargo
  0 siblings, 0 replies; 40+ messages in thread
From: Guilherme Camargo @ 2018-04-03  3:21 UTC (permalink / raw)
  To: Tim.Bird; +Cc: fuego

On Fri, Mar 30, 2018 at 10:58:24PM +0000, Tim.Bird@sony.com wrote:
> 
> 
> > -----Original Message-----
> > From: Guilherme Campos Camargo
> > 
> > Remove the 'minimal test' that had been copied from an example test.
> > 
> > Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
> > ---
> >  engine/tests/Functional.fuegotest/fuego_test.sh | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/engine/tests/Functional.fuegotest/fuego_test.sh
> > b/engine/tests/Functional.fuegotest/fuego_test.sh
> > index b4755bc..ee60c81 100755
> > --- a/engine/tests/Functional.fuegotest/fuego_test.sh
> > +++ b/engine/tests/Functional.fuegotest/fuego_test.sh
> > @@ -1,7 +1,5 @@
> >  #!/bin/bash
> > 
> > -set -e
> > -
> >  readonly fuego_release_dir=fuego-release
> > 
> >  function test_build {
> > @@ -26,9 +24,13 @@ function test_build {
> > 
> >  function test_run {
> >      sudo -n ${TEST_HOME}/test_run.py "${fuego_release_dir}/fuego"
> > -    report "echo ok 1 minimal test on target"
> > +    if [ "${?}" = 0 ]; then
> > +        report "echo fuego-test: ok"
> > +    else
> > +        report "echo fuego-test: fail"
> > +    fi
> 
> Please keep the output in TAP format, which should look like this:
>     report "echo ok 1 fuego release test"
> else
>     report "echo not ok 1 fuego release test"
> 
> >  }

Sure. Thanks. I will use the TAP format.

> > 
> >  function test_processing {
> > -    log_compare "$TESTDIR" "1" "^ok" "p"
> > +    log_compare "$TESTDIR" "1" "ok$" "p"
> We're trying to stay with TAP format where possible.
> This line can be left alone.

Thanks for reminding me to change this line too.

> 
>  -- Tim
> 

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 15/16] Improve Click and Check methods to allow multiple locators
  2018-03-30 23:03   ` Tim.Bird
@ 2018-04-03  3:22     ` Guilherme Camargo
  0 siblings, 0 replies; 40+ messages in thread
From: Guilherme Camargo @ 2018-04-03  3:22 UTC (permalink / raw)
  To: Tim.Bird; +Cc: fuego

On Fri, Mar 30, 2018 at 11:03:28PM +0000, Tim.Bird@sony.com wrote:
> 
> 
> > -----Original Message-----
> > From: Guilherme Campos Camargo
> > Subject: [Fuego] [PATCH 15/16] Improve Click and Check methods to allow
> > multiple locators
> 
> Need more information here.   In general, there should usually be something
> in the description of the commit, even if it just rewords the summary line.
> But in this case, more text explaining the methods would have been good.
> 
> (but the commit is already applied...)

Sorry about that. In future commits I'll make sure to have non-empty
descriptions.

Same for this one. Please let me know if you want me to send a fixup.

> > 
> > Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
> > ---
> >  engine/tests/Functional.fuegotest/test_run.py | 105 ++++++++++++++++--
> > --------
> >  1 file changed, 65 insertions(+), 40 deletions(-)
> > 
> > diff --git a/engine/tests/Functional.fuegotest/test_run.py
> > b/engine/tests/Functional.fuegotest/test_run.py
> > index 0370b6f..d433548 100755
> > --- a/engine/tests/Functional.fuegotest/test_run.py
> > +++ b/engine/tests/Functional.fuegotest/test_run.py
> > @@ -13,6 +13,7 @@ import docker
> >  import pexpect
> >  import requests
> >  from selenium import webdriver
> > +from selenium.webdriver.common.by import By
> >  import selenium.common.exceptions as selenium_exceptions
> > 
> >  LOGGER = logging.getLogger('test_run')
> > @@ -44,6 +45,46 @@ class SeleniumCommand:
> >          LOGGER.debug('Executing Selenium Command \'%s\'',
> >                       self.__class__.__name__)
> > 
> > +    @staticmethod
> > +    def check_element_text(element, text):
> > +        try:
> > +            if text in element.text:
> > +                LOGGER.\
> > +                    debug('  Text \'%s\' matches element.text \'%s\'',
> please use " to avoid escaping '.
> 
> > +                          text, element.text)
> > +                return True
> > +            else:
> > +                LOGGER.\
> > +                    debug('  Text \'%s\' does not match element.text \'%s\'',
> please use " to avoid escaping '.
> 
> > +                          text, element.text)
> > +                return False
> > +        except (selenium_exceptions.ElementNotVisibleException,
> > +                selenium_exceptions.NoSuchAttributeException,):
> > +            LOGGER.error('  Element has no visible Text')
> > +            return False
> > +
> > +    @staticmethod
> > +    def click_element(element):
> > +        try:
> > +            element.click()
> > +            LOGGER.debug('  Element clicked')
> > +            return True
> > +        except (selenium_exceptions.ElementClickInterceptedException,
> > +                selenium_exceptions.ElementNotVisibleException,):
> > +            LOGGER.error('  Element is not clickable')
> > +            return False
> > +
> > +    @staticmethod
> > +    def find_element(context, locator, pattern):
> > +        try:
> > +            element = context.driver.find_element(locator, pattern)
> > +        except selenium_exceptions.NoSuchElementException:
> > +            LOGGER.error('  Element not found')
> > +            return None
> > +
> > +        LOGGER.debug('  Element found')
> > +        return element
> > +
> > 
> >  class Visit(SeleniumCommand):
> >      def __init__(self, url, timeout=10, expected_result=200):
> > @@ -69,55 +110,37 @@ class Visit(SeleniumCommand):
> > 
> > 
> >  class CheckText(SeleniumCommand):
> > -    def __init__(self, _id, text, expected_result=True):
> > -        self._id = _id
> > +    def __init__(self, locator, pattern, text='', expected_result=True):
> > +        self.pattern = pattern
> > +        self.locator = locator
> >          self.text = text
> >          self.expected_result = expected_result
> > 
> >      def exec(self, selenium_ctx):
> >          super().exec(selenium_ctx)
> > 
> > -        try:
> > -            text = self.driver.find_element_by_id(self._id).text
> > -        except Exception:  # TODO: Use proper Exception
> > -            return False
> > -
> > -        LOGGER.debug(
> > -            '  Searching for \'%s\' in \'id:%s\'', self.text, self._id)
> > -
> > -        result = True
> > -        if self.text not in text:
> > -            LOGGER.error(
> > -                '  \'%s\' not found in id \'%s\' with text \'%s\'', self.text,
> > -                self._id, text)
> > -            result = False
> > -
> > -        LOGGER.debug('  \'%s\' was found', self.text)
> > +        element = SeleniumCommand.\
> > +            find_element(selenium_ctx, self.locator, self.pattern)
> > +        if element:
> > +            result = SeleniumCommand.check_element_text(element, self.text)
> > 
> >          return result == self.expected_result
> > 
> > 
> > -class ClickLink(SeleniumCommand):
> > -    def __init__(self, linktext, expected_result=True):
> > -        self.linktext = linktext
> > -        self.expected_result = expected_result
> > +class Click(SeleniumCommand):
> > +    def __init__(self, locator, pattern):
> > +        self.pattern = pattern
> > +        self.locator = locator
> > 
> >      def exec(self, selenium_ctx):
> >          super().exec(selenium_ctx)
> > 
> > -        LOGGER.debug(
> > -            '  Searching for a link with the text \'%s\'', self.linktext)
> > +        element = SeleniumCommand.\
> > +            find_element(selenium_ctx, self.locator, self.pattern)
> > +        if element:
> > +            return SeleniumCommand.click_element(element)
> > 
> > -        try:
> > -            link = self.driver.find_element_by_partial_link_text(self.linktext)
> > -            link.click()
> > -            LOGGER.debug('  Link found and clicked')
> > -            result = True
> > -        except selenium_exceptions.NoSuchElementException:
> > -            LOGGER.error('  Link not found')
> > -            result = False
> > -
> > -        return result == self.expected_result
> > +        return False
> > 
> > 
> >  class Back(SeleniumCommand):
> > @@ -529,25 +552,27 @@ def main():
> >          # Add Nodes
> >          ShExpect('ftc add-nodes docker'),
> >          ShExpect('ftc list-nodes -q', r'.*docker.*'),
> > -        CheckText(_id='executors', text='master'),
> > -        CheckText(_id='executors', text='docker'),
> > +        CheckText(By.ID, 'executors', text='master'),
> > +        CheckText(By.ID, 'executors', text='docker'),
> > 
> >          # Add Fuego TestPlan
> >          ShExpect('ftc add-jobs -b docker -p testplan_fuego_tests'),
> >          ShExpect('ftc list-jobs', r'.*docker\.testplan_fuego_tests\.batch.*'),
> > 
> > -        ClickLink(linktext='docker'),
> > -        CheckText(_id='projectstatus',
> > +        Click(By.PARTIAL_LINK_TEXT, 'docker'),
> > +        CheckText(By.ID, 'projectstatus',
> >                    text='docker.testplan_fuego_tests.batch'),
> >          Back(),
> > 
> >          # Install Views
> >          ShExpect('ftc add-view batch .*.batch'),
> > -        CheckText(_id='projectstatus-tabBar', text='batch'),
> > +        CheckText(By.ID, 'projectstatus-tabBar',
> > +                  text='batch'),
> > 
> >          # Start Tests
> >          ShExpect('ftc build-jobs *.*.Functional.fuego_board_check'),
> > -        CheckText(_id='buildQueue', text='Functional.fuego_board_check'),
> > +        CheckText(By.ID, 'buildQueue',
> > +                  text='Functional.fuego_board_check'),
> >      ]
> > 
> >      if not execute_tests(args.timeout):
> > --
> > 2.16.2
> > 
> > _______________________________________________
> > Fuego mailing list
> > Fuego@lists.linuxfoundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/fuego

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 01/16] Add fuego-release Functional test
  2018-04-03  3:18       ` Guilherme Camargo
@ 2018-04-03 20:03         ` Tim.Bird
  0 siblings, 0 replies; 40+ messages in thread
From: Tim.Bird @ 2018-04-03 20:03 UTC (permalink / raw)
  To: guicc; +Cc: fuego

> -----Original Message-----
> From: Guilherme Camargo on Monday, April 02, 2018 8:18 PM
> On Fri, Mar 30, 2018 at 10:37:21PM +0000, Tim.Bird@sony.com wrote:
> >
> >
> > > -----Original Message-----
> > > From: Tim Bird
> > > My strategy is to apply all your patches, but ask for modifications,
> > > that you can provide as patches on top of this set.
> > >
> > > If I make these changes, they will break other patches in the
> > > series, which will be obnoxious and time-consuming to fix.
> > >
> > > See below inline for comments.
> > >  -- Tim
> > >
> > > > -----Original Message-----
> > > > From: Guilherme Campos Camargo
> > > > Allows Fuego to test new Fuego releases.
> > > >
> > > > The fuego and fuego-core repositores and branches to be tested may
> be
> > > > configured on the specs.json file. Their defaults are:
> > > >
> > > > "FUEGOREPO":"https://bitbucket.org/tbird20d/fuego",
> > > > "FUEGOBRANCH":"master",
> > > > "FUEGOCOREREPO":"https://bitbucket.org/tbird20d/fuego-core",
> > > > "FUEGOCOREBRANCH":"master"
> > > >
> > > Can you please add underscores to these names:
> > > FUEGO_REPO, FUEGO_CORE_BRANCH, etc.
> > > ?
> > >
> > > It's very nice to be able to customize these in the spec file.
> > >
> > > > Signed-off-by: Guilherme Campos Camargo <guicc@profusion.mobi>
> > > > ---
> > > >  engine/tests/Functional.fuegotest/fuego_test.sh |  29 ++
> > > >  engine/tests/Functional.fuegotest/spec.json     |  11 +
> > > >  engine/tests/Functional.fuegotest/test_run.py   | 427
> > > > ++++++++++++++++++++++++
> > > >  3 files changed, 467 insertions(+)
> > > >  create mode 100755 engine/tests/Functional.fuegotest/fuego_test.sh
> > > >  create mode 100644 engine/tests/Functional.fuegotest/spec.json
> > > >  create mode 100755 engine/tests/Functional.fuegotest/test_run.py
> > > >
> > > > diff --git a/engine/tests/Functional.fuegotest/fuego_test.sh
> > > > b/engine/tests/Functional.fuegotest/fuego_test.sh
> > > > new file mode 100755
> > > > index 0000000..192c15b
> > > > --- /dev/null
> > > > +++ b/engine/tests/Functional.fuegotest/fuego_test.sh
> > > > @@ -0,0 +1,29 @@
> > > > +#!/bin/bash
> > > > +
> > > > +set -e
> > > > +
> > > > +readonly fuego_release_dir=fuego-release
> > > > +
> > > > +function test_build {
> > > > +    if [ -d ${fuego_release_dir} ]; then
> > > > +        rm -r ${fuego_release_dir}
> > > > +    fi
> > > > +    git clone --quiet --depth 1 --single-branch \
> > > > +        --branch "${FUNCTIONAL_FUEGOTEST_FUEGOBRANCH}" \
> > > > +        "${FUNCTIONAL_FUEGOTEST_FUEGOREPO}" \
> >
> > I realize now that my suggestion to change the test name and
> > add underscores is going to make some really long variable names.
> >
> > e.g. FUNCTIONAL_FUEGO_RELEASE_TEST_FUEGO_CORE_REPO.
> >
> > Yikes.  Oh well, let me know what you think.  I still think the
> > name has more clarity, but maybe it could be shortened inside
> > the test?
> >
> 
> I agree that the names will become more readable specially on the specs
> json by using the underlines and I like the change on the test name. I
> don't think that having these huge variables is a big problem, because,
> as you said, we can just create an alias for them in the fuego_test.sh
> 
> That's what I will do, then.
> 
> Also, do we really need the to have the test name as a prefix for specs
> variables? If not, we might consider changing that in the future, by
> exporting the variables with the same names that they have on the
> spec.json.

In a shell environment, where these end up being global in scope, it's
useful to prefix the variable with something to avoid name collisions.
I'd be happy to get rid of the "FUNCTIONAL_" part though, and possibly
we could use the same prefix, instead of a test-specific prefix:
something like: SPEC_ARGS, instead of FUNCTIONAL_HELLO_WORLD_ARGS.
I can't think of any cases where we use variables from multiple
tests in the same run.

(BTW - don't go do any of that - I've been meaning to rename 'spec' to
something else... it's on my to-do list.)
 -- Tim



^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 06/16] Allow the user to keep the container running after the test
  2018-04-03  3:20     ` Guilherme Camargo
@ 2018-04-03 20:08       ` Tim.Bird
  0 siblings, 0 replies; 40+ messages in thread
From: Tim.Bird @ 2018-04-03 20:08 UTC (permalink / raw)
  To: guicc; +Cc: fuego



> -----Original Message-----
> From: Guilherme Camargo 

> On Fri, Mar 30, 2018 at 10:48:06PM +0000, Tim.Bird@sony.com wrote:
> >
> >
> > > -----Original Message-----
> > > From: Guilherme Campos Camargo
> > >
> > > Through the `--remove-test-container` command line argument.
> >
> > Don't split a sentence in the commit description line and the
> > body of the commit description.  Also, the argument appears
> > to be '--no-rm-container'.  Description here was not helpful.
> >
> > Also, there are extra spaces between 'running' and 'after' in the commit
> description.
> > Note sure what happened there.
> >
> 
> Thanks, Tim. Sorry for that.
> 
> Should I send a fixup commit fixing that commit message? This would
> require a force push on master, so I'm not sure if you would want that.

No.  As you said, it would require a force push, which I try to avoid.
If something bothers me enough, I'll just change it when I'm doing
my integration. I think I did a commit reword on one of the patches.

The whitespace thing is a minor issue, but I thought
I'd mention it for future commits.  It also might not even be a real issue.
I seem to recall one white-space issue that appeared in the e-mail, but
in the  actual commit I cherry-picked from your tree was not there.

In general, while nice commit messages are appreciated, I don't think
people will do a lot of mining of commit messages for Fuego, so they're
not critical.

 -- Tim


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 07/16] Add Back() Selenium Command
  2018-04-03  3:21     ` Guilherme Camargo
@ 2018-04-03 20:09       ` Tim.Bird
  0 siblings, 0 replies; 40+ messages in thread
From: Tim.Bird @ 2018-04-03 20:09 UTC (permalink / raw)
  To: guicc; +Cc: fuego



> -----Original Message-----
> From: Guilherme Camargo
> 
> On Fri, Mar 30, 2018 at 10:52:13PM +0000, Tim.Bird@sony.com wrote:
> >
> >
> > > -----Original Message-----
> > > From: Guilherme Campos
> > > Camargo
> > > Sent: Wednesday, March 28, 2018 5:08 PM
> > > To: fuego@lists.linuxfoundation.org
> > > Subject: [Fuego] [PATCH 07/16] Add Back() Selenium Command
> > >
> > > That will go one level back in the browser history.
> >
> > Please don't split a sentence between the commit summary and commit
> description.
> >
> > Patch content is OK.
> >  -- Tim
> >
> 
> Thanks, Tim. I can fix that commit message in a fixup commit, as said on
> the previous patch. Would you like me to do it?

No.  Thanks though.
 -- Tim


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 01/16] Add fuego-release Functional test
  2018-04-03  3:17     ` Guilherme Camargo
@ 2018-04-03 20:22       ` Tim.Bird
  2018-04-03 22:09         ` Guilherme Camargo
  0 siblings, 1 reply; 40+ messages in thread
From: Tim.Bird @ 2018-04-03 20:22 UTC (permalink / raw)
  To: guicc; +Cc: fuego



> -----Original Message-----
> From: Guilherme Camargo
> 
> Hello, Tim. Thanks for the the review of this patch.
> 
> I'm implementing most of your comments. Please see a few comments
> inline.
> 
> Also, regarding the string formatting operations - the newer method
> str.format() VS the legacy version % - I think we should consider
> keeping the newer method for the following reasons:
> 
>    1. Python documentation encourages the use of the new method
>    (str.format()) instead of the old method - as we can see here
> https://docs.python.org/release/3.1.5/library/stdtypes.html#old-string-
> formatting-operations
> 
>    2. There are a few advantages of using this new method for complex
>    string formatting, specially when a value is used multiple times in
>    the string as in:
> 
>    ```
>    161 COMMAND_OUTPUT_PATTERN = re.compile(
>    162     r'^{0}(.*){0}\s+{1}'.format(
>    163         COMMAND_OUTPUT_DELIM, BASH_PATTERN), re.M | re.S)
>    ```
> 
>    or
> 
>    ```
>    191             "echo '{0}${{{1}}}{0}'".format(
>    192                 self.COMMAND_OUTPUT_DELIM,
>    193                 self.OUTPUT_VARIABLE))
>    ```
> 
>    Also, I checked and we're using the newer method consistently
>    throughout the test.
> 
> But it's your call, no problem if you still prefer using '%', please
> just let me know and I can make the change if that's the case.

My personal preference is to use the % format operator, and the
%s-style formatting strings.  I also use C, and it is a little easier on
my brain to not have to remember 2 different formatting syntaxes.

Please change them.

Thanks,
 -- Tim



^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Fuego] [PATCH 01/16] Add fuego-release Functional test
  2018-04-03 20:22       ` Tim.Bird
@ 2018-04-03 22:09         ` Guilherme Camargo
  0 siblings, 0 replies; 40+ messages in thread
From: Guilherme Camargo @ 2018-04-03 22:09 UTC (permalink / raw)
  To: Bird, Timothy; +Cc: fuego

[-- Attachment #1: Type: text/plain, Size: 1914 bytes --]

On Tue, Apr 3, 2018 at 5:22 PM, <Tim.Bird@sony.com> wrote:

>
>
> > -----Original Message-----
> > From: Guilherme Camargo
> >
> > Hello, Tim. Thanks for the the review of this patch.
> >
> > I'm implementing most of your comments. Please see a few comments
> > inline.
> >
> > Also, regarding the string formatting operations - the newer method
> > str.format() VS the legacy version % - I think we should consider
> > keeping the newer method for the following reasons:
> >
> >    1. Python documentation encourages the use of the new method
> >    (str.format()) instead of the old method - as we can see here
> > https://docs.python.org/release/3.1.5/library/stdtypes.html#old-string-
> > formatting-operations
> >
> >    2. There are a few advantages of using this new method for complex
> >    string formatting, specially when a value is used multiple times in
> >    the string as in:
> >
> >    ```
> >    161 COMMAND_OUTPUT_PATTERN = re.compile(
> >    162     r'^{0}(.*){0}\s+{1}'.format(
> >    163         COMMAND_OUTPUT_DELIM, BASH_PATTERN), re.M | re.S)
> >    ```
> >
> >    or
> >
> >    ```
> >    191             "echo '{0}${{{1}}}{0}'".format(
> >    192                 self.COMMAND_OUTPUT_DELIM,
> >    193                 self.OUTPUT_VARIABLE))
> >    ```
> >
> >    Also, I checked and we're using the newer method consistently
> >    throughout the test.
> >
> > But it's your call, no problem if you still prefer using '%', please
> > just let me know and I can make the change if that's the case.
>
> My personal preference is to use the % format operator, and the
> %s-style formatting strings.  I also use C, and it is a little easier on
> my brain to not have to remember 2 different formatting syntaxes.
>
> Please change them.
>
> Thanks,
>  -- Tim
>
>
> Ok. Just sent a patch fixing that. Thanks.​


​--
Guilherme​

[-- Attachment #2: Type: text/html, Size: 3074 bytes --]

^ permalink raw reply	[flat|nested] 40+ messages in thread

end of thread, other threads:[~2018-04-03 22:09 UTC | newest]

Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-29  0:08 [Fuego] [PATCH 00/16] Fuego Release Test Guilherme Campos Camargo
2018-03-29  0:08 ` [Fuego] [PATCH 01/16] Add fuego-release Functional test Guilherme Campos Camargo
2018-03-30 22:17   ` Tim.Bird
2018-03-30 22:37     ` Tim.Bird
2018-04-03  3:18       ` Guilherme Camargo
2018-04-03 20:03         ` Tim.Bird
2018-04-03  3:17     ` Guilherme Camargo
2018-04-03 20:22       ` Tim.Bird
2018-04-03 22:09         ` Guilherme Camargo
2018-03-29  0:08 ` [Fuego] [PATCH 02/16] Mount fuego-rw/ro/core into the fuego-under-test container Guilherme Campos Camargo
2018-03-30 22:31   ` Tim.Bird
2018-04-03  3:18     ` Guilherme Camargo
2018-03-29  0:08 ` [Fuego] [PATCH 03/16] Increase wait_for_jenkins timeout from 10 to 60s Guilherme Campos Camargo
2018-03-29  0:08 ` [Fuego] [PATCH 04/16] Print fuego repo/branch information during test build Guilherme Campos Camargo
2018-03-29  0:08 ` [Fuego] [PATCH 05/16] Properly check install return code and abort in case of failure Guilherme Campos Camargo
2018-03-29  0:08 ` [Fuego] [PATCH 06/16] Allow the user to keep the container running after the test Guilherme Campos Camargo
2018-03-30 22:48   ` Tim.Bird
2018-04-03  3:20     ` Guilherme Camargo
2018-04-03 20:08       ` Tim.Bird
2018-03-29  0:08 ` [Fuego] [PATCH 07/16] Add Back() Selenium Command Guilherme Campos Camargo
2018-03-30 22:52   ` Tim.Bird
2018-04-03  3:21     ` Guilherme Camargo
2018-04-03 20:09       ` Tim.Bird
2018-03-29  0:08 ` [Fuego] [PATCH 08/16] Add ClickLink selenium command Guilherme Campos Camargo
2018-03-30 22:53   ` Tim.Bird
2018-04-03  3:21     ` Guilherme Camargo
2018-03-29  0:08 ` [Fuego] [PATCH 09/16] Include add-jobs, add-views and build-jobs tests Guilherme Campos Camargo
2018-03-29  0:08 ` [Fuego] [PATCH 10/16] Write ok/fail on test report Guilherme Campos Camargo
2018-03-30 22:58   ` Tim.Bird
2018-04-03  3:21     ` Guilherme Camargo
2018-03-29  0:08 ` [Fuego] [PATCH 11/16] Properly quit Selenium driver when SeleniumSession is deleted Guilherme Campos Camargo
2018-03-29  0:08 ` [Fuego] [PATCH 12/16] Minor style/PEP8 fixes Guilherme Campos Camargo
2018-03-29  0:08 ` [Fuego] [PATCH 13/16] Move Selenium implicitly_wait() to SeleniumSession start Guilherme Campos Camargo
2018-03-29  0:08 ` [Fuego] [PATCH 14/16] Use a fixed language for Selenium Chrome-WebDriver Guilherme Campos Camargo
2018-03-29  0:08 ` [Fuego] [PATCH 15/16] Improve Click and Check methods to allow multiple locators Guilherme Campos Camargo
2018-03-30 23:03   ` Tim.Bird
2018-04-03  3:22     ` Guilherme Camargo
2018-03-29  0:08 ` [Fuego] [PATCH 16/16] Add test that starts a job through the UI Guilherme Campos Camargo
2018-03-30 21:34 ` [Fuego] [PATCH 00/16] Fuego Release Test Tim.Bird
2018-04-03  3:16   ` Guilherme Camargo

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.