Add integration tests for API servers #112
Conversation
backend/test/experiment_api_test.go
Outdated
| if err != nil { | ||
| glog.Exitf("Failed to get RPC connection. Error: %s", err.Error()) | ||
| } | ||
| s.experimentClient = api.NewExperimentServiceClient(s.conn) |
There was a problem hiding this comment.
Could we please use the wrapper in https://github.com/kubeflow/pipelines/tree/master/backend/src/common/client/api_server?
(Here and in other places)
There was a problem hiding this comment.
Would it make more sense to test using the raw client? There might be retries and error handling in the wrapper that I would be hesitate to use when the goal is to test the plain API.
There was a problem hiding this comment.
The goal of the wrapper is to be used by anything that needs to make the call in GO. Let's please switch to using it so that we don't duplicate work. For instance, be adding retries/timeout in the wrapper, any user of the wrapper immediately benefits from it.
If the wrapper does things that are problematic for the API e2e test, let's modify the wrapper accordingly.
There was a problem hiding this comment.
The goal of the wrapper is to be used by anything that needs to make the call in GO. Let's please switch to using it so that we don't duplicate work. For instance, be adding retries/timeout in the wrapper, any user of the wrapper immediately benefits from it.
If the wrapper does things that are problematic for the API e2e test, let's modify the wrapper accordingly.
There was a problem hiding this comment.
I can use it once it's ready. Right now it's missing quite a few clients and it's using HTTP which is not what being used by tests. Let;s add a todo and prioritize that work. The test itself is ready and really high priority.
backend/test/pipeline_api_test.go
Outdated
| requestStartTime := time.Now().Unix() | ||
| /* ---------- Upload pipelines YAML ---------- */ | ||
| pipelineBody, writer := uploadPipelineFileOrFail("resources/arguments-parameters.yaml") | ||
| response, err := clientSet.RESTClient().Post(). |
There was a problem hiding this comment.
Could we please use the wrapper in https://github.com/kubeflow/pipelines/tree/master/backend/src/common/client/api_server instead of making raw REST calls?
Could we also use these wrappers in any place were the the API is called?
* add tensorboard routing rule * rename tb routing rule * address comments * remove debugger * fix url prefix
…nto yangpa/test
IronPan
changed the title
| argParamsRun := listRunsResponse.Runs[0] | ||
| s.checkArgParamsRun(t, argParamsRun, argParamsExperiment.Id, argParamsJob.Id, requestStartTime) | ||
|
|
||
| /* ---------- Clean up ---------- */ |
There was a problem hiding this comment.
Could we please turn this into a utility that anyone can reuse? Otherwise all the e2e tests have to do their own clean up in the current setup.
Additionally, since these tests are not concurrent, let's use the list calls to list and then delete all the resources.
backend/test/experiment_api_test.go
Outdated
| if err != nil { | ||
| glog.Exitf("Failed to get RPC connection. Error: %s", err.Error()) | ||
| } | ||
| s.experimentClient = api.NewExperimentServiceClient(s.conn) |
There was a problem hiding this comment.
The goal of the wrapper is to be used by anything that needs to make the call in GO. Let's please switch to using it so that we don't duplicate work. For instance, be adding retries/timeout in the wrapper, any user of the wrapper immediately benefits from it.
If the wrapper does things that are problematic for the API e2e test, let's modify the wrapper accordingly.
|
fix #112 |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vicaire The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vicaire The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test presubmit-unit-test |
|
/test build-image |
2 similar comments
|
/test build-image |
|
/test build-image |
|
/test presubmit-e2e-test |
1 similar comment
|
/test presubmit-e2e-test |
* Add kfserving mnist S3 saved model example * Add service account yaml * Wire up service account
* organizing main readme * organizing main readme * 2nd round of changes * edit service account and rbac page
Turn of auto image tagging for dspo.
This change is