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

[Backend] Refactor integration tests, facilitate local testing #3138

Merged
merged 8 commits into from
Feb 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions backend/test/integration/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
## Api Server Integration Tests

### WARNING
**These integration tests will delete all the data in your KFP instance, please only use a test cluster to run these.**

### How to run

1. Configure kubectl to connect to your kfp cluster.
2. Run the following for all integration tests: `NAMESPACE=<kfp-namespace> ./run_tests_locally.sh`.
3. Or run the following to select certain tests: `NAMESPACE=<kfp-namespace> ./run_tests_locally.sh -testify.m Job`.
Reference: https://stackoverflow.com/a/43312451
26 changes: 20 additions & 6 deletions backend/test/integration/experiment_api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,21 @@ func (s *ExperimentApiTest) SetupTest() {
return
}

err := test.WaitForReady(*namespace, *initializeTimeout)
if err != nil {
glog.Exitf("Failed to initialize test. Error: %v", err)
if !*isDevMode {
err := test.WaitForReady(*namespace, *initializeTimeout)
if err != nil {
glog.Exitf("Failed to initialize test. Error: %v", err)
}
}
s.namespace = *namespace
clientConfig := test.GetClientConfig(*namespace)
var err error
s.experimentClient, err = api_server.NewExperimentClient(clientConfig, false)
if err != nil {
glog.Exitf("Failed to get experiment client. Error: %v", err)
}

s.cleanUp()
}

func (s *ExperimentApiTest) TestExperimentAPI() {
Expand Down Expand Up @@ -154,11 +159,20 @@ func (s *ExperimentApiTest) TestExperimentAPI() {
experiment, err = s.experimentClient.Get(&params.GetExperimentParams{ID: trainingExperiment.ID})
assert.Nil(t, err)
assert.Equal(t, expectedTrainingExperiment, experiment)

/* ---------- Clean up ---------- */
test.DeleteAllExperiments(s.experimentClient, t)
}

func TestExperimentAPI(t *testing.T) {
suite.Run(t, new(ExperimentApiTest))
}

func (s *ExperimentApiTest) TearDownSuite() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this method used in manual process? Didn't see it called in the script.

Copy link
Contributor Author

@Bobgy Bobgy Feb 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks!
/lgtm
/approve

if *runIntegrationTests {
if !*isDevMode {
s.cleanUp()
}
}
}

