test(e2e): add e2e tests for reverse deletionOrder when progressive sync enabled by ranakan19 · Pull Request #26673 · argoproj/argo-cd

ranakan19 · 2026-03-03T15:45:11Z

Changes introduced:

makes generateExpectedApp more configurable by including finalizers as a parameter to add on applications
makes action Delete configurable by introducing a boolean variable to control deleteOptions
merges test case TestProgressiveSyncMultipleAppsPerStep into TestProgressiveSyncMultipleAppsPerStepWithReverseDeletionOrder to check muliple apps per step, becoming healthy, and being deleted in reverse order.
tests when deletionOrder set to reverse, applications are deleted in the reverse order of steps defined in rolling sync. controls with removing finalizer after application in that step is confirmed to have deletionTimeStamp.

local runtime with test-e2e-local:

PASS command-line-arguments.TestApplicationSetProgressiveSyncStep (4.08s)
PASS command-line-arguments.TestProgressiveSyncHealthGating (41.36s)
PASS command-line-arguments.TestNoApplicationStatusWhenNoSteps (1.00s)
PASS command-line-arguments.TestNoApplicationStatusWhenNoApplications (2.60s)
PASS command-line-arguments.TestProgressiveSyncMultipleAppsPerStepWithReverseDeletionOrder (10.85s)
coverage: [no statements]
PASS command-line-arguments

Checklist:

Signed-off-by: Kanika Rana <krana@redhat.com>

…tep test case with reverse deletionOrder Signed-off-by: Kanika Rana <krana@redhat.com>

bunnyshell · 2026-03-03T15:45:18Z

❗ Preview Environment deployment failed on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

🚀 /bns:deploy to redeploy the environment
❌ /bns:delete to remove the environment

test/e2e/applicationset_progressive_sync_test.go

Signed-off-by: Kanika Rana <krana@redhat.com>

codecov · 2026-03-03T16:34:22Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (master@d11c99a). Learn more about missing BASE report.

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #26673   +/-   ##
=========================================
  Coverage          ?   62.79%           
=========================================
  Files             ?      414           
  Lines             ?    55952           
  Branches          ?        0           
=========================================
  Hits              ?    35137           
  Misses            ?    17463           
  Partials          ?     3352

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Kanika Rana <krana@redhat.com>

…o deletionOrderTests

Signed-off-by: Kanika Rana <krana@redhat.com>

Copilot

Pull request overview

This PR adds e2e tests that verify the reverse deletion order behavior for ApplicationSets with progressive sync enabled. It also makes the test infrastructure more flexible by parameterizing the Delete action (with or without foreground propagation) and generateExpectedApp (with optional custom finalizers). Additionally, it merges the former TestProgressiveSyncMultipleAppsPerStep into a new test TestProgressiveSyncMultipleAppsPerStepWithReverseDeletionOrder that verifies apps are deleted in the reverse step order (prod → staging → dev), gated by a custom e2e finalizer.

Changes:

Delete() action signature changed to Delete(bool), where false uses foreground propagation (the original behavior for most tests) and true omits the propagation policy (needed for reverse deletion order test).
generateExpectedApp gains a testFinalizer string parameter to support attaching extra finalizers to apps under test.
New test TestProgressiveSyncMultipleAppsPerStepWithReverseDeletionOrder verifies the step-by-step reverse deletion order, controlled by a custom e2e finalizer that gates each wave.
New expectation helpers ApplicationsBeingDeletedOrGone and ApplicationsExistAndNotBeingDeleted, plus new action RemoveFinalizerFromApps.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`test/e2e/fixture/applicationsets/actions.go`	`Delete()` → `Delete(bool)` with conditional `DeleteOptions`; new `RemoveFinalizerFromApps` action
`test/e2e/fixture/applicationsets/expectation.go`	New `ApplicationsBeingDeletedOrGone` and `ApplicationsExistAndNotBeingDeleted` expectations
`test/e2e/applicationset_progressive_sync_test.go`	Replaced `TestProgressiveSyncMultipleAppsPerStep` with new reverse-deletion-order test; updated all `Delete()` → `Delete(false/true)` and `generateExpectedApp` calls
`test/e2e/applicationset_test.go`	Updated all `Delete()` → `Delete(false)`
`test/e2e/applicationset_git_generator_test.go`	Updated all `Delete()` → `Delete(false)`
`test/e2e/cluster_generator_test.go`	Updated all `Delete()` → `Delete(false)`
`test/e2e/clusterdecisiongenerator_e2e_test.go`	Updated all `Delete()` → `Delete(false)`
`test/e2e/matrix_e2e_test.go`	Updated all `Delete()` → `Delete(false)`
`test/e2e/merge_e2e_test.go`	Updated all `Delete()` → `Delete(false)`

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-03T18:25:22Z

test/e2e/fixture/applicationsets/expectation.go

+func ApplicationsBeingDeletedOrGone(appNames []string) Expectation {
+	anyapp := false
+	return func(c *Consequences) (state, string) {
+		for _, appName := range appNames {
+			app := c.app(appName)
+			if (app != nil && app.DeletionTimestamp != nil) || app == nil {
+				anyapp = true
+			}
+		}
+		if !anyapp {
+			return pending, "no app in this step is being deleted yet"
+		}
+		return succeeded, fmt.Sprintf("all apps %v are being deleted or gone", appNames)
+	}


The anyapp variable in ApplicationsBeingDeletedOrGone is declared outside the returned closure, meaning it persists across all calls to the closure (as ExpectWithDuration/Expect repeatedly invokes the closure in a retry loop). Once anyapp is set to true in any iteration, it can never revert to false in subsequent calls. This means that once at least one app is seen as "being deleted or gone" in any retry iteration, all future invocations of this expectation function will always return succeeded, regardless of the current actual state.

While in this specific deletion test the condition is likely monotonically true, this design is a latent correctness bug. The variable should be declared inside the returned closure so that each invocation starts fresh and accurately reflects the current state.

Copilot · 2026-03-03T18:25:22Z

test/e2e/fixture/applicationsets/actions.go


 // Delete deletes the ApplicationSet within the context
-func (a *Actions) Delete() *Actions {
+func (a *Actions) Delete(deletionOrder bool) *Actions {


The parameter name deletionOrder bool in the Delete function is semantically misleading. When the parameter is true, empty DeleteOptions are passed (no propagation policy), and when false, foreground deletion is used. The parameter name doesn't describe what it does - the actual meaning is something like "skipForegroundDeletion" or "useDefaultDeletion". The current name suggests it has something to do with the deletion order feature itself, but it only controls which DeleteOptions to use when issuing the API call. A clearer name such as foreground bool (where true = foreground propagation, false = default) or splitting into a named options struct would make call sites much easier to understand.

Copilot · 2026-03-03T18:25:23Z

test/e2e/fixture/applicationsets/actions.go

+	for _, appName := range appNames {
+		app, err := fixtureClient.AppClientset.ArgoprojV1alpha1().Applications(fixture.TestNamespace()).Get(
+			context.Background(), appName, metav1.GetOptions{})
+		if err != nil {
+			a.lastError = err
+			continue
+		}
+
+		// Remove the finalizer
+		finalizers := []string{}
+		for _, f := range app.Finalizers {
+			if f != finalizer {
+				finalizers = append(finalizers, f)
+			}
+		}
+		app.Finalizers = finalizers
+
+		_, err = fixtureClient.AppClientset.ArgoprojV1alpha1().Applications(fixture.TestNamespace()).Update(
+			context.Background(), app, metav1.UpdateOptions{})
+		if err != nil {
+			a.lastError = err
+		}
+	}
+	a.describeAction = fmt.Sprintf("removing finalizer '%s' from apps %v", finalizer, appNames)
+	a.verifyAction()


In RemoveFinalizerFromApps, when Get fails on an app, a.lastError is set and the loop continues to the next app. However, if a subsequent Get succeeds, a.lastError is not updated or cleared on success — it retains the previous error. This means the final verifyAction() call may fail with a stale error from an earlier iteration even if all remaining apps were processed successfully. Additionally, if the Update call on the last app succeeds, the error from a Get failure on an earlier app will still cause verifyAction() to fail.

Consider collecting all errors (e.g. in a slice) and reporting them after the loop, or ensuring a.lastError is reset on each successful operation.

Copilot · 2026-03-03T18:25:23Z

test/e2e/applicationset_progressive_sync_test.go

+		RemoveFinalizerFromApps(stagingApps, testFinalizer).
+		Then().
+		And(func() {
+			t.Log("removed finalizer from staging apps, confirm prod apps deleted")


The log message at line 497 says "removed finalizer from staging apps, confirm prod apps deleted" but the finalizer has just been removed from staging apps — this should say "confirm staging apps deleted" (not "prod apps deleted"). The same copy-paste issue exists in the log message at line 483, which says "confirm prod apps deleted" right after removing the finalizer from prod apps — that one is correct. Only line 497 is wrong.

Suggested change

t.Log("removed finalizer from staging apps, confirm prod apps deleted")

t.Log("removed finalizer from staging apps, confirm staging apps deleted")

Copilot · 2026-03-03T18:25:23Z

test/e2e/fixture/applicationsets/expectation.go

+		if !anyapp {
+			return pending, "no app in this step is being deleted yet"
+		}
+		return succeeded, fmt.Sprintf("all apps %v are being deleted or gone", appNames)


The success message in ApplicationsBeingDeletedOrGone says "all apps %v are being deleted or gone", but the function's documented behavior and actual logic only checks that at least one app is being deleted or gone — not all of them. This creates a misleading success message that incorrectly implies all apps satisfy the condition. The message should be something like "at least one app in %v is being deleted or gone".

Suggested change

return succeeded, fmt.Sprintf("all apps %v are being deleted or gone", appNames)

return succeeded, fmt.Sprintf("at least one app in %v is being deleted or gone", appNames)

ranakan19 added 3 commits February 3, 2026 20:33

parking changes

1ee01da

Signed-off-by: Kanika Rana <krana@redhat.com>

boilerplate testcase, assisted-by:cursor

b7a5428

Signed-off-by: Kanika Rana <krana@redhat.com>

fix test, change delete action to remove deleteOptions, merge multi-s…

6a37327

…tep test case with reverse deletionOrder Signed-off-by: Kanika Rana <krana@redhat.com>

ranakan19 requested a review from a team as a code owner March 3, 2026 15:45

ranakan19 commented Mar 3, 2026

View reviewed changes

test/e2e/applicationset_progressive_sync_test.go Outdated Show resolved Hide resolved

ranakan19 and others added 2 commits March 3, 2026 10:47

remove unused variable

2f3fa65

Signed-off-by: Kanika Rana <krana@redhat.com>

Merge branch 'master' into deletionOrderTests

eae2099

ranakan19 added 3 commits March 3, 2026 12:09

lint check

e596765

Signed-off-by: Kanika Rana <krana@redhat.com>

Merge branch 'deletionOrderTests' of github.com:ranakan19/argo-cd int…

2f28b2f

…o deletionOrderTests

remove fmt.sprint

0581c36

Signed-off-by: Kanika Rana <krana@redhat.com>

rumstead requested a review from Copilot March 3, 2026 18:20

Copilot started reviewing on behalf of rumstead March 3, 2026 18:20 View session

Copilot AI reviewed Mar 3, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(e2e): add e2e tests for reverse deletionOrder when progressive sync enabled#26673

test(e2e): add e2e tests for reverse deletionOrder when progressive sync enabled#26673
ranakan19 wants to merge 8 commits intoargoproj:masterfrom
ranakan19:deletionOrderTests

ranakan19 commented Mar 3, 2026 •

edited

Loading

Uh oh!

bunnyshell bot commented Mar 3, 2026 •

edited

Loading

Uh oh!

Uh oh!

codecov bot commented Mar 3, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	t.Log("removed finalizer from staging apps, confirm prod apps deleted")
	t.Log("removed finalizer from staging apps, confirm staging apps deleted")

	return succeeded, fmt.Sprintf("all apps %v are being deleted or gone", appNames)
	return succeeded, fmt.Sprintf("at least one app in %v is being deleted or gone", appNames)

Conversation

ranakan19 commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bunnyshell bot commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❗ Preview Environment deployment failed on Bunnyshell

Uh oh!

Uh oh!

codecov bot commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ranakan19 commented Mar 3, 2026 •

edited

Loading

bunnyshell bot commented Mar 3, 2026 •

edited

Loading

codecov bot commented Mar 3, 2026 •

edited

Loading