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

Tests: Ginkgo test framework #1733

Merged
merged 1 commit into from Oct 26, 2017

Conversation

Projects
None yet
7 participants
@eloycoto
Copy link
Member

eloycoto commented Oct 10, 2017

Hello!

This PR is still WIP, at the moment all basic test should be in there, but I'll like to add the following test before get it merged:

  • Runtime: Cilium monitor
  • Kubernetes: Node-port

On the other hand, I didn't see any test related with the option -lb and maybe it's needed to add a test for that.

Related with other work to do:

  • At the moment, this Jenkins is not executing the go unittest; I want to speak with Ian before doing something here.
  • The Readme explains how to run the test, but maybe you prefer a Makefile that can have the following:
    make runtime
    make k8s-1.6
    make k8s-1.7
    make k8s-1.8

Please, can you have a look and make your notes/suggestions in this PR?

Regards

log "github.com/sirupsen/logrus"
)

var DefaultSettings map[string]string = map[string]string{

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

should omit type map[string]string from declaration of var DefaultSettings; it will be inferred from the right-hand side

"github.com/cilium/cilium/test/helpers"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

should not use dot imports


"github.com/cilium/cilium/test/helpers"

. "github.com/onsi/ginkgo"

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

should not use dot imports


"github.com/cilium/cilium/test/helpers"
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

should not use dot imports

"time"

"github.com/cilium/cilium/test/helpers"
. "github.com/onsi/ginkgo"

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

should not use dot imports

package k8sT

import (
. "github.com/onsi/ginkgo"

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

should not use dot imports


"github.com/cilium/cilium/test/helpers"
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

should not use dot imports

"time"

"github.com/cilium/cilium/test/helpers"
. "github.com/onsi/ginkgo"

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

should not use dot imports

"github.com/cilium/cilium/test/helpers"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

should not use dot imports

"github.com/asaskevich/govalidator"
"github.com/cilium/cilium/test/helpers"

. "github.com/onsi/ginkgo"

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

should not use dot imports

@eloycoto eloycoto force-pushed the eloycoto:ginkgo branch from 615f1db to cf750f7 Oct 10, 2017

return result, nil
}

//KVOutput: This is a helper functon that return a map with the key=val output.

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

comment on exported method CmdRes.KVOutput should be of the form "KVOutput ..."

return strings.Trim(res.stdout.String(), "\n")
}

func (res *CmdRes) FindResults(filter string) ([]reflect.Value, error) {

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

exported method CmdRes.FindResults should have comment or be unexported

exit bool
}

//Correct: return true if the command was sucessfull

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

comment on exported method CmdRes.Correct should be of the form "Correct ..."

"k8s.io/client-go/util/jsonpath"
)

type CmdRes struct {

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

exported type CmdRes should have comment or be unexported

return result, nil
}

//KVOutput: This is a helper functon that return a map with the key=val output.

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

comment on exported method CmdRes.KVOutput should be of the form "KVOutput ..."

return strings.Trim(res.stdout.String(), "\n")
}

func (res *CmdRes) FindResults(filter string) ([]reflect.Value, error) {

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

exported method CmdRes.FindResults should have comment or be unexported

exit bool
}

//Correct: return true if the command was sucessfull

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

comment on exported method CmdRes.Correct should be of the form "Correct ..."

"k8s.io/client-go/util/jsonpath"
)

type CmdRes struct {

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

exported type CmdRes should have comment or be unexported

@eloycoto eloycoto force-pushed the eloycoto:ginkgo branch from cf750f7 to f93075f Oct 10, 2017

"github.com/cilium/cilium/test/helpers"
)

//GetScope: return scope for running test

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

comment on exported function GetScope should be of the form "GetScope ..."

return addTestType(text, body, "IT", afterAll, timeout...)
}

//You can focus individual Its using FIt. this contains AfterAll wrapper

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

comment on exported function FIt should be of the form "FIt ..."

return ginkgoFunc(text, wrappedBody, timeout...)
}

//Text block with AfterAll wrapper

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

comment on exported function It should be of the form "It ..."

"github.com/onsi/ginkgo"
)

type AfterAll struct {

This comment has been minimized.

@houndci-bot

houndci-bot Oct 10, 2017

exported type AfterAll should have comment or be unexported

@eloycoto eloycoto force-pushed the eloycoto:ginkgo branch 2 times, most recently from e809277 to fdbe0c2 Oct 10, 2017

@nebril nebril added the wip label Oct 10, 2017

@ianvernon

This comment has been minimized.

Copy link
Contributor

ianvernon commented Oct 10, 2017

I cannot tell you how excited I am for this! My review is WIP - given the size of this, it will take a while.
To avoid with the review process, can you provide comments that describe which parts of the tests that currently exist in master map to the new gingko tests so that we can ensure that the tests correspond 1:1 with what we have in the current bash testing framework? This will help significantly with ensuring that no coverage is lost in the transition to the new framework.

@eloycoto

This comment has been minimized.

Copy link
Member Author

eloycoto commented Oct 10, 2017

Hello!

A bunch of test are duplicated, I made the list on the issue #1589 where I map all the test to there. So if that it's not clear to you I can map 1:1 with the bash test.

Regards

@eloycoto eloycoto force-pushed the eloycoto:ginkgo branch 3 times, most recently from 73f7778 to 994e204 Oct 11, 2017

@@ -0,0 +1,209 @@
package k8sT

This comment has been minimized.

@nebril

nebril Oct 11, 2017

Member

Can this package be called in a more descriptive way? Like k8sTests?

@@ -0,0 +1,652 @@
package RunT

This comment has been minimized.

@nebril

nebril Oct 11, 2017

Member

Would be nice to have this named "RuntimeTests"

Expect(res.Correct()).Should(BeTrue())
})

It("Default values", func() {

This comment has been minimized.

@nebril

nebril Oct 11, 2017

Member

Nitpick: "It Default values" doesn't really give much information. What about It("should disable policy enforcement with defauklt values")? It can be done later as I understand that this is direct conversion from bash scripts.

@joestringer
Copy link
Contributor

joestringer left a comment

I noted a few minor issues here from an initial scan through.

ciliumPod, err := kubectl.GetCiliumPodOnNode("kube-system", "k8s1")
Expect(err).Should(BeNil())

//Check that cilium detects a

This comment has been minimized.

@joestringer

joestringer Oct 11, 2017

Contributor

Incomplete comment? Also below.

if [[ "$(hostname)" == "k8s1" ]]; then
make docker-image-dev
docker tag cilium 192.168.36.11:5000/cilium/cilium-dev
docker push 192.168.36.11:5000/cilium/cilium-dev

This comment has been minimized.

@joestringer

joestringer Oct 11, 2017

Contributor

Can we refactor these hard-coded IPs to a single place?

There's a few more of these, I won't highlight every one.

This comment has been minimized.

@nebril

nebril Oct 16, 2017

Member

The problem is that these are used in k8s manifests as docker registry addresses. Could we put this ip into vm /etc/hosts and use hostname in k8s manifests?

Expect(res.Correct()).Should(BeTrue())
}, 300)

It("Test containers connectivity WITH policy", func() {

This comment has been minimized.

@joestringer

joestringer Oct 11, 2017

Contributor

Perhaps we could share some of the common test code between this and Test containers connectivity without policy?

@@ -1,3 +1,3 @@
PATH=/usr/lib/llvm-3.8/bin:/usr/local/sbin:/usr/local/bin:/usr/bin:/usr/sbin:/sbin:/bin
CILIUM_OPTS=--kvstore consul --kvstore-opt consul.address=127.0.0.1:8500
CILIUM_OPTS=--kvstore consul --kvstore-opt consul.address=127.0.0.1:8500 --debug

This comment has been minimized.

@joestringer

joestringer Oct 11, 2017

Contributor

Will this affect all deployments, or just in the testing? Not sure if this is unrelated change that should be kept out.

This comment has been minimized.

@eloycoto

eloycoto Oct 11, 2017

Author Member

My fault! I'll remove it ;-)

@nebril
Copy link
Member

nebril left a comment

Overall looks like a great job! Left some questions and suggestions inline.

log "github.com/sirupsen/logrus"
)

var _ = Describe("RunConnectivyTest", func() {

This comment has been minimized.

@nebril

nebril Oct 16, 2017

Member

s/Connectivy/Connectivity

Jenkinsfile Outdated
sh './tests/k8s/start.sh'
}
"Runtime":{
sh 'cd ${TESTDIR}; ginkgo --focus="Run*" -v -noColor'

This comment has been minimized.

@nebril

nebril Oct 16, 2017

Member

Nit: can we change runtime test name convention from Run<test name> to Runtime<test name>?

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

Would it be possible to get coverage results of the Cilium commands using ginkgo -cover ? Or is that not possible since we are running cilium commands via its CLI and are not actually testing Cilium code directly with Ginkgo?

)

var _ = Describe("K8s", func() {
// Describe("Categorizing book length", func() {

This comment has been minimized.

@nebril

nebril Oct 16, 2017

Member

Do we need to keep this example? There are plenty of tests to check for examples, I would like this file gone. If we really want it to stay, I think it should be uncommented.

if [[ "$(hostname)" == "k8s1" ]]; then
make docker-image-dev
docker tag cilium 192.168.36.11:5000/cilium/cilium-dev
docker push 192.168.36.11:5000/cilium/cilium-dev

This comment has been minimized.

@nebril

nebril Oct 16, 2017

Member

The problem is that these are used in k8s manifests as docker registry addresses. Could we put this ip into vm /etc/hosts and use hostname in k8s manifests?

@@ -0,0 +1,116 @@
# Cilium Test Suite

This comment has been minimized.

@nebril

nebril Oct 16, 2017

Member

Shouldn't this be a part of Documentation/contributing.rst?

killCmd := "sudo kill -9 $(pgrep cilium-agent)"
go docker.Node.Exec(cmd)
timeout := time.After(300 * time.Second)
for {

This comment has been minimized.

@nebril

nebril Oct 16, 2017

Member

Both select cases are returning from the func, so for is not needed here.

}

done := make(chan string, 1)
agent := func(option string) {

This comment has been minimized.

@nebril

nebril Oct 16, 2017

Member

Please make the agent accept done channel as an argument so that each test is separated. Otherwise ending of one test may cause the first one to kill node from other test.

DisableTimestamp: true,
})

// var filename string = "test.log"

This comment has been minimized.

@nebril

nebril Oct 16, 2017

Member

Please remove commented out code.

var vagrant helpers.Vagrant

func init() {
// log.SetOutput(os.Stdout)

This comment has been minimized.

@nebril

nebril Oct 16, 2017

Member

Please remove commented out code.

@@ -0,0 +1,7 @@
package ciliumTest

This comment has been minimized.

@nebril

nebril Oct 16, 2017

Member

What is the purpose of this package?

@ianvernon
Copy link
Contributor

ianvernon left a comment

These are some initial comments from what I have reviewed thus far. Given the magnitude of this patch, I didn't want to wait until I was completely done with review to provide initial comments so that the work on what I have suggested can be done in parallel with my upcoming review of what I haven't looked at yet.

Jenkinsfile Outdated
junit 'test/*.xml'
sh 'cd test/; vagrant destroy -f'
sh 'cd test/; K8S_VERSION=1.6; vagrant destroy -f'
}

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

Need cleanup for K8s 1.7 VMs

Jenkinsfile Outdated
sh 'cd ./tests/k8s && vagrant destroy -f || true'
sh './tests/k8s/start.sh'
}
"Runtime":{

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

Add unit tests stage as we discussed. We will need to install Docker on the Jenkins hosts to run the unit tests in a Docker container.

Jenkinsfile Outdated
sh 'cd ./tests/k8s && vagrant destroy -f'
post {
always {
junit 'test/*.xml'

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

We should still be gathering logs from our tests as was done before on lines 34-37, as we need as many datapoints to debug failures as possible.

# -*- mode: ruby -*-
# vi: set ft=ruby :

$build_number = ENV['BUILD_NUMBER'] || "0"

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

We need more differentiation in build number per VM. Example: say if we have two builds running with the same value for BUILD_NUMBER on the Jenkins slave - there will be interactions that we are unsure of. Is there a limitation in the existing VM that is launched with cilium/Vagrantfile that made you need to use this VM instead? It's ideal to keep the # of VMs that we have to manage to a minimum.

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

cc @aanm regarding this new Vagrantfile. What are your thoughts?

This comment has been minimized.

@eloycoto

eloycoto Oct 16, 2017

Author Member

Hey,

Thanks for the review, a lot of work to do, thanks! :-)

Not sure if I understand this, the network part is different per each K8S_VERSION:

            server.vm.network "private_network",
                ip: "192.168.36.1#{i}",
                virtualbox__intnet: "cilium-k8s#{$build_number}-#{$K8S_VERSION}"

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

see cilium/Vagrantfile:L72-82:

# Create unique ID for use in vboxnet name so Jenkins pipeline can have concurrent builds.
$job_name = ENV['JOB_BASE_NAME'] || "local"

$build_number = ENV['BUILD_NUMBER'] || "0"
$build_id = "#{$job_name}-#{$build_number}"

# Only create the build_id_name for Jenkins environment so that
# we can run VMs locally without having any the `build_id` in the name.
if ENV['BUILD_NUMBER'] then
    $build_id_name = "-build-#{$build_id}"
end

If there are two jobs running concurrently on the same Jenkins slave for PR 1 and PR 2, each with BUILD_NUMBER=1 , then both will be part of the same virtualbox__intnetwhich we don't want. We need to have the JOB_BASE_NAME to differentiate between Jenkins jobs that might be using the same BUILD_NUMBER .

server.vm.provision "shell" do |sh|
sh.path = "./provision/k8s_install.sh"
sh.args = ["k8s#{i}", "192.168.36.1#{i}", "#{$K8S_version}"]
end

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

Indentation is inconsistent here.

return fmt.Sprintf("%s/runtime/manifests/", basePath)
}

//GetFullPath return the valid path for a file

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

returns

return fmt.Sprintf("%s%s", c.ManifestsPath(), name)
}

//PolicyEndpointsSummary Return a map of the status of the policies

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

returns the count of whether policy enforcement is enabled, disabled, and the total number of endpoints, and an error if the Cilium endpoint metadata cannot be retrieved via the API.

return result, nil
}

//PolicyEnforcementSet set policyEnforcement in endpoint

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

sets the PolicyEnforcement configuration value for the Cilium agent to the provided status.

return res
}

//PolicyDel delete a given policy

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

deletes a policy with the given ID

return rev.IntOutput()
}

//PolicyImport Import a new policy in cilium and wait until all endpoints

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

imports a new policy into Cilium and waits until the policy revision number increments.

@ianvernon
Copy link
Contributor

ianvernon left a comment

more comments

func addTestType(text string, body func(), testType string, control *AfterAll, timeout ...float64) bool {
var ginkgoFunc func(text string, body interface{}, timeout ...float64) bool

//FIXME: XIT functions

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

What does this mean? If this is something that can be fixed after this initial patch is merged as an enhancement, please create a follow-up GitHub issue, refer to this PR / its corresponding issue, and add GH- in this FIXME comment so we can be sure that we have an issue tracking this FIXME.

var data []models.Endpoint
err := c.Exec(fmt.Sprintf("endpoint get %s", id)).UnMarshal(&data)
if err != nil {
c.logCxt.Infof("EndpointsGet fail %d: %s", id, err)

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

Make sure to update this log message when you rename the function (EndpointsGet --> EndpointGet).

Jenkinsfile Outdated
sh './tests/k8s/start.sh'
}
"Runtime":{
sh 'cd ${TESTDIR}; ginkgo --focus="Run*" -v -noColor'

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

Would it be possible to get coverage results of the Cilium commands using ginkgo -cover ? Or is that not possible since we are running cilium commands via its CLI and are not actually testing Cilium code directly with Ginkgo?

# -*- mode: ruby -*-
# vi: set ft=ruby :

$build_number = ENV['BUILD_NUMBER'] || "0"

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

cc @aanm regarding this new Vagrantfile. What are your thoughts?

"github.com/onsi/ginkgo"
)

//AfterAll struct to run after all process finish

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

Would change to "AfterAll is a structure whose Body is called after all processes finish".
What is the scope of these processes? I.e., are these called after the suite is finished, after a test is finished, etc. - please clarify in the description / comment.

@@ -0,0 +1,69 @@
package k8sT

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

add copyright header

@@ -0,0 +1,119 @@
package k8sT

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

add copyright header

ciliumPod, err := kubectl.GetCiliumPodOnNode("kube-system", "k8s1")
Expect(err).Should(BeNil())

//Check that cilium detects a

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

Incomplete comment

@@ -0,0 +1,17 @@
package k8sT

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

add copyright header if this file is going to be kept. I think this can be deleted.

docker tag cilium 192.168.36.11:5000/cilium/cilium-dev
docker push 192.168.36.11:5000/cilium/cilium-dev
else
echo "No master, no need to compile"

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

This log is not clear; change to not on master K8s node; no need to compile Cilium

@ianvernon ianvernon requested a review from aanm Oct 16, 2017

@ianvernon
Copy link
Contributor

ianvernon left a comment

more comments. I still need to review the tests' content to ensure that they cover what currently exists in in the bash scripts.

key: node-role.kubernetes.io/master
- effect: NoSchedule
key: node.cloudprovider.kubernetes.io/uninitialized
value: "true"

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

Having this many DaemonSet files to maintain will be very difficult for us, as we have to update examples/minikube, examples/kubernetes, etc. Is there a way we can just use the files located in examples/ directory instead of duplicating them here?

Expect(endPoints["disabled"]).To(Equal(1))
})

