fix(unit): stabilize backend unit tests#13525
Conversation
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates the v2 metadata fake client and related launcher tests to make RecordArtifact failures deterministic per output name (avoiding flaky behavior due to map iteration order).
Changes:
- Extend
RecordArtifactFailureFakeClientto track call counts peroutputNameand optionally fail only for a specific output. - Add a new constructor to configure failures for a single output name.
- Update
Test_executeV2_publishLogsassertions to validate retries specifically forexecutor-logs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| backend/src/v2/metadata/client_fake.go | Adds per-output failure configuration and per-output call counting in the fake metadata client. |
| backend/src/v2/component/launcher_v2_test.go | Updates tests to use output-specific failure fake and assert retry counts deterministically. |
| func (c *RecordArtifactFailureFakeClient) RecordArtifact(ctx context.Context, outputName, schema string, runtimeArtifact *pipelinespec.RuntimeArtifact, state pb.Artifact_State, bucketConfig *objectstore.Config) (*OutputArtifact, error) { | ||
| c.RecordArtifactCalls++ | ||
| c.OutputNameCalls[outputName]++ |
There was a problem hiding this comment.
NewRecordArtifactFailureFakeClientForOutput is the sole constructor that always initializes OutputNameCalls
| // and succeed the second time, to test retry behavior without depending on map iteration order. | ||
| if test.uploadFailure { | ||
| countingFakeMetadataClient = metadata.NewRecordArtifactFailureFakeClient(1) | ||
| countingFakeMetadataClient = metadata.NewRecordArtifactFailureFakeClientForOutput("executor-logs", 1) |
1ca9758 to
87dba7f
Compare
| FailOutputName string | ||
| OutputNameCalls map[string]int |
87dba7f to
ad0a059
Compare
…now targets the `executor-logs` artifact explicitly, avoiding random failures when artifacts are processed in a different map order. Signed-off-by: arpechenin <arpechenin@avito.ru>
ad0a059 to
e311e0f
Compare
|
/retest |
The
Test_executeV2_publishLogs/retry_required_-_component_successcase had an implicit dependency on artifact map iteration order. Whenexecutor-logswas not processed in the expected order, the retry path could miss uploading logs and fail randomly with: blob (key "executor-logs-0") (code=NotFound): blob not foundThe fake metadata client now fails the
executor-logsartifact explicitly, so the test no longer depends on which artifact is processed first.failed tests:
https://github.com/kubeflow/pipelines/actions/runs/27442866956/job/81120877056?pr=13171