fix(test): prevent cleanup failures from failing passing tests#13261
fix(test): prevent cleanup failures from failing passing tests#13261hbelmiro wants to merge 4 commits intokubeflow:masterfrom
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
This PR adjusts backend integration test utilities and pipeline list API tests to reduce false failures from cleanup logic and to avoid false positives caused by leftover pipelines affecting list assertions.
Changes:
- Removes
gomega.Expectassertions fromDeletePipeline/DeleteAllPipelinesso cleanup errors don’t fail otherwise-passing tests. - Adds
DeleteAllPipelinesinBeforeEachfor Pagination and Sorting list-pipelines contexts to reduce flakiness from leftover pipelines.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| backend/test/v2/api/pipeline_api_test.go | Adds pre-test cleanup in pagination/sorting contexts to reduce leftover-resource interference. |
| backend/test/testutil/pipeline_utils.go | Makes pipeline cleanup best-effort by removing failing assertions during deletion. |
| logger.Log("Deleting all pipeline version of pipeline with id=%s", pipelineID) | ||
| DeleteAllPipelineVersions(client, pipelineID) | ||
| logger.Log("Deleting pipeline with id=%s", pipelineID) | ||
| err = client.Delete(&pipeline_params.PipelineServiceDeletePipelineParams{PipelineID: pipelineID}) |
There was a problem hiding this comment.
Why would the in here execute if the pipeline doesn't exist, and if does exist, then we should be able to delete it, if not, then there probably is an issue somewhere. I don't agree with the following log statement and exiting without hard failure
| } | ||
|
|
||
| /* DeleteAllPipelines deletes all pipelines */ | ||
| func DeleteAllPipelines(client *api_server.PipelineClient, namespace *string) { |
There was a problem hiding this comment.
I can't think of an exact scenario on top of my head, but I think the reason why we had the logic before was to prevent some issues with parallel execution with some tests creating and deleting pipeline within the test body vs AfterEach or AfterAll
Signed-off-by: Helber Belmiro <helber.belmiro@gmail.com>
…y pipeline list assertions Signed-off-by: Helber Belmiro <helber.belmiro@gmail.com>
…tion and sorting tests Signed-off-by: Helber Belmiro <helber.belmiro@gmail.com>
…n cleanup logic - Updated `DeletePipeline` to support cascade deletions by passing a `cascade` flag. - Refactored tests to use cascade deletion for improved cleanup consistency and reliability. Signed-off-by: Helber Belmiro <helber.belmiro@gmail.com>
Description of your changes:
Checklist: