feat: add comprehensive e2e tests for ApplicationSet progressive sync #25081
Conversation
- Implement 5 e2e test functions covering progressive sync scenarios - Add ApplicationSetHasProgressiveStatus() for wave completion validation - Add OnlyApplicationsExist() for sequential order verification - Enable progressive sync feature via ConfigMap in test init() - Test 2-step, 3-step, and percentage-based maxUpdate constraints - Test deletion strategies (AllAtOnce and Reverse order) - Use Ginkgo BDD pattern with Given/When/Then fixtures Fixes argoproj#25042 Signed-off-by: AvhiMaz <[email protected]>
🔴 Preview Environment stopped on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
- Handle error return value from SetParamInSettingConfigMap (errcheck) - Fix import ordering for gofumpt compliance Signed-off-by: AvhiMaz <[email protected]>
Signed-off-by: AvhiMaz <[email protected]>
…ssive sync e2e tests Signed-off-by: AvhiMaz <[email protected]>
Signed-off-by: AvhiMaz <[email protected]>
Signed-off-by: AvhiMaz <[email protected]>
| var appDependencyList [][]string | ||
| var appStepMap map[string]int |
There was a problem hiding this comment.
| var appDependencyList [][]string | |
| var appStepMap map[string]int | |
| var ( | |
| appDependencyList [][]string | |
| appStepMap map[string]int | |
| ) |
There was a problem hiding this comment.
better to put the appSyncMap too.
| } | ||
|
|
||
| func (r *ApplicationSetReconciler) performProgressiveSyncs(ctx context.Context, logCtx *log.Entry, appset argov1alpha1.ApplicationSet, applications []argov1alpha1.Application, desiredApplications []argov1alpha1.Application) (map[string]bool, error) { | ||
| func (r *ApplicationSetReconciler) performProgressiveSyncs(ctx context.Context, logCtx *log.Entry, appset argov1alpha1.ApplicationSet, applications []argov1alpha1.Application, desiredApplications []argov1alpha1.Application) (map[string]bool, [][]string, map[string]int, error) { |
There was a problem hiding this comment.
can we add a method comment that also states why are we returning specific values like slice of slice of strings etc?
| _, err := r.updateApplicationSetApplicationStatus(ctx, logCtx, &appset, applications, appStepMap) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to update applicationset app status: %w", err) | ||
| return nil, nil, nil, fmt.Errorf("failed to update applicationset app status: %w", err) |
There was a problem hiding this comment.
i understand why you returned nil here because there's probably no use of appDepList or appStepMap. However, it's better to return those values since we may need them in future. Returning nil simply means either they haven't been built. I'm of mixed opinions here. @ranakan19 wdyt?
There was a problem hiding this comment.
yeah makes sense, i’m fine keeping them returned for now in case they’re needed later. we can clean it up once we’re sure they aren’t used anywhere else.
|
|
||
| // filterApplicationsByProgressiveStep filters applications to only include those in the current eligible step. | ||
| // Applications are only created/updated if all applications in their current step's prerequisite steps are healthy. | ||
| func (r *ApplicationSetReconciler) filterApplicationsByProgressiveStep(logCtx *log.Entry, appset *argov1alpha1.ApplicationSet, applications []argov1alpha1.Application, appDependencyList [][]string, appStepMap map[string]int) []argov1alpha1.Application { |
There was a problem hiding this comment.
please write unit tests for this function.
There was a problem hiding this comment.
Thank you for raising the PR. I don't think the e2e tests have been written so far, do you intend to put this in draft state? Secondly, i would like to hear from @ranakan19 thoughts on this PR especially on the implementation of filterApplicationsByProgressiveStep() and if we actually need it. If everything goes well, please consider writing unit tests too. Thanks!
- Consolidate variable declarations - Add method documentation - Fix error return values - Add unit tests for filterApplicationsByProgressiveStep Signed-off-by: AvhiMaz <[email protected]>
2 participants
Description
This PR adds comprehensive end-to-end tests for the ApplicationSet progressive sync feature, addressing issue #25042.
The implementation includes:
Related Issues
Fixes #25042