refactor(storage): migrate utilities.constants imports to submodules#5320
Open
vsibirsk wants to merge 1 commit into
Open
refactor(storage): migrate utilities.constants imports to submodules#5320vsibirsk wants to merge 1 commit into
vsibirsk wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughAll test files under ChangesConstants Import Refactor
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
openshift-virtualization-qe-bot-5
requested review from
Ahmad-Hafe,
RoniKishner,
acinko-rh,
akalenyu,
awels,
dsanatar,
josemacassan,
jpeimer,
kgoldbla,
kshvaika,
rlobillo and
rnetser
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Replace `from utilities.constants import X` with direct submodule imports across all 57 files in tests/storage/ and tests/data_protection/. `from utilities.constants import Images` is intentionally kept as-is — Images is computed dynamically in __init__.py until the circular import in utilities/architecture.py is resolved. Generated-by: Claude Sonnet 4.6 Signed-off-by: vsibirsk <vsibirsk@redhat.com>
vsibirsk
force-pushed
the
vk-migrate-storage-imports
branch
from
521a648 to
9d2b7e3
Compare
Collaborator
Author
|
/verified rebase to resolve merge conflicts |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
Contributor
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Output rules: Your deliverable: Test Execution Plan
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL — Verification rules for smoke and gating decisions: |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5320"
echo "== PR metadata =="
gh pr view "$pr" --repo "$repo" --json baseRefOid,headRefOid,files --jq '{
baseOid: .baseRefOid,
headOid: .headRefOid,
files: [.files[].path]
}'
base="$(gh pr view "$pr" --repo "$repo" --json baseRefOid --jq '.baseRefOid')"
head="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
echo
echo "== Changed files =="
gh pr diff "$pr" --repo "$repo" --name-only | sort
echo
echo "== First changed file and first added line =="
python - <<'PY'
import subprocess, re, json
repo="RedHatQE/openshift-virtualization-tests"
pr="5320"
files=subprocess.check_output(["gh","pr","diff",pr,"--repo",repo,"--name-only"], text=True).splitlines()
first=files[0]
diff=subprocess.check_output(["gh","pr","diff",pr,"--repo",repo,"--",first], text=True)
new_line=None
for line in diff.splitlines():
if line.startswith("@@"):
m=re.search(r"\+(\d+)(?:,(\d+))?", line)
new_line=int(m.group(1)) if m else None
continue
if new_line is None:
continue
if line.startswith("+") and not line.startswith("+++"):
print(json.dumps({"path": first, "line": new_line}, indent=2))
break
if not line.startswith("-"):
new_line += 1
PY
echo
echo "== Changed utilities/libs files =="
gh pr diff "$pr" --repo "$repo" --name-only | rg '^(utilities/|libs/)' || true
echo
echo "== Changed conftest files =="
gh pr diff "$pr" --repo "$repo" --name-only | rg '(^|/)conftest\.py$' || true
echo
echo "== AST outline of changed conftest files =="
while IFS= read -r f; do
[ -n "$f" ] || continue
ast-grep outline "$f" --view expanded || true
done < <(gh pr diff "$pr" --repo "$repo" --name-only | rg '(^|/)conftest\.py$' || true)
echo
echo "== Diff hunks for changed conftest files =="
while IFS= read -r f; do
[ -n "$f" ] || continue
echo "-- $f --"
git diff "$base...$head" -- "$f"
done < <(gh pr diff "$pr" --repo "$repo" --name-only | rg '(^|/)conftest\.py$' || true)
echo
echo "== utilities.constants import changes across PR =="
git diff "$base...$head" -- $(gh pr diff "$pr" --repo "$repo" --name-only) | rg -n '^[+-]from utilities\.constants' -C 1 || true
echo
echo "== Changed test files that directly have smoke/gating markers =="
while IFS= read -r f; do
[ -f "$f" ] || continue
rg -n '`@pytest`\.mark\.(smoke|gating)' "$f" || true
done < <(gh pr diff "$pr" --repo "$repo" --name-only | rg '(^|/)test_.*\.py$|(^|/).*_test\.py$' || true)
echo
echo "== All smoke-marked test files =="
rg -l '`@pytest`\.mark\.smoke' tests/ || true
echo
echo "== All gating-marked test files =="
rg -l '`@pytest`\.mark\.gating' tests/ || true
echo
echo "== Changed test files =="
gh pr diff "$pr" --repo "$repo" --name-only | rg '(^|/)test_.*\.py$|(^|/).*_test\.py$' || true
echo
echo "== Parent conftest coverage for smoke/gating files =="
python - <<'PY'
import subprocess, pathlib, json
repo="RedHatQE/openshift-virtualization-tests"
pr="5320"
changed=set(subprocess.check_output(["gh","pr","diff",pr,"--repo",repo,"--name-only"], text=True).splitlines())
changed_conftests={p for p in changed if p.endswith("conftest.py")}
smoke=set(filter(None, subprocess.check_output(["rg","-l",r'`@pytest`\.mark\.smoke',"tests/"], text=True).splitlines()))
gating=set(filter(None, subprocess.check_output(["rg","-l",r'`@pytest`\.mark\.gating',"tests/"], text=True).splitlines()))
def parents(path):
parts=path.split("/")[:-1]
out=[]
while parts:
out.append("/".join(parts+["conftest.py"]))
parts.pop()
return out
def hits(files):
rows=[]
for f in sorted(files):
direct = f in changed
scope_hits=[p for p in parents(f) if p in changed_conftests]
if direct or scope_hits:
rows.append({"file": f, "direct_changed": direct, "changed_parent_conftests": scope_hits})
return rows
print(json.dumps({"smoke_hits": hits(smoke), "gating_hits": hits(gating)}, indent=2))
PY
echo
echo "== Test outline for changed test files =="
python - <<'PY'
import ast, subprocess, pathlib, json
repo="RedHatQE/openshift-virtualization-tests"
pr="5320"
files=subprocess.check_output(["gh","pr","diff",pr,"--repo",repo,"--name-only"], text=True).splitlines()
for f in files:
p=pathlib.Path(f)
if not p.exists() or p.suffix != ".py" or not p.name.startswith("test_"):
continue
try:
tree=ast.parse(p.read_text())
except SyntaxError as e:
print(json.dumps({f: {"syntax_error": str(e)}}))
continue
items=[]
for node in tree.body:
if isinstance(node, ast.FunctionDef) and node.name.startswith("test_"):
items.append(node.name)
elif isinstance(node, ast.ClassDef) and node.name.startswith("Test"):
methods=[m.name for m in node.body if isinstance(m, ast.FunctionDef) and m.name.startswith("test_")]
if methods:
items.append({node.name: methods})
print(json.dumps({f: items}, ensure_ascii=False))
PYLength of output: 10936 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
owner="${repo%%/*}"
name="${repo#*/}"
pr="5320"
meta="$(gh pr view "$pr" --repo "$repo" --json baseRefOid,headRefOid,files)"
base="$(jq -r '.baseRefOid' <<<"$meta")"
head="$(jq -r '.headRefOid' <<<"$meta")"
mapfile -t files < <(jq -r '.files[].path' <<<"$meta")
first_file="${files[0]}"
python - "$owner" "$name" "$pr" "$base" "$head" "$first_file" "${files[@]}" <<'PY'
import ast
import json
import pathlib
import re
import subprocess
import sys
owner, name, pr, base, head, first_file, *files = sys.argv[1:]
changed_files = files
changed_set = set(changed_files)
changed_conftests = [f for f in changed_files if f.endswith("conftest.py")]
changed_conftest_set = set(changed_conftests)
# Find first added line on the first changed file using local git diff
diff = subprocess.check_output(["git", "diff", f"{base}...{head}", "--", first_file], text=True)
new_line = None
first_line = None
for line in diff.splitlines():
if line.startswith("@@"):
m = re.search(r"\+(\d+)(?:,(\d+))?", line)
new_line = int(m.group(1)) if m else None
continue
if new_line is None:
continue
if line.startswith("+") and not line.startswith("+++"):
first_line = new_line
break
if not line.startswith("-"):
new_line += 1
if first_line is None:
raise SystemExit("No added line found in first changed file")
def rg_files(pattern: str):
proc = subprocess.run(["rg", "-l", pattern, "tests/"], text=True, capture_output=True, check=False)
return set(filter(None, proc.stdout.splitlines()))
smoke_files = rg_files(r'`@pytest`\.mark\.smoke')
gating_files = rg_files(r'`@pytest`\.mark\.gating')
def parent_conftests(path: str):
parts = path.split("/")[:-1]
out = []
while parts:
out.append("/".join(parts + ["conftest.py"]))
parts.pop()
return out
smoke_hits = []
for sf in sorted(smoke_files):
if sf in changed_set:
smoke_hits.append(f"`{sf}` changed directly")
else:
hits = [c for c in parent_conftests(sf) if c in changed_conftest_set]
if hits:
smoke_hits.append(f"`{sf}` → `{hits[0]}`")
gating_hits = []
for gf in sorted(gating_files):
if gf in changed_set:
gating_hits.append(f"`{gf}` changed directly")
else:
hits = [c for c in parent_conftests(gf) if c in changed_conftest_set]
if hits:
gating_hits.append(f"`{gf}` → `{hits[0]}`")
# Build affected test list: changed test files + changed conftest directory scopes.
affected = []
for f in changed_files:
if f.endswith("conftest.py"):
scope = f.rsplit("/", 1)[0] + "/"
affected.append(f"`{scope}` (collection/setup scope via changed `{f}`)")
elif pathlib.Path(f).name.startswith("test_") and f.endswith(".py"):
tags = []
if f in smoke_files:
tags.append("smoke")
if f in gating_files:
tags.append("gating")
suffix = f" ({', '.join(tags)})" if tags else ""
affected.append(f"`{f}`{suffix}")
# Deduplicate preserving order
deduped = []
seen = set()
for item in affected:
if item not in seen:
deduped.append(item)
seen.add(item)
affected_text = "\n".join(f"- {item}" for item in deduped)
smoke_true = bool(smoke_hits)
gating_true = bool(gating_hits)
smoke_detail = (
"True — verified collection/setup path: "
+ "; ".join(smoke_hits[:6])
+ ("; ..." if len(smoke_hits) > 6 else "")
+ " → changed import path in the test module/conftest."
if smoke_true else
"False — smoke-marked files exist in `tests/`, but none were changed directly and none inherit a changed parent `conftest.py` scope in this PR."
)
gating_detail = (
"True — verified collection/setup path: "
+ "; ".join(gating_hits[:6])
+ ("; ..." if len(gating_hits) > 6 else "")
+ " → changed import path in the test module/conftest."
if gating_true else
"False — gating-marked files exist in `tests/`, but none were changed directly and none inherit a changed parent `conftest.py` scope in this PR."
)
body = f"""**Test Execution Plan**
- **Run smoke tests: {'True' if smoke_true else 'False'}** — {smoke_detail}
- **Run gating tests: {'True' if gating_true else 'False'}** — {gating_detail}
**Affected tests to run**
{affected_text}
**Real tests (cluster required)**
Error/fix path (import resolution at collection/setup time):
`uv run pytest -rA -s -o log_cli=true --tc-file=tests/global_config_amd64.py --tc-format=python --cluster-sanity-skip-check --storage-class-matrix=ocs-storagecluster-ceph-rbd-virtualization --skip-deprecated-api-test tests/storage/ tests/data_protection/oadp/ --collect-only`
Expected: collection succeeds with the direct `utilities.constants.<submodule>` imports; no `ImportError` or circular-import regression.
Happy path (regression):
`uv run pytest -rA -s -o log_cli=true --tc-file=tests/global_config_amd64.py --tc-format=python --cluster-sanity-skip-check --storage-class-matrix=ocs-storagecluster-ceph-rbd-virtualization --skip-deprecated-api-test tests/storage/cdi_import/test_import_http.py tests/storage/snapshots/test_snapshots.py tests/data_protection/oadp/test_velero.py --collect-only`
Expected: representative storage and OADP modules still collect normally after the import-path refactor.
Note: no `utilities/` or `libs/` implementation files changed, and no new tests were added; risk is limited to import resolution in changed test modules and `conftest.py` setup scopes.
"""
payload = {
"body": body,
"commit_id": head,
"path": first_file,
"side": "RIGHT",
"line": first_line,
}
path = pathlib.Path("/tmp/coderabbit_tep_payload.json")
path.write_text(json.dumps(payload))
subprocess.check_call([
"gh", "api",
f"/repos/{owner}/{name}/pulls/{pr}/comments",
"-X", "POST",
"--input", str(path),
])
PYLength of output: 6280 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
owner="RedHatQE"
name="openshift-virtualization-tests"
pr="5320"
tmp_meta="$(mktemp)"
tmp_files="$(mktemp)"
tmp_payload="$(mktemp)"
trap 'rm -f "$tmp_meta" "$tmp_files" "$tmp_payload"' EXIT
gh pr view "$pr" --repo "$repo" --json baseRefOid,headRefOid,files > "$tmp_meta"
base="$(jq -r '.baseRefOid' "$tmp_meta")"
head="$(jq -r '.headRefOid' "$tmp_meta")"
jq -r '.files[].path' "$tmp_meta" > "$tmp_files"
python - "$owner" "$name" "$pr" "$base" "$head" "$tmp_files" "$tmp_payload" <<'PY'
import json
import pathlib
import re
import subprocess
import sys
owner, name, pr, base, head, files_path, payload_path = sys.argv[1:]
changed_files = [line for line in pathlib.Path(files_path).read_text().splitlines() if line]
if not changed_files:
raise SystemExit("No changed files found")
first_file = changed_files[0]
changed_set = set(changed_files)
changed_conftests = [f for f in changed_files if f.endswith("conftest.py")]
changed_conftest_set = set(changed_conftests)
# First added line in first changed file.
diff = subprocess.check_output(["git", "diff", f"{base}...{head}", "--", first_file], text=True)
new_line = None
first_line = None
for line in diff.splitlines():
if line.startswith("@@"):
m = re.search(r"\+(\d+)(?:,(\d+))?", line)
new_line = int(m.group(1)) if m else None
continue
if new_line is None:
continue
if line.startswith("+") and not line.startswith("+++"):
first_line = new_line
break
if not line.startswith("-"):
new_line += 1
if first_line is None:
raise SystemExit(f"No added line found in {first_file}")
def rg_files(pattern: str):
proc = subprocess.run(["rg", "-l", pattern, "tests/"], text=True, capture_output=True, check=False)
return set(filter(None, proc.stdout.splitlines()))
smoke_files = rg_files(r'`@pytest`\.mark\.smoke')
gating_files = rg_files(r'`@pytest`\.mark\.gating')
def parent_conftests(path: str):
parts = path.split("/")[:-1]
out = []
while parts:
out.append("/".join(parts + ["conftest.py"]))
parts.pop()
return out
smoke_hits = []
for sf in sorted(smoke_files):
if sf in changed_set:
smoke_hits.append(f"`{sf}` changed directly")
continue
hits = [c for c in parent_conftests(sf) if c in changed_conftest_set]
if hits:
smoke_hits.append(f"`{sf}` → `{hits[0]}`")
gating_hits = []
for gf in sorted(gating_files):
if gf in changed_set:
gating_hits.append(f"`{gf}` changed directly")
continue
hits = [c for c in parent_conftests(gf) if c in changed_conftest_set]
if hits:
gating_hits.append(f"`{gf}` → `{hits[0]}`")
affected = []
for f in changed_files:
if f.endswith("conftest.py"):
scope = f.rsplit("/", 1)[0] + "/"
affected.append(f"`{scope}` (collection/setup scope via changed `{f}`)")
elif pathlib.Path(f).name.startswith("test_") and f.endswith(".py"):
tags = []
if f in smoke_files:
tags.append("smoke")
if f in gating_files:
tags.append("gating")
suffix = f" ({', '.join(tags)})" if tags else ""
affected.append(f"`{f}`{suffix}")
deduped = []
seen = set()
for item in affected:
if item not in seen:
deduped.append(item)
seen.add(item)
affected_text = "\n".join(f"- {item}" for item in deduped)
smoke_true = bool(smoke_hits)
gating_true = bool(gating_hits)
smoke_detail = (
"True — verified collection/setup path: "
+ "; ".join(smoke_hits[:8])
+ ("; ..." if len(smoke_hits) > 8 else "")
+ " → changed import path in the test module/conftest."
if smoke_true else
"False — smoke-marked files exist in `tests/`, but none were changed directly and none inherit a changed parent `conftest.py` scope in this PR."
)
gating_detail = (
"True — verified collection/setup path: "
+ "; ".join(gating_hits[:8])
+ ("; ..." if len(gating_hits) > 8 else "")
+ " → changed import path in the test module/conftest."
if gating_true else
"False — gating-marked files exist in `tests/`, but none were changed directly and none inherit a changed parent `conftest.py` scope in this PR."
)
body = f"""**Test Execution Plan**
- **Run smoke tests: {'True' if smoke_true else 'False'}** — {smoke_detail}
- **Run gating tests: {'True' if gating_true else 'False'}** — {gating_detail}
**Affected tests to run**
{affected_text}
**Real tests (cluster required)**
Error/fix path (import resolution at collection/setup time):
`uv run pytest -rA -s -o log_cli=true --tc-file=tests/global_config_amd64.py --tc-format=python --cluster-sanity-skip-check --storage-class-matrix=ocs-storagecluster-ceph-rbd-virtualization --skip-deprecated-api-test tests/storage/ tests/data_protection/oadp/ --collect-only`
Expected: collection succeeds with the direct `utilities.constants.<submodule>` imports; no `ImportError` or circular-import regression.
Happy path (regression):
`uv run pytest -rA -s -o log_cli=true --tc-file=tests/global_config_amd64.py --tc-format=python --cluster-sanity-skip-check --storage-class-matrix=ocs-storagecluster-ceph-rbd-virtualization --skip-deprecated-api-test tests/storage/cdi_import/test_import_http.py tests/storage/snapshots/test_snapshots.py tests/data_protection/oadp/test_velero.py --collect-only`
Expected: representative storage and OADP modules still collect normally after the import-path refactor.
Note: no `utilities/` or `libs/` implementation files changed, and no new tests were added; risk is limited to import resolution in changed test modules and `conftest.py` setup scopes.
"""
payload = {
"body": body,
"commit_id": head,
"path": first_file,
"side": "RIGHT",
"line": first_line,
}
pathlib.Path(payload_path).write_text(json.dumps(payload))
print(json.dumps({
"path": first_file,
"line": first_line,
"smoke_true": smoke_true,
"gating_true": gating_true,
"smoke_hits_sample": smoke_hits[:5],
"gating_hits_sample": gating_hits[:5],
}, indent=2))
PY
gh api "/repos/$owner/$name/pulls/$pr/comments" -X POST --input "$tmp_payload"Length of output: 7408 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
owner="RedHatQE"
name="openshift-virtualization-tests"
pr="5320"
tmp_meta="$(mktemp)"
tmp_files="$(mktemp)"
tmp_body="$(mktemp)"
tmp_payload="$(mktemp)"
trap 'rm -f "$tmp_meta" "$tmp_files" "$tmp_body" "$tmp_payload"' EXIT
gh pr view "$pr" --repo "$repo" --json files > "$tmp_meta"
jq -r '.files[].path' "$tmp_meta" > "$tmp_files"
python - "$tmp_files" "$tmp_body" <<'PY'
import pathlib
import subprocess
import sys
files_path, body_path = sys.argv[1:]
changed_files = [line for line in pathlib.Path(files_path).read_text().splitlines() if line]
changed_set = set(changed_files)
changed_conftests = [f for f in changed_files if f.endswith("conftest.py")]
changed_conftest_set = set(changed_conftests)
def rg_files(pattern: str):
proc = subprocess.run(["rg", "-l", pattern, "tests/"], text=True, capture_output=True, check=False)
return set(filter(None, proc.stdout.splitlines()))
smoke_files = rg_files(r'`@pytest`\.mark\.smoke')
gating_files = rg_files(r'`@pytest`\.mark\.gating')
def parent_conftests(path: str):
parts = path.split("/")[:-1]
out = []
while parts:
out.append("/".join(parts + ["conftest.py"]))
parts.pop()
return out
smoke_hits = []
for sf in sorted(smoke_files):
if sf in changed_set:
smoke_hits.append(f"`{sf}` changed directly")
continue
hits = [c for c in parent_conftests(sf) if c in changed_conftest_set]
if hits:
smoke_hits.append(f"`{sf}` → `{hits[0]}`")
gating_hits = []
for gf in sorted(gating_files):
if gf in changed_set:
gating_hits.append(f"`{gf}` changed directly")
continue
hits = [c for c in parent_conftests(gf) if c in changed_conftest_set]
if hits:
gating_hits.append(f"`{gf}` → `{hits[0]}`")
affected = []
for f in changed_files:
if f.endswith("conftest.py"):
scope = f.rsplit("/", 1)[0] + "/"
affected.append(f"`{scope}` (collection/setup scope via changed `{f}`)")
elif pathlib.Path(f).name.startswith("test_") and f.endswith(".py"):
tags = []
if f in smoke_files:
tags.append("smoke")
if f in gating_files:
tags.append("gating")
suffix = f" ({', '.join(tags)})" if tags else ""
affected.append(f"`{f}`{suffix}")
seen = set()
deduped = []
for item in affected:
if item not in seen:
deduped.append(item)
seen.add(item)
affected_text = "\n".join(f"- {item}" for item in deduped)
smoke_true = bool(smoke_hits)
gating_true = bool(gating_hits)
smoke_detail = (
"True — verified collection/setup path: "
+ "; ".join(smoke_hits[:8])
+ ("; ..." if len(smoke_hits) > 8 else "")
+ " → changed import path in the test module/conftest."
if smoke_true else
"False — smoke-marked files exist in `tests/`, but none were changed directly and none inherit a changed parent `conftest.py` scope in this PR."
)
gating_detail = (
"True — verified collection/setup path: "
+ "; ".join(gating_hits[:8])
+ ("; ..." if len(gating_hits) > 8 else "")
+ " → changed import path in the test module/conftest."
if gating_true else
"False — gating-marked files exist in `tests/`, but none were changed directly and none inherit a changed parent `conftest.py` scope in this PR."
)
body = f"""**Test Execution Plan**
- **Run smoke tests: {'True' if smoke_true else 'False'}** — {smoke_detail}
- **Run gating tests: {'True' if gating_true else 'False'}** — {gating_detail}
**Affected tests to run**
{affected_text}
**Real tests (cluster required)**
Error/fix path (import resolution at collection/setup time):
`uv run pytest -rA -s -o log_cli=true --tc-file=tests/global_config_amd64.py --tc-format=python --cluster-sanity-skip-check --storage-class-matrix=ocs-storagecluster-ceph-rbd-virtualization --skip-deprecated-api-test tests/storage/ tests/data_protection/oadp/ --collect-only`
Expected: collection succeeds with the direct `utilities.constants.<submodule>` imports; no `ImportError` or circular-import regression.
Happy path (regression):
`uv run pytest -rA -s -o log_cli=true --tc-file=tests/global_config_amd64.py --tc-format=python --cluster-sanity-skip-check --storage-class-matrix=ocs-storagecluster-ceph-rbd-virtualization --skip-deprecated-api-test tests/storage/cdi_import/test_import_http.py tests/storage/snapshots/test_snapshots.py tests/data_protection/oadp/test_velero.py --collect-only`
Expected: representative storage and OADP modules still collect normally after the import-path refactor.
Note: no `utilities/` or `libs/` implementation files changed, and no new tests were added; risk is limited to import resolution in changed test modules and `conftest.py` setup scopes.
"""
pathlib.Path(body_path).write_text(body)
PY
jq -n --rawfile body "$tmp_body" '{body: $body}' > "$tmp_payload"
gh api "/repos/$owner/$name/issues/$pr/comments" -X POST --input "$tmp_payload"Length of output: 5486
CRITICAL: I could not complete the required posting step because this integration does not have comment-write permission on Why this happened:
Ready-to-post test execution plan: **Test Execution Plan**
- **Run smoke tests: True** — verified collection/setup path: `tests/storage/cdi_clone/test_clone.py` changed directly; `tests/storage/cdi_upload/test_upload_virtctl.py` changed directly → changed import path in the test module/conftest.
- **Run gating tests: True** — verified collection/setup path: `tests/storage/cdi_clone/test_clone.py` changed directly; `tests/storage/cdi_config/test_cdi_config.py` changed directly; `tests/storage/cdi_import/test_import_http.py` changed directly; `tests/storage/cdi_upload/test_upload_virtctl.py` changed directly; `tests/storage/disk_preallocation/test_disk_preallocation.py` → `tests/storage/conftest.py` → changed import path in the test module/conftest.
**Affected tests to run**
- `tests/data_protection/oadp/` (collection/setup scope via changed `tests/data_protection/oadp/conftest.py`)
- `tests/data_protection/oadp/test_velero.py`
- `tests/storage/cdi_clone/` (collection/setup scope via changed `tests/storage/cdi_clone/conftest.py`)
- `tests/storage/cdi_clone/test_clone.py` (smoke, gating)
- `tests/storage/cdi_config/test_cdi_config.py` (gating)
- `tests/storage/cdi_import/` (collection/setup scope via changed `tests/storage/cdi_import/conftest.py`)
- `tests/storage/cdi_import/test_import_http.py` (gating)
- `tests/storage/cdi_import/test_import_registry.py`
- `tests/storage/cdi_upload/` (collection/setup scope via changed `tests/storage/cdi_upload/conftest.py`)
- `tests/storage/cdi_upload/test_upload.py`
- `tests/storage/cdi_upload/test_upload_virtctl.py` (smoke, gating)
- `tests/storage/checkups/` (collection/setup scope via changed `tests/storage/checkups/conftest.py`)
- `tests/storage/checkups/test_checkups.py`
- `tests/storage/` (collection/setup scope via changed `tests/storage/conftest.py`)
- `tests/storage/cross_cluster_live_migration/` (collection/setup scope via changed `tests/storage/cross_cluster_live_migration/conftest.py`)
- `tests/storage/cross_cluster_live_migration/test_cclm.py`
- `tests/storage/data_import_cron/` (collection/setup scope via changed `tests/storage/data_import_cron/conftest.py`)
- `tests/storage/general/` (collection/setup scope via changed `tests/storage/general/conftest.py`)
- `tests/storage/general/test_storage_behavior.py`
- `tests/storage/golden_image/test_cached_snapshots.py`
- `tests/storage/golden_image/test_golden_image.py`
- `tests/storage/hpp/` (collection/setup scope via changed `tests/storage/hpp/conftest.py`)
- `tests/storage/hpp/test_hostpath.py`
- `tests/storage/hpp/test_hpp_node_placement.py`
- `tests/storage/memory_dump/` (collection/setup scope via changed `tests/storage/memory_dump/conftest.py`)
- `tests/storage/online_resize/` (collection/setup scope via changed `tests/storage/online_resize/conftest.py`)
- `tests/storage/online_resize/test_online_resize.py`
- `tests/storage/restricted_namespace_cloning/` (collection/setup scope via changed `tests/storage/restricted_namespace_cloning/conftest.py`)
- `tests/storage/restricted_namespace_cloning/test_restricted_namespace_cloning.py`
- `tests/storage/restricted_namespace_cloning/test_restricted_namespace_cloning_vms.py`
- `tests/storage/snapshots/` (collection/setup scope via changed `tests/storage/snapshots/conftest.py`)
- `tests/storage/snapshots/test_snapshots.py`
- `tests/storage/storage_migration/` (collection/setup scope via changed `tests/storage/storage_migration/conftest.py`)
- `tests/storage/storage_migration/test_storage_class_migration.py`
- `tests/storage/test_cdi_certificate.py`
- `tests/storage/test_cdi_resources.py`
- `tests/storage/test_data_import_cron.py`
- `tests/storage/test_hotplug.py`
- `tests/storage/test_libguestfs.py`
- `tests/storage/test_wffc.py`
- `tests/storage/upgrade/` (collection/setup scope via changed `tests/storage/upgrade/conftest.py`)
- `tests/storage/upgrade/test_upgrade_storage.py`
- `tests/storage/vm_export/` (collection/setup scope via changed `tests/storage/vm_export/conftest.py`)
**Real tests (cluster required)**
Error/fix path (import resolution at collection/setup time):
`uv run pytest -rA -s -o log_cli=true --tc-file=tests/global_config_amd64.py --tc-format=python --cluster-sanity-skip-check --storage-class-matrix=ocs-storagecluster-ceph-rbd-virtualization --skip-deprecated-api-test tests/storage/ tests/data_protection/oadp/ --collect-only`
Expected: collection succeeds with the direct `utilities.constants.<submodule>` imports; no `ImportError` or circular-import regression.
Happy path (regression):
`uv run pytest -rA -s -o log_cli=true --tc-file=tests/global_config_amd64.py --tc-format=python --cluster-sanity-skip-check --storage-class-matrix=ocs-storagecluster-ceph-rbd-virtualization --skip-deprecated-api-test tests/storage/cdi_import/test_import_http.py tests/storage/snapshots/test_snapshots.py tests/data_protection/oadp/test_velero.py --collect-only`
Expected: representative storage and OADP modules still collect normally after the import-path refactor.
Note: no `utilities/` or `libs/` implementation files changed, and no new tests were added; risk is limited to import resolution in changed test modules and `conftest.py` setup scopes.Target location for the inline comment, if permissions are fixed:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/storage/hpp/test_hpp_node_placement.py`:
- Line 17: The HPP node-placement test is importing NODE_STR from the AAQ
constants module, creating an unnecessary cross-domain dependency. Update the
test to stop depending on utilities.constants.aaq and instead use a
domain-neutral source for the node key, or replace the import with a local plain
"node" value in the HPP test setup/assertions. Keep the change limited to the
test code that references NODE_STR so the placement tests remain independent of
AAQ-specific constants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 509073fe-5ac0-49cc-ae8c-d6968c1ae4b1
📒 Files selected for processing (56)
tests/data_protection/oadp/conftest.pytests/data_protection/oadp/test_velero.pytests/data_protection/oadp/utils.pytests/storage/cdi_clone/conftest.pytests/storage/cdi_clone/test_clone.pytests/storage/cdi_config/test_cdi_config.pytests/storage/cdi_import/conftest.pytests/storage/cdi_import/test_import_http.pytests/storage/cdi_import/test_import_registry.pytests/storage/cdi_import/utils.pytests/storage/cdi_upload/conftest.pytests/storage/cdi_upload/test_upload.pytests/storage/cdi_upload/test_upload_virtctl.pytests/storage/checkups/conftest.pytests/storage/checkups/constants.pytests/storage/checkups/test_checkups.pytests/storage/conftest.pytests/storage/constants.pytests/storage/cross_cluster_live_migration/conftest.pytests/storage/cross_cluster_live_migration/test_cclm.pytests/storage/cross_cluster_live_migration/utils.pytests/storage/data_import_cron/conftest.pytests/storage/general/conftest.pytests/storage/general/test_storage_behavior.pytests/storage/golden_image/test_cached_snapshots.pytests/storage/golden_image/test_golden_image.pytests/storage/hpp/conftest.pytests/storage/hpp/test_hostpath.pytests/storage/hpp/test_hpp_node_placement.pytests/storage/hpp/utils.pytests/storage/memory_dump/conftest.pytests/storage/memory_dump/utils.pytests/storage/online_resize/conftest.pytests/storage/online_resize/test_online_resize.pytests/storage/online_resize/utils.pytests/storage/restricted_namespace_cloning/conftest.pytests/storage/restricted_namespace_cloning/test_restricted_namespace_cloning.pytests/storage/restricted_namespace_cloning/test_restricted_namespace_cloning_vms.pytests/storage/restricted_namespace_cloning/utils.pytests/storage/snapshots/conftest.pytests/storage/snapshots/test_snapshots.pytests/storage/snapshots/utils.pytests/storage/storage_migration/conftest.pytests/storage/storage_migration/test_storage_class_migration.pytests/storage/storage_migration/utils.pytests/storage/test_cdi_certificate.pytests/storage/test_cdi_resources.pytests/storage/test_data_import_cron.pytests/storage/test_hotplug.pytests/storage/test_libguestfs.pytests/storage/test_wffc.pytests/storage/upgrade/conftest.pytests/storage/upgrade/test_upgrade_storage.pytests/storage/utils.pytests/storage/vm_export/conftest.pytests/storage/vm_export/utils.py
openshift-virtualization-qe-bot-5
added
lgtm-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
labels
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Replace
from utilities.constants import Xwith direct submodule imports across all 57 files in tests/storage/ and tests/data_protection/.from utilities.constants import Imagesis intentionally kept as-is — Images is computed dynamically in init.py until the circular import in utilities/architecture.py is resolved.Generated-by: Claude Sonnet 4.6
Which issue(s) this PR fixes:
Special notes for reviewer:
This is a next PR in a series.
https://redhat.atlassian.net/browse/CNV-80952 and a follow up of #5188
jira-ticket:
https://redhat.atlassian.net/browse/CNV-89565
Summary by CodeRabbit
Refactor
Chores