Skip to content

[wip]NO-JIRA: Move test/e2e/accept.go to its own folder#2294

Open
hongkailiu wants to merge 3 commits into
openshift:mainfrom
hongkailiu:e2e-subfolder
Open

[wip]NO-JIRA: Move test/e2e/accept.go to its own folder#2294
hongkailiu wants to merge 3 commits into
openshift:mainfrom
hongkailiu:e2e-subfolder

Conversation

@hongkailiu

@hongkailiu hongkailiu commented Jun 29, 2026

Copy link
Copy Markdown
Member

The goal is to get additional approvers for the testing like we do for the core code.

Summary by CodeRabbit

  • Chores
    • Added review ownership coverage for the admin upgrade end-to-end test area to ensure the right approvers can review future changes.
    • Updated the upgrade acceptance test package and setup to use the shared end-to-end test helpers for consistent CLI initialization and test gating.
    • Exported the shared MicroShift-skipping helper and updated relevant end-to-end CLI tests to use the standard skip function.

```console
$ cp ./pkg/cli/admin/upgrade/OWNERS ./test/e2e/admin/upgrade/
```
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 29, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@hongkailiu: This pull request explicitly references no jira issue.

Details

In response to this:

The goal is to get additional approvers for the testing like we do for the core code.

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 29, 2026
@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown

Walkthrough

Adds an OWNERS file under test/e2e/admin/upgrade/, changes the accept test package to accept, exports the MicroShift skip helper, and updates CLI tests to use the exported helper.

Changes

Upgrade e2e ownership and helper updates

Layer / File(s) Summary
Ownership and accept setup
test/e2e/admin/upgrade/OWNERS, test/e2e/admin/upgrade/accept/accept.go
Adds the upgrade path OWNERS file with update-approvers, changes the accept test package name, and updates its setup and gating calls to use e2e-prefixed helpers.
Export MicroShift skip helper
test/e2e/util.go
Renames skipIfMicroShift to exported SkipIfMicroShift and keeps the same skip behavior.
Update skip call sites
test/e2e/cli.go
Replaces unqualified MicroShift skip helper calls with SkipIfMicroShift(oc) across the shown CLI e2e tests.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 15
✅ Passed checks (15 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the main change: relocating the accept test into its own folder and updating related references.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed Changed/added Ginkgo titles are static strings; no dynamic names or generated values were introduced in the touched test files.
Test Structure And Quality ✅ Passed PASS: The PR only relocates/renames test helpers and adds OWNERS; no new It blocks, waits, or resource lifecycle changes violate the stated test-quality rules.
Microshift Test Compatibility ✅ Passed The only touched e2e spec is already guarded by SkipIfMicroShift and TechPreviewNoUpgrade; other changes just export/use the skip helper.
Single Node Openshift (Sno) Test Compatibility ✅ Passed The new accept-risks Ginkgo test only uses cluster-version APIs and oc adm upgrade accept; it has no multi-node/HA assumptions and already skips MicroShift.
Topology-Aware Scheduling Compatibility ✅ Passed Only e2e test/helper renames and OWNERS changed; no manifests/controllers or topology-sensitive scheduling constraints were introduced.
Ote Binary Stdout Contract ✅ Passed No new stdout writes were added in process-level code; the touched files only use Ginkgo test blocks and helper logging, not main/init/TestMain/suite setup.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed The PR only relocates/renames existing e2e test code and helper exports; the accept-risks test uses cluster APIs/oc and has no IPv4-only or public-internet assumptions.
No-Weak-Crypto ✅ Passed Changed files only adjust test wiring and OWNERS; no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB, custom crypto, or secret comparisons were added.
Container-Privileges ✅ Passed No container/K8s manifests were changed; the PR only edits Go e2e tests and an OWNERS file.
No-Sensitive-Data-In-Logs ✅ Passed Diff only renames/exports helpers and updates references; no new log statements or sensitive-data output was introduced.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@openshift-ci openshift-ci Bot requested review from ardaguclu and tchap June 29, 2026 14:05
@hongkailiu

Copy link
Copy Markdown
Member Author

/test unit

@ardaguclu

Copy link
Copy Markdown
Member

I'm supportive of the idea
/approve

@openshift-ci

openshift-ci Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu, hongkailiu

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:

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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 29, 2026
@hongkailiu

Copy link
Copy Markdown
Member Author

/test unit

2 similar comments
@hongkailiu

Copy link
Copy Markdown
Member Author

/test unit

@hongkailiu

Copy link
Copy Markdown
Member Author

/test unit

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
test/e2e/admin/upgrade/accept/accept.go (1)

1-24: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Add a Ginkgo suite bootstrap for test/e2e/admin/upgrade/accept. This directory currently has only accept.go, and there’s no RunSpecs/*_suite_test.go or oc-tests-ext wiring that imports it, so these specs won’t be discovered by go test or the Ginkgo runner.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/admin/upgrade/accept/accept.go` around lines 1 - 24, Add a Ginkgo
suite bootstrap for the accept package so these specs are actually registered
and run. Create the missing suite entry point for test/e2e/admin/upgrade/accept
(the package containing accept.go) with the usual RunSpecs setup, and make sure
it is imported by the existing e2e test wiring such as oc-tests-ext so go test
and the Ginkgo runner discover the suite.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@test/e2e/admin/upgrade/accept/accept.go`:
- Around line 1-24: Add a Ginkgo suite bootstrap for the accept package so these
specs are actually registered and run. Create the missing suite entry point for
test/e2e/admin/upgrade/accept (the package containing accept.go) with the usual
RunSpecs setup, and make sure it is imported by the existing e2e test wiring
such as oc-tests-ext so go test and the Ginkgo runner discover the suite.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 20359540-14b3-4e34-b5f1-fd02d13ae497

📥 Commits

Reviewing files that changed from the base of the PR and between 05c178e and 86dfa4a.

📒 Files selected for processing (3)
  • test/e2e/admin/upgrade/accept/accept.go
  • test/e2e/cli.go
  • test/e2e/util.go

@hongkailiu

Copy link
Copy Markdown
Member Author

Took this run:

$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_oc/2294/pull-ci-openshift-oc-main-e2e-aws-ovn-serial-1of2/2072801388647157760/artifacts/e2e-aws-ovn-serial/openshift-e2e-test/artifacts/e2e.log | grep risks
started: 0/1/54 "[Jira:\"Cluster Version Operator\"] cluster-version-operator should work with accept risks"
skipped: (300ms) 2026-07-03T00:29:47 "[Jira:\"Cluster Version Operator\"] cluster-version-operator should work with accept risks"

Skipped as expected. Similar to #2170 (comment) .

/verified by @hongkailiu

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Jul 3, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@hongkailiu: This PR has been marked as verified by @hongkailiu.

Details

In response to this:

Took this run:

$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_oc/2294/pull-ci-openshift-oc-main-e2e-aws-ovn-serial-1of2/2072801388647157760/artifacts/e2e-aws-ovn-serial/openshift-e2e-test/artifacts/e2e.log | grep risks
started: 0/1/54 "[Jira:\"Cluster Version Operator\"] cluster-version-operator should work with accept risks"
skipped: (300ms) 2026-07-03T00:29:47 "[Jira:\"Cluster Version Operator\"] cluster-version-operator should work with accept risks"

Skipped as expected. Similar to #2170 (comment) .

/verified by @hongkailiu

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 openshift-eng/jira-lifecycle-plugin repository.

@hongkailiu

Copy link
Copy Markdown
Member Author

/retest-required

@openshift-ci

openshift-ci Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

@hongkailiu: 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/e2e-aws-ovn-upgrade 86dfa4a link true /test e2e-aws-ovn-upgrade
ci/prow/e2e-aws-ovn 86dfa4a link true /test e2e-aws-ovn

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants