From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=profusion-mobi.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=n61Wt6f/SUQX5m5LOOjZASECV2MfgVewwKxW/xLT6yE=; b=FYY2RQShZUDmtExWGjYDbCLZMcs2cyxki7895sRzULXI0niB1n24GBJGj0K7AfHRjC 8PeP1ucK6X6kZjoinquSP++boId8lhtQ+GojsbQMROgjJaTUCMFnPi1UjCWH1SGSqtmO nX78RseDcjknebAEZiPJYUOoLwx6E7D6orF5mfFkr5pijE5vGwWlQCK7hepQsE/fZHWN ot06r+3VGoEs4mV9iaRzQTzeTC7ipm4UnGNR9SDJtHLJxA9QDpycsZhhlNkQhBZsBKvG +wZ372MqPsrWL4BN6NqdbQ43gOCLgVisOYJ6EjoM4pB/1t6Bb17DEJRolqcK8v8Twt9s 9Jgg== Date: Tue, 3 Apr 2018 00:17:43 -0300 From: Guilherme Camargo Message-ID: <20180403031743.ua6qx6xdnmqhkpkm@guicc-arch-three> References: <20180329000832.12268-1-guicc@profusion.mobi> <20180329000832.12268-2-guicc@profusion.mobi> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Fuego] [PATCH 01/16] Add fuego-release Functional test List-Id: Mailing list for the Fuego test framework List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Tim.Bird@sony.com Cc: fuego@lists.linuxfoundation.org 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 > > --- > > 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