Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/enable-e2e-tests-on-pull-requests #195

Merged
merged 24 commits into from Mar 13, 2018

Conversation

Projects
None yet
4 participants
@tadovas
Copy link
Member

commented Mar 8, 2018

No description provided.

.travis.yml Outdated
@@ -32,10 +35,15 @@ install:

script:
- bin/test
- bin/client_build
- bin/server_build
- if [ "$TRAVIS_OS_NAME" == 'linux' ] && [ "$TRAVIS_PULL_REQUEST" != "false" ]; then

This comment has been minimized.

Copy link
@donce

donce Mar 8, 2018

Contributor

Isn't "$TRAVIS_PULL_REQUEST" == "true" more obvious? :D

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 8, 2018

Author Member

From Travis documentatin about $TRAVIS_PULL_REQUEST:
"TRAVIS_PULL_REQUEST: The pull request number if the current job is a pull request, “false” if it’s not a pull request."
So if it's pull request this variable will not contain true, but the PR number

This comment has been minimized.

Copy link
@donce

donce Mar 8, 2018

Contributor

wow, that is confusing :D

@@ -0,0 +1,28 @@
FROM golang:1.9.2-alpine AS builder

This comment has been minimized.

Copy link
@donce

donce Mar 8, 2018

Contributor

Why do we need separate Dockerfile for e2e?

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 8, 2018

Author Member

Because we don't have lightweight docker image for building both node/client and ability to start them up.

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 8, 2018

Member

I agree, I see this Dockerfile.node as duplication of our official image.
+
You would be testing full integration (code + configs + docker image), because Docker is our recommended deployment

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 8, 2018

Author Member

Which one? I don't see any official images of building client + node on alpine

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 12, 2018

Member

mysteriumnetwork/mysterium-node:alpine
mysteriumnetwork/mysterium-client:alpine

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 12, 2018

Author Member

Ha ha, as they are commited one hour ago :) I will use them - thanks

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 12, 2018

Member

This Dockerfile.node is unused now

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 13, 2018

Author Member

Removed

var host = flag.String("tequila.host", "localhost", "Specify tequila host for e2e tests")
var port = flag.Int("tequila.port", 4050, "Specify tequila port for e2e tests")

func TestClientConnectsToNode(t *testing.T) {

This comment has been minimized.

Copy link
@donce

donce Mar 8, 2018

Contributor

Method name is misleading - it doesn't test that.

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 8, 2018

Author Member

Renamed.

@@ -0,0 +1,28 @@
FROM golang:1.9.2-alpine AS builder

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 8, 2018

Member

I agree, I see this Dockerfile.node as duplication of our official image.
+
You would be testing full integration (code + configs + docker image), because Docker is our recommended deployment

COPY --from=builder /go/src/github.com/mysterium/node/bin/client_package/config client_config


ENTRYPOINT ["/mysterium/mysterium_server"]

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 8, 2018

Member

You compile 2 executables, but use only mysterium_server. Do you have a case then You need run both executables in same machine?

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 8, 2018

Author Member

I am using the same image to start both server and node container - in that case container is built once, with two execs ready. See docker-compose node and client definitions.

ports:
- 4052:4050
entrypoint:
"/mysterium/mysterium_client"

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 8, 2018

Member

We have Dockerfile for client also, just nobody is testing them. Would be nice integration covers existing tools. Because they are breaking from time to time: Dockerfile of server, Dockerfile of client

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 8, 2018

Author Member

Let's make a clean separation between integration testing and packaging testing

@@ -11,7 +11,7 @@

ARGUMENTS=$@
if [ -z "$ARGUMENTS" ]; then
ARGUMENTS=`go list ./...`
ARGUMENTS=`go list ./... | sed '/e2e/d'` #skip e2e package - integration tests by default

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 8, 2018

Member

Go test has some tags feature, would be good to investigate that

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 8, 2018

Author Member

Tags have some limitations when it comes for full package of tests switching of. Tried that - does not help

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 12, 2018

Member

Anyway would be nice to filter by full package name

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 12, 2018

Author Member

Yea will need to investigate further - maybe some other time.

- MKNOD
- NET_ADMIN
ports:
- 4052:4050

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 8, 2018

Member

Do we really have to take hosts port then running tests? What if tests are run in docker-compose's neighbour container?

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 8, 2018

Author Member

Possible, but a bit complicated - at the moment is simplest way to run tests agains full started up system with only single port exposed

.travis.yml Outdated
- if [ "$TRAVIS_OS_NAME" == 'linux' ] && [ "$TRAVIS_PULL_REQUEST" != "false" ]; then
bin/travis_scripts/check_docker.sh;
bin/travis_scripts/check_docker_compose.sh;
bin/run_e2e_tests;

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 8, 2018

Member

bin/test and bin/test_e2e

then
print_error "Unexpected status response: $result"
print_error "Tests failed"

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 8, 2018

Member

Why not cleaning containers if tests succeeds?

This comment has been minimized.

Copy link
@donce

donce Mar 8, 2018

Contributor

It is being cleaned-up, look where cleanup function is invoked.

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 8, 2018

Author Member

it is:
print_success "Tests passed"
cleanup

@@ -0,0 +1,4 @@
#!/usr/bin/env bash
set -e

This comment has been minimized.

Copy link
@zolia

zolia Mar 12, 2018

Member

What is the point to use set -e if there is only 1 command?

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 12, 2018

Author Member

Reserved for future usage :)

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 12, 2018

Author Member

Fixed

@tadovas tadovas force-pushed the feature/master-integration-tests branch from a127286 to e504176 Mar 12, 2018

tadovas added some commits Mar 12, 2018

@@ -5,13 +5,3 @@ if [ ! -f .env ]; then
exit 1
fi
source .env

COMMIT="$TRAVIS_COMMIT"

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 12, 2018

Member

Should we move these to .evn_example maybe?

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 13, 2018

Author Member

Well they are optional and travis specific anyway. Should we trash that .env file even more?

@@ -0,0 +1,28 @@
FROM golang:1.9.2-alpine AS builder

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 12, 2018

Member

This Dockerfile.node is unused now

@Waldz

Waldz approved these changes Mar 13, 2018

@Waldz Waldz merged commit 2a96aeb into master Mar 13, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Waldz Waldz deleted the feature/master-integration-tests branch Mar 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.