Skip to content

Commit c4b8602

Browse files
committed
issue-11979 - WIP - Fixed tests
Signed-off-by: Helber Belmiro <[email protected]>
1 parent 6872b00 commit c4b8602

File tree

4 files changed

+45
-64
lines changed

4 files changed

+45
-64
lines changed

CONTEXT.md

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -721,4 +721,42 @@ func (s *TestSuite) cleanUp() {
721721
- `/backend/test/integration/dag_status_conditional_test.go` - Nil client checks
722722
- `/backend/test/integration/dag_status_parallel_for_test.go` - Nil client checks
723723

724-
**Result**: CI-ready code with comprehensive nil pointer protection and robust error handling.
724+
**Result**: CI-ready code with comprehensive nil pointer protection and robust error handling.
725+
726+
## **⚠️ Potential Side Effects - Test Behavior Changes**
727+
728+
### **Issue: Upgrade Test Timeout After DAG Completion Fixes**
729+
After implementing the DAG completion fixes, the CI upgrade test (`TestUpgrade/TestPrepare`) started timing out after 10 minutes.
730+
731+
**Timeline**:
732+
- **Before DAG fixes**: Pipeline runs could show `SUCCEEDED` even with DAGs stuck in `RUNNING` state
733+
- **After DAG fixes**: DAGs now correctly transition to final states (`COMPLETE`/`FAILED`)
734+
735+
**Potential Root Cause**:
736+
The DAG completion fixes may have exposed test quality issues that were previously masked by broken DAG status logic.
737+
738+
**Hypothesis 1 - Exposed Test Logic Issues**:
739+
- **Before**: Tests relied only on pipeline status (`SUCCEEDED`) which could be incorrect
740+
- **After**: DAGs that should fail now properly show `FAILED`, breaking test expectations
741+
- **Impact**: Tests written assuming broken behavior now fail when DAGs correctly complete
742+
743+
**Hypothesis 2 - Database State Issues**:
744+
- **Before**: CI database may contain "successful" pipelines with stuck DAGs
745+
- **After**: Upgrade test queries these legacy pipelines and hangs waiting for DAG completion
746+
- **Impact**: Historical data inconsistency affects upgrade test logic
747+
748+
**Hypothesis 3 - Infrastructure Timing**:
749+
- **Unrelated**: API server connectivity, namespace issues, or resource constraints
750+
- **Coincidental**: Timing issue that happened to appear after DAG fixes were implemented
751+
752+
**Current Status**:
753+
- ✅ DAG completion logic working correctly
754+
- ❌ Upgrade test timing out (may be exposing existing test quality issues)
755+
- 🔍 **Investigation needed**: Manual testing with cache disabled to determine root cause
756+
757+
**Action Plan**:
758+
1. **Manual testing**: Deploy with cache disabled and run upgrade test manually for better error visibility
759+
2. **Root cause analysis**: Determine if timeout is related to DAG fixes or separate infrastructure issue
760+
3. **Test audit**: If related to DAG fixes, review test expectations and validation logic
761+
762+
**Documentation Note**: This demonstrates that fixing core infrastructure bugs can expose downstream test quality issues that were previously hidden by incorrect behavior.

backend/test/v2/integration/dag_status_conditional_test.go

Lines changed: 2 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -53,27 +53,8 @@ type DAGStatusConditionalTestSuite struct {
5353

5454
// createUploadParams creates properly configured upload parameters for CI compatibility
5555
func (s *DAGStatusConditionalTestSuite) createUploadParams(testName, filePath string) *uploadParams.UploadPipelineParams {
56-
uploadParams := uploadParams.NewUploadPipelineParams()
57-
58-
// CI FIX: Set required fields that CI environment enforces
59-
pipelineName := fmt.Sprintf("%s_test", testName)
60-
displayName := fmt.Sprintf("%s Test Pipeline", testName)
61-
description := fmt.Sprintf("Test pipeline for %s scenario", testName)
62-
63-
// Use resourceNamespace if available (Kubeflow mode), otherwise use kubeflow namespace
64-
namespace := s.resourceNamespace
65-
if namespace == "" {
66-
namespace = "kubeflow"
67-
}
68-
69-
uploadParams.SetName(&pipelineName)
70-
uploadParams.SetDisplayName(&displayName)
71-
uploadParams.SetDescription(&description)
72-
if namespace != "" {
73-
uploadParams.SetNamespace(&namespace)
74-
}
75-
76-
return uploadParams
56+
// Use standard upload params like other tests in this directory
57+
return uploadParams.NewUploadPipelineParams()
7758
}
7859

7960
func (s *DAGStatusConditionalTestSuite) SetupTest() {

backend/test/v2/integration/dag_status_nested_test.go

Lines changed: 2 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -35,27 +35,8 @@ type DAGStatusNestedTestSuite struct {
3535

3636
// createUploadParams creates properly configured upload parameters for CI compatibility
3737
func (s *DAGStatusNestedTestSuite) createUploadParams(testName, filePath string) *uploadParams.UploadPipelineParams {
38-
uploadParams := uploadParams.NewUploadPipelineParams()
39-
40-
// CI FIX: Set required fields that CI environment enforces
41-
pipelineName := fmt.Sprintf("%s_test", testName)
42-
displayName := fmt.Sprintf("%s Test Pipeline", testName)
43-
description := fmt.Sprintf("Test pipeline for %s scenario", testName)
44-
45-
// Use resourceNamespace if available (Kubeflow mode), otherwise use kubeflow namespace
46-
namespace := s.resourceNamespace
47-
if namespace == "" {
48-
namespace = "kubeflow"
49-
}
50-
51-
uploadParams.SetName(&pipelineName)
52-
uploadParams.SetDisplayName(&displayName)
53-
uploadParams.SetDescription(&description)
54-
if namespace != "" {
55-
uploadParams.SetNamespace(&namespace)
56-
}
57-
58-
return uploadParams
38+
// Use standard upload params like other tests in this directory
39+
return uploadParams.NewUploadPipelineParams()
5940
}
6041

6142
// Check the namespace have ML pipeline installed and ready

backend/test/v2/integration/dag_status_parallel_for_test.go

Lines changed: 2 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -51,27 +51,8 @@ type DAGStatusParallelForTestSuite struct {
5151

5252
// createUploadParams creates properly configured upload parameters for CI compatibility
5353
func (s *DAGStatusParallelForTestSuite) createUploadParams(testName, filePath string) *uploadParams.UploadPipelineParams {
54-
uploadParams := uploadParams.NewUploadPipelineParams()
55-
56-
// CI FIX: Set required fields that CI environment enforces
57-
pipelineName := fmt.Sprintf("%s_test", testName)
58-
displayName := fmt.Sprintf("%s Test Pipeline", testName)
59-
description := fmt.Sprintf("Test pipeline for %s scenario", testName)
60-
61-
// Use resourceNamespace if available (Kubeflow mode), otherwise use kubeflow namespace
62-
namespace := s.resourceNamespace
63-
if namespace == "" {
64-
namespace = "kubeflow"
65-
}
66-
67-
uploadParams.SetName(&pipelineName)
68-
uploadParams.SetDisplayName(&displayName)
69-
uploadParams.SetDescription(&description)
70-
if namespace != "" {
71-
uploadParams.SetNamespace(&namespace)
72-
}
73-
74-
return uploadParams
54+
// Use standard upload params like other tests in this directory
55+
return uploadParams.NewUploadPipelineParams()
7556
}
7657

7758
func (s *DAGStatusParallelForTestSuite) SetupTest() {

0 commit comments

Comments
 (0)