func (s *ExperimentApiTest) cleanUp() {
test.DeleteAllExperiments(s.experimentClient, s.T())
}
7 changes: 7 additions & 0 deletions backend/test/integration/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,10 @@ import (
var namespace = flag.String("namespace", "kubeflow", "The namespace ml pipeline deployed to")
var initializeTimeout = flag.Duration("initializeTimeout", 2*time.Minute, "Duration to wait for test initialization")
var runIntegrationTests = flag.Bool("runIntegrationTests", false, "Whether to also run integration tests that call the service")

/**
* Differences in dev mode:
* 1. Resources are not cleaned up when a test finishes, so that developer can debug manually.
* 2. One step that doesn't work locally is skipped.
*/
var isDevMode = flag.Bool("isDevMode", false, "Dev mode helps local development of integration tests")
32 changes: 23 additions & 9 deletions backend/test/integration/job_api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,15 @@ func (s *JobApiTestSuite) SetupTest() {
return
}

err := test.WaitForReady(*namespace, *initializeTimeout)
if err != nil {
glog.Exitf("Failed to initialize test. Error: %s", err.Error())
if !*isDevMode {
err := test.WaitForReady(*namespace, *initializeTimeout)
if err != nil {
glog.Exitf("Failed to initialize test. Error: %s", err.Error())
}
}
s.namespace = *namespace
clientConfig := test.GetClientConfig(*namespace)
var err error
s.experimentClient, err = api_server.NewExperimentClient(clientConfig, false)
if err != nil {
glog.Exitf("Failed to get pipeline upload client. Error: %s", err.Error())
Expand All @@ -67,6 +70,8 @@ func (s *JobApiTestSuite) SetupTest() {
if err != nil {
glog.Exitf("Failed to get job client. Error: %s", err.Error())
}

s.cleanUp()
}

func (s *JobApiTestSuite) TestJobApis() {
Expand Down Expand Up @@ -207,12 +212,6 @@ func (s *JobApiTestSuite) TestJobApis() {
assert.Equal(t, 1, totalSize)
argParamsRun := runs[0]
s.checkArgParamsRun(t, argParamsRun, argParamsExperiment.ID, argParamsExperiment.Name, argParamsJob.ID, argParamsJob.Name)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that you make sure all the pipelines/runs/experiments/... are cleaned up before the test begins in SetupTests() methods, and therefore you don't need to clean up those pipelines/runs/experiments/... after the test ends, which enables the dev to debug integration test errors after the test ends. Is that correct? If so, can we add comments here as why we don't clean up after test ends (since conventionally we do)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's correct.
Another benefit is, if you halt your test in the middle, rerunning the test won't fail because of cluster in an intermediate state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized, frontend-integration-test actually fails after this.
So I guess either we need to add cluster cleanup in frontend-integration-test, or we still need clean up after each integration test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated implementation to use a flag to turn off resource clean up when developing it.

/* ---------- Clean up ---------- */
test.DeleteAllExperiments(s.experimentClient, t)
test.DeleteAllPipelines(s.pipelineClient, t)
test.DeleteAllJobs(s.jobClient, t)
test.DeleteAllRuns(s.runClient, t)
}

func (s *JobApiTestSuite) checkHelloWorldJob(t *testing.T, job *job_model.APIJob, experimentID string, experimentName string, pipelineID string) {
Expand Down Expand Up @@ -312,3 +311,18 @@ func TestJobApi(t *testing.T) {
}

// TODO(jingzhang36): include UploadPipelineVersion in integration test

func (s *JobApiTestSuite) TearDownSuite() {
if *runIntegrationTests {
if !*isDevMode {
s.cleanUp()
}
}
}

func (s *JobApiTestSuite) cleanUp() {
test.DeleteAllExperiments(s.experimentClient, s.T())
test.DeleteAllPipelines(s.pipelineClient, s.T())
test.DeleteAllJobs(s.jobClient, s.T())
test.DeleteAllRuns(s.runClient, s.T())
}
26 changes: 20 additions & 6 deletions backend/test/integration/pipeline_api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,14 @@ func (s *PipelineApiTest) SetupTest() {
return
}

err := test.WaitForReady(*namespace, *initializeTimeout)
if err != nil {
glog.Exitf("Failed to initialize test. Error: %s", err.Error())
if !*isDevMode {
err := test.WaitForReady(*namespace, *initializeTimeout)
if err != nil {
glog.Exitf("Failed to initialize test. Error: %s", err.Error())
}
}
clientConfig := test.GetClientConfig(*namespace)
var err error
s.pipelineUploadClient, err = api_server.NewPipelineUploadClient(clientConfig, false)
if err != nil {
glog.Exitf("Failed to get pipeline upload client. Error: %s", err.Error())
Expand All @@ -52,6 +55,8 @@ func (s *PipelineApiTest) SetupTest() {
if err != nil {
glog.Exitf("Failed to get pipeline client. Error: %s", err.Error())
}

s.cleanUp()
}

func (s *PipelineApiTest) TestPipelineAPI() {
Expand Down Expand Up @@ -179,9 +184,6 @@ func (s *PipelineApiTest) TestPipelineAPI() {
var expectedWorkflow v1alpha1.Workflow
err = yaml.Unmarshal(expected, &expectedWorkflow)
assert.Equal(t, expectedWorkflow, *template)

/* ---------- Clean up ---------- */
test.DeleteAllPipelines(s.pipelineClient, t)
}

func verifyPipeline(t *testing.T, pipeline *model.APIPipeline) {
Expand Down Expand Up @@ -217,3 +219,15 @@ func TestPipelineAPI(t *testing.T) {
}

// TODO(jingzhang36): include UploadPipelineVersion in integration test

func (s *PipelineApiTest) TearDownSuite() {
if *runIntegrationTests {
if !*isDevMode {
s.cleanUp()
}
}
}

func (s *PipelineApiTest) cleanUp() {
test.DeleteAllPipelines(s.pipelineClient, s.T())
}
31 changes: 23 additions & 8 deletions backend/test/integration/run_api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,15 @@ func (s *RunApiTestSuite) SetupTest() {
return
}

err := test.WaitForReady(*namespace, *initializeTimeout)
if err != nil {
glog.Exitf("Failed to initialize test. Error: %s", err.Error())
if !*isDevMode {
err := test.WaitForReady(*namespace, *initializeTimeout)
if err != nil {
glog.Exitf("Failed to initialize test. Error: %s", err.Error())
}
}
s.namespace = *namespace
clientConfig := test.GetClientConfig(*namespace)
var err error
s.experimentClient, err = api_server.NewExperimentClient(clientConfig, false)
if err != nil {
glog.Exitf("Failed to get pipeline upload client. Error: %s", err.Error())
Expand All @@ -58,6 +61,8 @@ func (s *RunApiTestSuite) SetupTest() {
if err != nil {
glog.Exitf("Failed to get run client. Error: %s", err.Error())
}

s.cleanUp()
}

func (s *RunApiTestSuite) TestRunApis() {
Expand Down Expand Up @@ -215,11 +220,6 @@ func (s *RunApiTestSuite) TestRunApis() {
longRunningRunDetail, _, err = s.runClient.Get(&runparams.GetRunParams{RunID: longRunningRunDetail.Run.ID})
assert.Nil(t, err)
s.checkTerminatedRunDetail(t, longRunningRunDetail, helloWorldExperiment.ID, helloWorldExperiment.Name, longRunningPipeline.ID)

/* ---------- Clean up ---------- */
test.DeleteAllExperiments(s.experimentClient, t)
test.DeleteAllPipelines(s.pipelineClient, t)
test.DeleteAllRuns(s.runClient, t)
}

func (s *RunApiTestSuite) checkTerminatedRunDetail(t *testing.T, runDetail *run_model.APIRunDetail, experimentId string, experimentName string, pipelineId string) {
Expand Down Expand Up @@ -314,3 +314,18 @@ func TestRunApi(t *testing.T) {
}

// TODO(jingzhang36): include UploadPipelineVersion in integration test

func (s *RunApiTestSuite) TearDownSuite() {
if *runIntegrationTests {
if !*isDevMode {
s.cleanUp()
}
}
}

func (s *RunApiTestSuite) cleanUp() {
/* ---------- Clean up ---------- */
test.DeleteAllExperiments(s.experimentClient, s.T())
test.DeleteAllPipelines(s.pipelineClient, s.T())
test.DeleteAllRuns(s.runClient, s.T())
}
25 changes: 25 additions & 0 deletions backend/test/integration/run_tests_locally.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
#!/bin/bash

set -e

if [ -z "${NAMESPACE}" ]; then
echo "NAMESPACE env var is not provided, please set it to your KFP namespace"
exit
fi

echo "The api integration tests run against the cluster your kubectl communicates to.";
echo "It's currently '$(kubectl config current-context)'."
echo "WARNING: this will clear up all existing KFP data in this cluster."
read -r -p "Are you sure? [y/N] " response
case "$response" in
[yY][eE][sS]|[yY])
;;
*)
exit
;;
esac

echo "Starting integration tests..."
command="go test -v ./... -namespace ${NAMESPACE} -args -runIntegrationTests=true -isDevMode=true"
echo $command "$@"
$command "$@"
13 changes: 8 additions & 5 deletions backend/test/integration/visualization_api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (

type VisualizationApiTest struct {
suite.Suite
namespace string
namespace string
visualizationClient *api_server.VisualizationClient
}

Expand All @@ -25,12 +25,15 @@ func (s *VisualizationApiTest) SetupTest() {
return
}

err := test.WaitForReady(*namespace, *initializeTimeout)
if err != nil {
glog.Exitf("Failed to initialize test. Error: %v", err)
if !*isDevMode {
err := test.WaitForReady(*namespace, *initializeTimeout)
if err != nil {
glog.Exitf("Failed to initialize test. Error: %v", err)
}
}
s.namespace = *namespace
clientConfig := test.GetClientConfig(*namespace)
var err error
s.visualizationClient, err = api_server.NewVisualizationClient(clientConfig, false)
if err != nil {
glog.Exitf("Failed to get experiment client. Error: %v", err)
Expand All @@ -43,7 +46,7 @@ func (s *VisualizationApiTest) TestVisualizationAPI() {
/* ---------- Generate custom visualization --------- */
visualization := &visualization_model.APIVisualization{
Arguments: `{"code": ["print(2)"]}`,
Type: visualization_model.APIVisualizationTypeCUSTOM,
Type: visualization_model.APIVisualizationTypeCUSTOM,
}
customVisualization, err := s.visualizationClient.Create(&params.CreateVisualizationParams{
Body: visualization,
Expand Down