Skip to content

ROX-29771: Refactor common nongroovy test code#17374

Merged
vikin91 merged 22 commits intomasterfrom
piotr/ROX-29771-refactor-nongroovy-common
Nov 19, 2025
Merged

ROX-29771: Refactor common nongroovy test code#17374
vikin91 merged 22 commits intomasterfrom
piotr/ROX-29771-refactor-nongroovy-common

Conversation

@vikin91
Copy link
Contributor

@vikin91 vikin91 commented Oct 20, 2025

Description

(Extracted from #17354, which was extracted from #17216)
🛑 Blocks #17354 not anymore

Refactor nongroovy Go e2e test infrastructure to improve robustness, maintainability, and debuggability.

Problems:

What has been changed in this PR:

  • Replace kubectl CLI calls with K8s API client for deployment creation
    • (Fix for TestDelegatedScanning): Call create deployment and delete pod using k8s API with retries. Before it was an cmd.exec with single attempt.
  • Add namespace parameter support to all deployment setup functions
    • This allows us to explicitly specify the namespace to know where look for the pod logs in the CI artifacts
  • Rename ambiguous function names for clarity (waitForDeploymentwaitForDeploymentInCentral)
    • Many assertions were focused on Central seeing something, despite the names not reflecting that
  • Add waitForDeploymentReadyInK8s to check deployment status before waiting in Central
    • Before asserting that Central received info about a deployment, we should make sure that the deployment was successfully created in k8s first. That check should run under a separate timeout.
  • Ensure that logging within test uses the t.Logf instead of log.Infof
    • That will guarantee linear time consistency of log output
  • Add extensive logging to deployment creation and waiting functions
    • If something flakes again, we must have as many details in the logs as possible.

User-facing documentation

  • CHANGELOG.md is updated OR update is not needed
  • documentation PR is created and is linked above OR is not needed

Testing and quality

  • the change is production ready: the change is GA, or otherwise the functionality is gated by a feature flag
  • CI results are inspected

Automated testing

  • added unit tests
  • added e2e tests
  • added regression tests
  • added compatibility tests
  • modified existing tests

Modified existing e2e tests to use the new refactored helper functions.

How I validated my change

  • I run ocp-4-18-nongroovy-e2e-tests and ocp-4-19-nongroovy-e2e-tests 10 times and made sure that none of the modified tests are failing. I focused on TestDelegatedScanning, TestPod and TestContainerInstances. (TestDelegatedScanning requires an OCP cluster).
  • I did the same for gke-nongroovy-e2e-tests but fewer times.

AI assistance:
~40% of code was AI-generated (boilerplate K8s API calls, logging statements, error handling patterns). All core logic, function signatures, architectural decisions, and test updates were designed and validated manually. AI-generated code was reviewed line-by-line and modified where needed for correctness and consistency with existing patterns.

@openshift-ci
Copy link

openshift-ci bot commented Oct 20, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@codecov
Copy link

codecov bot commented Oct 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 49.40%. Comparing base (1f1d627) to head (d4d8c77).
⚠️ Report is 12 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #17374   +/-   ##
=======================================
  Coverage   49.40%   49.40%           
=======================================
  Files        2685     2685           
  Lines      197716   197716           
=======================================
+ Hits        97686    97691    +5     
+ Misses      92401    92400    -1     
+ Partials     7629     7625    -4     
Flag Coverage Δ
go-unit-tests 49.40% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rhacs-bot
Copy link
Contributor

rhacs-bot commented Oct 21, 2025

Images are ready for the commit at 7bec0eb.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.10.x-109-g7bec0eb124.

@vikin91 vikin91 marked this pull request as ready for review October 22, 2025 07:31
@vikin91 vikin91 force-pushed the piotr/ROX-29771-refactor-nongroovy-common branch from d74dfdc to e52691d Compare October 22, 2025 11:09
@vikin91 vikin91 added the auto-retest PRs with this label will be automatically retested if prow checks fails label Oct 23, 2025
@rhacs-bot
Copy link
Contributor

/retest

2 similar comments
@rhacs-bot
Copy link
Contributor

/retest

@rhacs-bot
Copy link
Contributor

/retest

@rhacs-bot
Copy link
Contributor

rhacs-bot commented Oct 23, 2025

Images are ready for the commit at 5a3a9f6.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.10.x-153-g5a3a9f6a5c.

@vikin91 vikin91 force-pushed the piotr/ROX-29771-refactor-nongroovy-common branch from 7bec0eb to 5a3a9f6 Compare October 24, 2025 10:29
@rhacs-bot
Copy link
Contributor

/retest

@vikin91
Copy link
Contributor Author

vikin91 commented Oct 24, 2025

/retest-times 20 gke-nongroovy-e2e-tests

@rhacs-bot
Copy link
Contributor

/test gke-nongroovy-e2e-tests

12 similar comments
@rhacs-bot
Copy link
Contributor

/test gke-nongroovy-e2e-tests

@rhacs-bot
Copy link
Contributor

/test gke-nongroovy-e2e-tests

@rhacs-bot
Copy link
Contributor

/test gke-nongroovy-e2e-tests

@rhacs-bot
Copy link
Contributor

/test gke-nongroovy-e2e-tests

@rhacs-bot
Copy link
Contributor

/test gke-nongroovy-e2e-tests

@rhacs-bot
Copy link
Contributor

/test gke-nongroovy-e2e-tests

@rhacs-bot
Copy link
Contributor

/test gke-nongroovy-e2e-tests

@rhacs-bot
Copy link
Contributor

/test gke-nongroovy-e2e-tests

@rhacs-bot
Copy link
Contributor

/test gke-nongroovy-e2e-tests

@rhacs-bot
Copy link
Contributor

/test gke-nongroovy-e2e-tests

@rhacs-bot
Copy link
Contributor

/test gke-nongroovy-e2e-tests

@rhacs-bot
Copy link
Contributor

/test gke-nongroovy-e2e-tests

@openshift-ci
Copy link

openshift-ci bot commented Nov 11, 2025

@vikin91: The following tests 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/ocp-4-12-qa-e2e-tests 7bec0eb link false /test ocp-4-12-qa-e2e-tests
ci/prow/ocp-4-19-qa-e2e-tests 7bec0eb link false /test ocp-4-19-qa-e2e-tests

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.

@vikin91
Copy link
Contributor Author

vikin91 commented Nov 12, 2025

/retest

@vikin91 vikin91 added the pls-merge Indicates to the *reviewer* that the PR can be merged once tests pass and approval is granted label Nov 14, 2025
Copy link
Contributor

@lvalerom lvalerom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you for the improvements! 🙂

…edScopes

This commit addresses flakiness in e2e tests by improving deployment management
and test reliability:

1. **Refactored deployment creation** (common.go):
   - Replaced kubectl CLI calls with K8s API client for better control
   - Added extensive logging at each step of deployment lifecycle
   - Bubbled up namespace parameter (configurable, defaults to 'default')
   - Added waitForDeploymentReadyInK8s() to ensure deployments are ready in K8s
     before waiting for Central ingestion
   - Renamed functions for clarity:
     * waitForDeployment -> waitForDeploymentInCentral
     * waitForDeploymentCount -> waitForDeploymentCountInCentral

2. **Fixed TestExcludedScopes** (excluded_scopes_test.go):
   - Changed image from 'nginx' to 'quay.io/rhacs-eng/qa-multi-arch-nginx:latest'
   - This ensures the 'Latest tag' policy is triggered (required for test logic)
   - Updated to use new deployment functions with explicit namespace

3. **Updated related tests** for consistency:
   - backup_test.go: Use prefetched image
   - active_vuln_test.go: Use prefetched images from quay.io to avoid rate limiting
   - resourcecollection_test.go: Use renamed waitForDeploymentCountInCentral

All changes maintain backward compatibility while improving debuggability
and reducing flakiness in CI environments.

Partially AI-generated.

Resolves: ROX-29771
Replace undefined waitForDeployment with waitForDeploymentInCentral to fix
compilation error in excluded_scopes_test.go. Test now properly waits for
deployment to be registered in Central before proceeding.

User request: fix compilation issues in TestExcludedScopes test migrated to
use prefetcher approach.

Partially AI-assisted.
- Use t.Logf instead of log.Infof
- Add missing namespace args
- Readd removed comment
@vikin91 vikin91 force-pushed the piotr/ROX-29771-refactor-nongroovy-common branch from 90e73f3 to d4d8c77 Compare November 19, 2025 11:26
@vikin91 vikin91 merged commit 7faf849 into master Nov 19, 2025
88 checks passed
@vikin91 vikin91 deleted the piotr/ROX-29771-refactor-nongroovy-common branch November 19, 2025 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-retest PRs with this label will be automatically retested if prow checks fails pls-merge Indicates to the *reviewer* that the PR can be merged once tests pass and approval is granted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants