Ensure that spec.nonAdmin and spec.vmFileRestore enable/disable have basic tests

[weshayutin]

Why the changes were made

because the lord knows we NEED tests

How to test the changes made

make test-e2e TEST_FILTER="" GINKGO_ARGS="--focus='Should enable and disable VMFileRestore'"

[coderabbitai]

Walkthrough

Documentation example added for running E2E tests with TEST_FILTER unset. Two new boolean configuration fields (EnableVMFR and EnableNonAdmin) added to the test specification, with corresponding E2E tests for enabling and disabling these features in DPA deployments.

Changes

Cohort / File(s)	Change Summary
Documentation docs/developer/testing/TESTING.md	Added example command demonstrating E2E test execution with TEST_FILTER="" for DPA configuration; minor text reflow.
Test Specification and E2E Tests tests/e2e/dpa_deployment_suite_test.go	Added boolean fields EnableVMFR and EnableNonAdmin to TestDPASpec; extended createTestDPASpec to apply these flags to DPA spec; introduced two new E2E tests for enabling/disabling NonAdmin and VMFileRestore features.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Duplicated test logic: The two new E2E tests ("Should enable and disable NonAdmin" and "Should enable and disable VMFileRestore") appear to be inserted in two separate locations within the file with identical logic. Verify whether this duplication is intentional or an oversight.
• Flag application logic: Confirm that the logic in createTestDPASpec correctly applies EnableVMFR and EnableNonAdmin flags to the DPA spec structure.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

tests/e2e/dpa_deployment_suite_test.go (2)

389-410: NonAdmin enable/disable test could assert feature state, not just reconciliation

This test correctly exercises the enable→disable flow using CreateOrUpdate and IsReconciledTrue(), consistent with the rest of the suite. If you want it to catch regressions specific to the NonAdmin feature (not just general reconciliation), consider additionally reading back the DPA resource and asserting that .Spec.NonAdmin.Enable (or the presence/absence of NonAdmin-related behavior/resources) actually flips between enabled and disabled.

---

412-433: VMFileRestore enable/disable test mirrors NonAdmin; consider asserting VMFR-specific state

Similar to the NonAdmin test, this covers the VMFileRestore enable→disable transition and ensures the operator still reconciles successfully. To tighten coverage on the VMFR feature itself, consider also verifying that .Spec.VMFileRestore.Enable (or VMFR-related resources) reflect the enabled vs disabled state after each reconciliation, not just that IsReconciledTrue() returns true.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between f8b06e8 and 0dd5fca.

📒 Files selected for processing (2)

• docs/developer/testing/TESTING.md (1 hunks)
• tests/e2e/dpa_deployment_suite_test.go (3 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

• docs/developer/testing/TESTING.md
• tests/e2e/dpa_deployment_suite_test.go

🧬 Code graph analysis (1)

tests/e2e/dpa_deployment_suite_test.go (2)

api/v1alpha1/dataprotectionapplication_types.go (2)

• NonAdmin (695-733)
• VMFileRestore (736-745)

pkg/client/client.go (1)

• CreateOrUpdate (39-46)

🔇 Additional comments (2)

docs/developer/testing/TESTING.md (1)

103-107: VMFileRestore E2E invocation example is clear and useful

The added note and make test-e2e TEST_FILTER="" ... example make it much easier to run the VMFileRestore configuration test explicitly when TEST_FILTER would otherwise interfere. No issues from a docs/readability standpoint.

tests/e2e/dpa_deployment_suite_test.go (1)

21-38: NonAdmin/VMFileRestore flags are correctly mapped into the DPA spec

The new EnableNonAdmin and EnableVMFR booleans on TestDPASpec, and their use in createTestDPASpec to set NonAdmin.Enable and VMFileRestore.Enable via ptr.To(true), align with the API types and existing patterns (e.g., NodeAgent/Restic). Leaving these sub-structs nil when the flags are false is a reasonable way to represent the disabled state in the CR.

Also applies to: 134-143

[weshayutin]

/retest

[weshayutin]

/retest

[weshayutin]

/retest

[shubham-pampattiwar]

/retest

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kaovilai, shubham-pampattiwar, weshayutin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [kaovilai,shubham-pampattiwar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci]

@weshayutin: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/4.20-e2e-test-cli-aws	0dd5fca	link	true	/test 4.20-e2e-test-cli-aws

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.