-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Changes from all commits
7d18606
c6cd7c8
257af1c
6e0424c
58cf6b2
4be178d
4158e6c
4275bc4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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()) | ||
|
@@ -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() { | ||
|
@@ -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) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's correct. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just realized, frontend-integration-test actually fails after this. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
@@ -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()) | ||
} |
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 "$@" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://godoc.org/github.com/stretchr/testify/suite#TearDownAllSuite
it will be called by testify
There was a problem hiding this comment.
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