By("Apply a new policy")

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

specify the path of the policy

@@ -0,0 +1,652 @@
package RunT

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

add copyright header


var _ = Describe("RunPolicies", func() {

var initilized bool

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

initilized --> initialized

var docker *helpers.Docker
var cilium *helpers.Cilium

initilize := func() {

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

initilize --> initialize

}, 500)

It("Service L4 tests", func() {
// createInterface(docker.Node)

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

Remove this if it is not being used.

}
path := "/vagrant/create_veth_interface"
node.Exec("sudo ip addr add fd02:1:1:1:1:1:1:1 dev cilium_host")
//FIXME: Here we need to check if executes correctly

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

Error needs to be handled before this patch is merged.

@@ -0,0 +1,43 @@
[

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

can you rename these policy files to correspond to the test in which they are used ?
e.g., Policies-l7-simple.json

@@ -0,0 +1,7 @@
package ciliumTest

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

add copyright header

@@ -0,0 +1,90 @@
package ciliumTest

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

add copyright header

@ianvernon
Copy link
Contributor

ianvernon left a comment

Added a few more comments. We need to ensure that the coverage is equivalent to what we have in cilium/tests/k8s currently (minus the stress tests, as I think we can convert those in a later PR / in the nightly builds we proposed last week).
I think we still need to port tests/k8s/tests/01-guestbook.sh and tests/k8s/tests/02-cnp-specs.sh as well. @aanm please confirm.


It("PolicyEnforcement Changes", func() {
//This is a small test that check that everything is working in k8s. Full monkey testing
// is on runtime/Policies

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

is in

res, err := kubectl.GetPodsNames("default", fmt.Sprintf("id=%s", v))
Expect(err).Should(BeNil())
appPods[v] = res[0]
logger.Infof("PolicyRulesTest: pod='%s' assigned to '%s'", res[0], v)

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

Use log.WithFields

}, 300)

//FIXME: Check service with IPV6
//FIXME: Check the service with cross-node

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

I think this is a blocker for merging and that it is critical that we have cross-service tests as is done in tests/k8s/tests/02-cnp-specs.sh. cc: @aanm do you agree?

var kubectl *helpers.Kubectl
var demoDSPath string
var logger *log.Entry
var initilized bool

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

initilized --> initialized

var logger *log.Entry
var initilized bool

initilize := func() {

This comment has been minimized.

@ianvernon

ianvernon Oct 16, 2017

Contributor

initilize --> initialize


"github.com/cilium/cilium/test/helpers"
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"

This comment has been minimized.

@houndci-bot

houndci-bot Oct 17, 2017

should not use dot imports

"fmt"

"github.com/cilium/cilium/test/helpers"
. "github.com/onsi/ginkgo"

This comment has been minimized.

@houndci-bot

houndci-bot Oct 17, 2017

should not use dot imports


"github.com/cilium/cilium/test/helpers"
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"

This comment has been minimized.

@houndci-bot

houndci-bot Oct 17, 2017

should not use dot imports

"context"

"github.com/cilium/cilium/test/helpers"
. "github.com/onsi/ginkgo"

This comment has been minimized.

@houndci-bot

houndci-bot Oct 17, 2017

should not use dot imports


import (
"github.com/cilium/cilium/test/helpers"
. "github.com/onsi/ginkgo"

This comment has been minimized.

@houndci-bot

houndci-bot Oct 17, 2017

should not use dot imports

@eloycoto eloycoto force-pushed the eloycoto:ginkgo branch 3 times, most recently from de60e7e to 684fb27 Oct 17, 2017

@cilium cilium deleted a comment from houndci-bot Oct 17, 2017

@cilium cilium deleted a comment from houndci-bot Oct 17, 2017

@cilium cilium deleted a comment from houndci-bot Oct 17, 2017

@aanm

aanm approved these changes Oct 23, 2017

Copy link
Member

aanm left a comment

LGTM, please update the vendor.conf with the new dependencies added so we can merge it.

The remaining comments can be fixed after

@ianvernon
Copy link
Contributor

ianvernon left a comment

I'll wait for the Jenkinsfile changes to be made (keeping the old Jenkinsfile around) before giving my final approval. Please address my comment re: dependency versions as this directly affects the running of the tests on the Cilium Jenkins slaves.


Before run any test, you should have the following tools installed.

- Virtualbox

This comment has been minimized.

@ianvernon

ianvernon Oct 23, 2017

Contributor

Specify which versions of these dependencies should be installed.
On the Cilium build slaves, we have:

  • VirtualBox 5.1
  • Vagrant 2.0.0

Version of Docker, Docker Compose, Gingko, and Gomega and golang are not currently specified for the build slaves. Is go 1.9 alright? I can take care of this as part of #1768 .

log "github.com/sirupsen/logrus"

"github.com/cilium/cilium/api/v1/models"
"github.com/cilium/cilium/pkg/k8s"

This comment has been minimized.

@ianvernon

ianvernon Oct 23, 2017

Contributor

Remove this dependency. This dependency has a transitive dependency of pkg/bpf which makes it impossible to run Ginkgo on a Mac setup, which is required for these tests. The only use of this pkg is lines 132-133. I'm OK with duplicating those constants for now and then adding a TODO / FIXME and explain why they are not being pulled in from the pkg/k8s pkg.

}

//NetworkCreate creates a Docker network of the provided name with the specified subnet
func (do *Docker) NetworkCreate(name string, subnet string) *CmdRes {

This comment has been minimized.

@joestringer

joestringer Oct 23, 2017

Contributor

The caller of these NetworkCreate(), NetworkDelete(), ContainerCreate(), ContainerRm() functions never check the return values; Perhaps these should be modified to not return anything, then use ginkgo.Fail() if there's ever any error that occurs. This should ensure that we write our tests to properly instantiate and clean up containers and networks.

We may also consider just creating the docker network during BeforeSuite() if we just need it to exist for every test. That would simplify the tests.

This comment has been minimized.

@joestringer

joestringer Oct 23, 2017

Contributor

That last suggestion is a bit more invasive, would involve refactoring all of the individual tests' initialize() functions to run in BeforeSuite() so that you could have access to the docker instance.

Expect(links).Should(Equal(originalLinks),
"Some network interfaces were accidentally removed!")
}, 300)
})

This comment has been minimized.

@ianvernon

ianvernon Oct 23, 2017

Contributor

TODO: Is there a way for us to cleanup the objects we have created? I.e., the opposite of initialize? That way we leave the system in the same state it was in when the test started. In the current bash scripts we have a trap finish-test EXIT in each test script which runs a set of commands when a test completes (either exit or failure).


"github.com/cilium/cilium/test/helpers"

. "github.com/onsi/ginkgo"

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2017

should not use dot imports

@ianvernon

This comment has been minimized.

Copy link
Contributor

ianvernon commented Oct 25, 2017

@eloycoto can you please rebase against master and trigger the build again? Not sure what the cause of failure was here. Looks unrelated to your changes.

@eloycoto eloycoto force-pushed the eloycoto:ginkgo branch from 900bffe to cd00dcf Oct 25, 2017


"github.com/cilium/cilium/test/helpers"
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2017

should not use dot imports

"fmt"

"github.com/cilium/cilium/test/helpers"
. "github.com/onsi/ginkgo"

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2017

should not use dot imports


"github.com/cilium/cilium/test/helpers"
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2017

should not use dot imports

"fmt"

"github.com/cilium/cilium/test/helpers"
. "github.com/onsi/ginkgo"

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2017

should not use dot imports

@tgraf

tgraf approved these changes Oct 25, 2017

@eloycoto eloycoto force-pushed the eloycoto:ginkgo branch from cd00dcf to 87ed530 Oct 25, 2017

return true
}

var EndpointWaitUntilReadyRetry int = 0 //List how many retries EndpointWaitUntilReady should have

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2017

exported var EndpointWaitUntilReadyRetry should have comment or be unexported
should drop = 0 from declaration of var EndpointWaitUntilReadyRetry; it is the zero value

)

const (
MaxRetries = 30

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2017

exported const MaxRetries should have comment (or a comment on this block) or be unexported

return true
}

var EndpointWaitUntilReadyRetry int = 0 //List how many retries EndpointWaitUntilReady should have

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2017

exported var EndpointWaitUntilReadyRetry should have comment or be unexported
should drop = 0 from declaration of var EndpointWaitUntilReadyRetry; it is the zero value

)

const (
MaxRetries = 30

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2017

exported const MaxRetries should have comment (or a comment on this block) or be unexported

@eloycoto eloycoto force-pushed the eloycoto:ginkgo branch from 87ed530 to 986f5aa Oct 25, 2017

Tests: Ginkgo test framework
Added the migration of current test platform to the new one using
Ginkgo. These changes add the test/ folder where all test_suites are
defined and described.

On the other hand, created a new basebox that had all cilium
dependencies in place, so provision don't need to install all the deps
each time that want to run the test.

Updated dependencies and added Gingko, gomega and ssh_config

Signed-off-by: Eloy Coto <eloy.coto@gmail.com>

@eloycoto eloycoto force-pushed the eloycoto:ginkgo branch from 986f5aa to 25e0de9 Oct 26, 2017

@tgraf tgraf merged commit f0741a6 into cilium:master Oct 26, 2017

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
hound 18 violations found.
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.