test(backend): Add integration tests for artifact reading and large artifacts handling #12445
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
hbelmiro
force-pushed
the
test-artifact-download
branch
from
b66ed61 to
6d2a857
Compare
…rtifacts handling Signed-off-by: Helber Belmiro <[email protected]>
hbelmiro
force-pushed
the
test-artifact-download
branch
from
8bbd54f to
c5e5bdd
Compare
hbelmiro
changed the title
|
@HumairAK we can use this test to ensure that Open /assign @HumairAK |
| func (s *ArtifactAPITest) extractWorkflowNodeID(runID string) string { | ||
| t := s.T() | ||
|
|
||
| resp, err := http.Get(fmt.Sprintf("http://localhost:8888/apis/v1beta1/runs/%s", runID)) |
There was a problem hiding this comment.
can you re-use the api server host info (as well as the scheme http/https) from backend/test/config/flags.go instead of hardcoding it? This applies to everywhere, this applies to any place you are hardcoding localhost:8888
| t := s.T() | ||
|
|
||
| resp, err := http.Get(fmt.Sprintf("http://localhost:8888/apis/v1beta1/runs/%s", runID)) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
handle error (same with other resp.Body.Close() calls)
| s.deleteAllPipelineVersions(pipelineID) | ||
| }() | ||
|
|
||
| s.waitForRunCompletion(runID) |
There was a problem hiding this comment.
Sorry I should have been more clear, what I was suggesting we test is the endpoint allows us to continue testing that we can download artifacts - and not that we can download large files. My concern around download large files is that it can choke the CI and cause flaky behavior.
My preference is to just write a simple pipeline that will output an artifact (using @component instead of ContainerOp). The artifact shouldn't be large, we just need to confirm we can continue to download the artifact via this endpoint, and then verify the data is what we expect. This helps us ensure your PR doesn't break the current api behavior.
Verifying large downloads in master doesn't help us verify the issue once it's fixed, since to do that you would need to emulate the DOS for api server, and this is more of a perf test for which we don't have CI infrastructure for. So I think we should just rely on the fact blob handles this gracefully since that's in its spec, and don't test large file downloads for this endpoint.
Also, you can make this part of the original PR to keep things simple.
|
closed in favor of : #12394 |
2 participants
Description of your changes:
This PR adds a new integration test that uploads and runs a pipeline which generates a large binary artifact, then exercises the artifact read endpoint to ensure it correctly returns the artifact data. The test validates both the response metadata (HTTP headers, status codes) and payload format (JSON with base64‑encoded, gzip‑compressed data) to catch regressions in large artifact handling.
Checklist: