Acceptance framework PoC

[scrye]

Also includes an acceptance test for dispatcher reconnections in Go.

---

This change is 

[scrye]

a discussion (no related file):
The two go/lib changes are included to make the acceptance test pass. They will not be part of this PR. The changes are included in #1906.

---

[kormat]

Reviewed 3 of 8 files at r1, 5 of 5 files at r2.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @scrye)

---

acceptance/color.sh, line 3 at r2 (raw file):

#!/bin/bash

NC='\033[0m'

Raw control codes aren't a great idea, as you can't count on all terminals/platforms interpreting them correctly. It's also good to be sure that there's a terminal attached. So:

if [ -t 1 ]; then
    GREEN=$(tput setaf 2)
    RED=$(tput setaf 9)
    YELLOW=$(tput setaf 11)
    NC=$(tput sgr0)
fi

(colors taken from https://jonasjacek.github.io/colors/)

If stdout is not a terminal, then the colour vars will be undefined, and the print_X functions will gracefully degrade.

---

acceptance/color.sh, line 8 at r2 (raw file):

print_green() {
    GREEN='\033[0;32m'
    printf "${GREEN}$1${NC} $2 \n"

To make these functions a bit easier to use, i'd do:

local prefix="$1"
shift
printf "${GREEN}$prefix{$NC} $@\n"

That way you can call print_green with any number of arguments.

---

acceptance/lib.sh, line 1 at r2 (raw file):

#!/bin/bash

A shell script that is sourced and not executed shouldn't have a she-bang.

---

acceptance/lib.sh, line 5 at r2 (raw file):

. acceptance/color.sh

run_command() {

The way that this is run (with the output being hidden) means that i get a sudo prompt, but zero information as to what i'm agreeing to. 😨

#1941 is out now merged to fix this.

---

acceptance/lib.sh, line 6 at r2 (raw file):

run_command() {
    local COMMAND="$1"

I'd use lowercase (and shorter) local var names. e.g cmd and out.

---

acceptance/lib.sh, line 12 at r2 (raw file):

    else
        "$COMMAND"
    fi

I think this can be simplified to:

local cmd="$1"
local out="$2"
$cmd &>> "${out:-/dev/stdout}"
local ret=$?
if [[ -n "$out" && $ret -ne 0 ]]; then
  cat "$OUTPUT_FILE"
fi
return $ret

(note that $cmd is deliberately unquoted, which allows a multi-arg command to be run).

---

acceptance/lib.sh, line 41 at r2 (raw file):

global_setup() {
    local ARTIFACTS_FOLDER="$1"
    set -e

Note that this is a global change for the current shell. So for example global_setup is called by acceptance/run, and it applies to the everything afterwards.

Using set -e is a reasonable thing to do, especially for this kind of env, but in that case i'd just set it globally at the top of the executable shell scripts, and lib.sh.

---

acceptance/lib.sh, line 51 at r2 (raw file):

        local SETUP_DOCKER_PERAPP="${ARTIFACTS_FOLDER}/global_setup_docker_perapp.out"
    fi
    run_command stop_infra $SETUP_PRE_CLEAN

This can be simplified a bit:

local out_dir="$1"
[[ -n "$out_dir" ]] && mkdir "$out_dir
...
run_command stop_infra ${out_dir:+$out_dir/global_setup_pre_clean.out}`
rm -f logs/*
run_command build_docker_base ${out_dir:+$out_dir/global_setup_docker_base.out}`
...

---

acceptance/lib.sh, line 60 at r2 (raw file):

test_setup_wrapper() {
    if run_command test_setup "$1"; then
        print_green "[ SETUP    ]" "$TEST_NAME" && true

This makes it look like you are testing the return value of print_green. I'd instead make it more explicit.

if run_command test_setup "$1"; then
   print_green ...
   return 0
fi
stats_failed...
print_...
return 1

---

acceptance/lib.sh, line 84 at r2 (raw file):

test_teardown_wrapper() {
    run_command test_teardown "$1"

Drop, it's covered by the next line.

---

acceptance/lib.sh, line 104 at r2 (raw file):

            RUN_FILE="$ARTIFACTS_FOLDER/$TEST_NAME/run.out"
            TEARDOWN_FILE="$ARTIFACTS_FOLDER/$TEST_NAME/teardown.out"
            test_setup_wrapper "$SETUP_FILE" && test_run_wrapper "$RUN_FILE" && test_teardown_wrapper "$TEARDOWN_FILE" && save_logs "$ARTIFACTS_FOLDER"

I'd wrap this:

test_setup_wrapper ... && \
    test_run_wrapper ... && \
...

---

acceptance/Readme.md, line 1 at r2 (raw file):

README.md seems to be the standard naming.

---

acceptance/Readme.md, line 8 at r2 (raw file):

`/acceptance`, with `xxx` replaced by the name of your test.

The folder must contain a `test.sh` file, which must define the following elements:

I would prefer to not lock us into tests being shell scripts. My suggestion would be that folder/test must exist, be executable, and it must take a set of subcommands that perform the desired actions. E.g. folder/test name, folder/test setup, folder/test run, etc. That way the test can be in whatever language suits best, and is decoupled from the framework.

---

acceptance/run, line 5 at r2 (raw file):

. acceptance/lib.sh

run_all() {

If this is being called unconditionally, then there's no reason to make it a function. Also $1 is currently always empty, as there are never any arguments being passed to it.

---

acceptance/run, line 7 at r2 (raw file):

run_all() {
    TEST_REGEX_MATCHER=$1
    ARTIFACTS_FOLDER="$(date +"%H-%M-%S").artifacts"

Use a tempdir for artifacts. ARTIFACTS_FOLDER=$(mktemp -d /tmp/acceptance-artifacts.XXXXXXX), and print it at the top of the run output.

Also, rather than passing it around to every function, i'd just declare it globally (as you have done here unintentionally i suspect), and reference it in the places that need it.

In the other direction, i'd pass the regex as a param to global_run, and not use a global var for that.

---

acceptance/reconnecting_acceptance/test.sh, line 12 at r2 (raw file):

test_setup() {
    set -e
    ./scion.sh topology -c $TEST_TOPOLOGY -d -sd=go -ps=go

I'm not thrilled with this, as it is destructive. Anything the user has already running is stopped, and their existing gen folder is thrown away.

On the other side, it isn't destructive enough, as it doesn't clear zookeeper.

@worxli is going to look at Docker-in-docker docker Magic™ to see if we can insulate this properly. In the meantime i'd add zkclean after topology.

---

acceptance/reconnecting_acceptance/test.sh, line 27 at r2 (raw file):

    python/integration/end2end_test.py 1-ff00:0:112 1-ff00:0:110
    docker restart dispatcher
    sqlite3 gen-cache/sd1-ff00_0_112.path.db "delete from segments;"

This implies that sqlite3 should be added to env/debian/pkgs.txt

[scrye]

Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @kormat and @scrye)

---

acceptance/color.sh, line 3 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Raw control codes aren't a great idea, as you can't count on all terminals/platforms interpreting them correctly. It's also good to be sure that there's a terminal attached. So:

if [ -t 1 ]; then
    GREEN=$(tput setaf 2)
    RED=$(tput setaf 9)
    YELLOW=$(tput setaf 11)
    NC=$(tput sgr0)
fi

(colors taken from https://jonasjacek.github.io/colors/)

If stdout is not a terminal, then the colour vars will be undefined, and the print_X functions will gracefully degrade.

Done.

---

acceptance/color.sh, line 8 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

To make these functions a bit easier to use, i'd do:

local prefix="$1"
shift
printf "${GREEN}$prefix{$NC} $@\n"

That way you can call print_green with any number of arguments.

Done.

---

acceptance/lib.sh, line 1 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

A shell script that is sourced and not executed shouldn't have a she-bang.

Done.

---

acceptance/lib.sh, line 6 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I'd use lowercase (and shorter) local var names. e.g cmd and out.

Done.

---

acceptance/lib.sh, line 12 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I think this can be simplified to:

local cmd="$1"
local out="$2"
$cmd &>> "${out:-/dev/stdout}"
local ret=$?
if [[ -n "$out" && $ret -ne 0 ]]; then
  cat "$OUTPUT_FILE"
fi
return $ret

(note that $cmd is deliberately unquoted, which allows a multi-arg command to be run).

Done.

---

acceptance/lib.sh, line 41 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Note that this is a global change for the current shell. So for example global_setup is called by acceptance/run, and it applies to the everything afterwards.

Using set -e is a reasonable thing to do, especially for this kind of env, but in that case i'd just set it globally at the top of the executable shell scripts, and lib.sh.

The goal here is to have each test stop on the first error within the test, while the main loop goes through all the tests. (Since they're independent, this collects information about every failing acceptance test instead of stopping on the first one).

Can this be done with subshells or something?

---

acceptance/lib.sh, line 51 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This can be simplified a bit:

local out_dir="$1"
[[ -n "$out_dir" ]] && mkdir "$out_dir
...
run_command stop_infra ${out_dir:+$out_dir/global_setup_pre_clean.out}`
rm -f logs/*
run_command build_docker_base ${out_dir:+$out_dir/global_setup_docker_base.out}`
...

Done.

---

acceptance/lib.sh, line 60 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This makes it look like you are testing the return value of print_green. I'd instead make it more explicit.

if run_command test_setup "$1"; then
   print_green ...
   return 0
fi
stats_failed...
print_...
return 1

Done.

---

acceptance/lib.sh, line 84 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Drop, it's covered by the next line.

Done.

---

acceptance/lib.sh, line 104 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I'd wrap this:

test_setup_wrapper ... && \
    test_run_wrapper ... && \
...

Done.

---

acceptance/Readme.md, line 1 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

README.md seems to be the standard naming.

Done.

---

acceptance/Readme.md, line 8 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I would prefer to not lock us into tests being shell scripts. My suggestion would be that folder/test must exist, be executable, and it must take a set of subcommands that perform the desired actions. E.g. folder/test name, folder/test setup, folder/test run, etc. That way the test can be in whatever language suits best, and is decoupled from the framework.

Done.

---

acceptance/run, line 5 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

If this is being called unconditionally, then there's no reason to make it a function. Also $1 is currently always empty, as there are never any arguments being passed to it.

Done.

---

acceptance/run, line 7 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Use a tempdir for artifacts. ARTIFACTS_FOLDER=$(mktemp -d /tmp/acceptance-artifacts.XXXXXXX), and print it at the top of the run output.

Also, rather than passing it around to every function, i'd just declare it globally (as you have done here unintentionally i suspect), and reference it in the places that need it.

In the other direction, i'd pass the regex as a param to global_run, and not use a global var for that.

Done.

---

acceptance/reconnecting_acceptance/test.sh, line 12 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I'm not thrilled with this, as it is destructive. Anything the user has already running is stopped, and their existing gen folder is thrown away.

On the other side, it isn't destructive enough, as it doesn't clear zookeeper.

@worxli is going to look at Docker-in-docker docker Magic™ to see if we can insulate this properly. In the meantime i'd add zkclean after topology.

Done.

---

acceptance/reconnecting_acceptance/test.sh, line 27 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This implies that sqlite3 should be added to env/debian/pkgs.txt

Done.

[kormat]

Reviewed 8 of 8 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @kormat and @scrye)

---

acceptance/lib.sh, line 41 at r2 (raw file):

Previously, scrye (Sergiu Costea) wrote…

The goal here is to have each test stop on the first error within the test, while the main loop goes through all the tests. (Since they're independent, this collects information about every failing acceptance test instead of stopping on the first one).

Can this be done with subshells or something?

Your addition of set +e at the end of the func should work fine, and i think the rest of the code will actually do what you want as-is. We can revisit if we actually run into issues at a later point.

global_run should be nuking the logs between runs though, as otherwise later acceptance tests will end up with the cumulative logs of all previous tests. The simple answer is probably just to change save_logs to do: mkdir -p "$out/logs"; mv logs/* "$out/logs".

---

acceptance/lib.sh, line 85 at r3 (raw file):

global_run() {
    local out="$ARTIFACTS_FOLDER"
	local regex_matcher="$1"

Your editor has betrayed you i think - there's 3 lines now with tabs in this file.

---

acceptance/lib.sh, line 101 at r3 (raw file):

            test_setup_wrapper "$SETUP_FILE" && \
                test_run_wrapper "$RUN_FILE" && \
                test_teardown_wrapper "$TEARDOWN_FILE" && \

Looking at this again, i think the logic should be a bit more fine-grained:

• if setup fails, skip the rest
• even if run fails, do teardown and log saving.

That way you don't run a test if its env isn't in place, and you try to clean up after a failed test.

---

acceptance/run, line 5 at r3 (raw file):

. acceptance/lib.sh

ARTIFACTS_FOLDER=$(mktemp -d /tmp/acceptance-artifacts-$(date +"%H-%M-%S").XXXXXXX)

If including a timestamp in the dir, might as well make it one that's unambiguous:

$ date +%Y%m%d-%H%M%S
20181009-153319

---

acceptance/README.md, line 14 at r3 (raw file):

* `run`, which runs the test itself (including assertions). If the return value
  of the function is non-zero, the test is considered to have failed.
* `teardown`, which cleans up after the test.

teardown should probably also return an error if it fails - if that happens you probably fail the entire run, as you have an inconsistent/unknown state.

[scrye]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @kormat)

---

acceptance/lib.sh, line 41 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Your addition of set +e at the end of the func should work fine, and i think the rest of the code will actually do what you want as-is. We can revisit if we actually run into issues at a later point.

global_run should be nuking the logs between runs though, as otherwise later acceptance tests will end up with the cumulative logs of all previous tests. The simple answer is probably just to change save_logs to do: mkdir -p "$out/logs"; mv logs/* "$out/logs".

Done.

---

acceptance/lib.sh, line 85 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Your editor has betrayed you i think - there's 3 lines now with tabs in this file.

Done.

---

acceptance/lib.sh, line 101 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Looking at this again, i think the logic should be a bit more fine-grained:

• if setup fails, skip the rest
• even if run fails, do teardown and log saving.

That way you don't run a test if its env isn't in place, and you try to clean up after a failed test.

Done.

---

acceptance/run, line 5 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

If including a timestamp in the dir, might as well make it one that's unambiguous:

$ date +%Y%m%d-%H%M%S
20181009-153319

Done.

---

acceptance/README.md, line 14 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

teardown should probably also return an error if it fails - if that happens you probably fail the entire run, as you have an inconsistent/unknown state.

Done.

[scrye]

---

acceptance/lib.sh, line 101 at r3 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Done.

With the mention that teardown now always happens, so that even stuff that is partially set up is cleaned up.

[kormat]

Reviewed 4 of 4 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @scrye)

---

acceptance/README.md, line 15 at r4 (raw file):

  of the function is non-zero, the test is considered to have failed.
* `teardown`, which cleans up after the test. If the return value of the
  function is non-zero, the run of the **entire** test suite is aborted.

It doesn't look like this is implemented yet.

[kormat]

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @scrye)

[scrye]

Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on @kormat)

---

acceptance/README.md, line 15 at r4 (raw file):

Previously, kormat (Stephen Shirley) wrote…

It doesn't look like this is implemented yet.

Done.

[kormat]

Reviewed 1 of 1 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved