[Storage] STD: Make Velero Hooks in virt-launcher Optional or Removable#5340
[Storage] STD: Make Velero Hooks in virt-launcher Optional or Removable#5340ema-aka-young wants to merge 10 commits into
Conversation
Signed-off-by: Emanuele Prella <eprella@redhat.com>
Signed-off-by: Emanuele Prella <eprella@redhat.com>
Signed-off-by: Emanuele Prella <eprella@redhat.com>
Signed-off-by: Emanuele Prella <eprella@redhat.com>
…oks-std Signed-off-by: Emanuele Prella <eprella@redhat.com>
Signed-off-by: Emanuele Prella <eprella@redhat.com>
ema-aka-young
changed the title
📝 WalkthroughWalkthroughA new test module ChangesVelero Hook Opt-Out Tests
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested labels
A few observations worth flagging: HIGH — Both classes are marked # More discoverable than __test__ = False:
`@pytest.mark.skip`(reason="CNV-16267: implementation pending")
def test_backup_succeeds_with_hooks_disabled_on_paused_vm(self): ...MEDIUM — Test method bodies are empty beyond docstrings Both LOW — No fixtures declared The test methods take only Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/data_protection/oadp/test_velero_backup_hooks.py`:
- Around line 23-34: The test method
`test_backup_succeeds_with_hooks_disabled_on_paused_vm` violates the STD rule by
defining multiple expected outcomes in its docstring (lines 31-34). Split this
test into separate single-purpose test cases where each test method has its own
docstring with only one `Expected:` outcome. Create one test to verify the
backup completes with status Completed and a separate test to verify no
freeze/unfreeze hooks are executed during backup. Apply the same refactoring to
the other test case mentioned in the comment (lines 48-62) to ensure each test
validates only one aspect.
🪄 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: 6a9b2b55-661b-41a8-87d9-624f81f08f86
📒 Files selected for processing (1)
tests/data_protection/oadp/test_velero_backup_hooks.py
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
D/S test |
|
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-6
requested review from
awels,
josemacassan and
jpeimer
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
/wip |
openshift-virtualization-qe-bot
removed
the
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
D/S test |
Signed-off-by: Emanuele Prella <eprella@redhat.com>
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
D/S test |
|
/wip cancel |
openshift-virtualization-qe-bot-5
changed the title
|
/verified |
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: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the content of the new test file
fd test_velero_backup_hooks.py --exec cat -n {}
# Check for smoke/gating markers in the new file
rg '`@pytest.mark.smoke`|`@pytest.mark.gating`' tests/data_protection/oadp/ -n
# Check if there's a conftest in the oadp directory
fd conftest.py tests/data_protection/ --exec cat -n {}
# Check for smoke and gating markers across tests (just file list)
rg -l '`@pytest.mark.smoke`' tests/ | head -20
rg -l '`@pytest.mark.gating`' tests/ | head -20
# Check imports in the new file
rg 'import|from' tests/data_protection/oadp/test_velero_backup_hooks.pyLength of output: 18197 🏁 Script executed: #!/bin/bash
# Get PR number and repo details, then post inline review comment
PR_NUMBER=5340
REPO="RedHatQE/openshift-virtualization-tests"
# Get the latest commit SHA for the PR
COMMIT_SHA=$(gh api repos/$REPO/pulls/$PR_NUMBER --jq '.head.sha')
echo "Commit SHA: $COMMIT_SHA"
# Post a non-blocking inline review comment on the first changed line of the new file
gh api repos/$REPO/pulls/$PR_NUMBER/reviews \
--method POST \
--field commit_id="$COMMIT_SHA" \
--field event="COMMENT" \
--field body="" \
--field "comments[][path]=tests/data_protection/oadp/test_velero_backup_hooks.py" \
--field "comments[][position]=1" \
--field "comments[][body]=**Test Execution Plan**
- **Run smoke tests: False** — No smoke-marked tests trace through the new file. The added file only imports \`pytest\` with no fixture dependencies and is not collected by pytest (\`__test__ = False\`).
- **Run gating tests: False** — No gating-marked tests are in the affected set. No utilities or libs were modified.
**Affected tests to run:**
\`\`\`
tests/data_protection/oadp/test_velero_backup_hooks.py --collect-only
\`\`\`
Expected: 0 tests collected (both classes have \`__test__ = False\` — these are stub/placeholder definitions pending full implementation).
> ℹ️ Once \`__test__\` is removed or set to \`True\` and fixtures are wired in, re-evaluate this plan against the conftest fixtures in \`tests/data_protection/oadp/conftest.py\`."Length of output: 3374 |
|
/verified cancel |
|
/verified |
| __test__ = False | ||
|
|
||
| @pytest.mark.polarion("CNV-16267") | ||
| def test_backup_paused_vm_hooks_disabled(self): |
There was a problem hiding this comment.
Do we need both of these tests? I'm not sure I fully understand the difference between them, aside from the restore part in the second test. Could you clarify the intent?
There was a problem hiding this comment.
The main difference is the VM state. In this one we are performing the backup of a paused VM to provide coverage for this use case.
There was a problem hiding this comment.
Why not to restore the VM?
There was a problem hiding this comment.
From the STP:
- [CNV-79727] — As a cluster administrator, I want to verify that the opt-out annotation is honored for a paused VM, confirming hooks are not injected regardless of VM state
- Test Scenario: [Tier 2] Deploy a VM configured to opt out of backup hooks and pause it. Run a Velero backup. Confirm no hooks were attempted and the backup completed successfully.
This case proves the annotation is honored for a paused state, while the next one proves the full backup/restore works without hooks. From my understanding, the restore doesn't know or care if the state of VM when backing up was paused or running.
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: |
|
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_NUMBER="5340"
workdir="$(mktemp -d)"
trap 'rm -rf "$workdir"' EXIT
echo "== Fetch PR metadata and changed files =="
gh api "repos/$REPO/pulls/$PR_NUMBER" > "$workdir/pr.json"
gh api --paginate --slurp "repos/$REPO/pulls/$PR_NUMBER/files" > "$workdir/pr_files_pages.json"
python <<'PY' "$workdir/pr_files_pages.json" "$workdir/pr_files.json"
import json, sys
pages_path, out_path = sys.argv[1], sys.argv[2]
pages = json.load(open(pages_path))
files = [item for page in pages for item in page]
json.dump(files, open(out_path, "w"))
print(f"flattened_files={len(files)}")
for i, f in enumerate(files, 1):
print(f"{i:02d} {f['status']:>9} {f['filename']}")
PY
echo
echo "== Determine first changed file and first added line =="
python <<'PY' "$workdir/pr_files.json" "$workdir/first_target.json"
import json, re, sys
files = json.load(open(sys.argv[1]))
out_path = sys.argv[2]
def first_added_line_from_patch(patch: str):
if not patch:
return None
new_line = None
for raw in patch.splitlines():
m = re.match(r'^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@', raw)
if m:
new_line = int(m.group(1))
continue
if new_line is None:
continue
if raw.startswith('+') and not raw.startswith('+++'):
return new_line
if raw.startswith('-') and not raw.startswith('---'):
continue
new_line += 1
return None
target = None
for f in files:
line = first_added_line_from_patch(f.get("patch") or "")
if line is not None:
target = {"path": f["filename"], "line": line}
break
if target is None:
raise SystemExit("No commentable added line found in PR patches.")
json.dump(target, open(out_path, "w"))
print(json.dumps(target, indent=2))
PY
first_path="$(jq -r '.path' "$workdir/first_target.json")"
first_line="$(jq -r '.line' "$workdir/first_target.json")"
echo
echo "== Inspect changed Python/test/conftest/utilities/libs files =="
python <<'PY' "$workdir/pr_files.json" > "$workdir/interesting_paths.txt"
import json, sys
files = json.load(open(sys.argv[1]))
for f in files:
path = f["filename"]
if path.endswith(".py") or "/conftest.py" in path or path.startswith("tests/") or path.startswith("utilities/") or path.startswith("libs/"):
print(path)
PY
if [[ -s "$workdir/interesting_paths.txt" ]]; then
while IFS= read -r path; do
echo "--- $path"
if [[ -f "$path" ]]; then
case "$path" in
*.py)
ast-grep outline "$path" --view expanded || true
sed -n '1,220p' "$path"
;;
*)
sed -n '1,220p' "$path"
;;
esac
else
echo "(not present in working tree)"
fi
echo
done < "$workdir/interesting_paths.txt"
else
echo "No directly interesting repo files matched the inspection filters."
fi
echo
echo "== Smoke and gating marker inventory =="
rg -n --glob 'tests/**/*.py' '`@pytest`\.mark\.smoke' tests/ || true
echo "---"
rg -n --glob 'tests/**/*.py' '`@pytest`\.mark\.gating' tests/ || true
echo
echo "== Analyze PR impact and compose inline review body =="
python <<'PY' "$workdir/pr_files.json" "$workdir/comment_body.md"
import json, os, re, sys, subprocess
from pathlib import Path
files = json.load(open(sys.argv[1]))
out_path = sys.argv[2]
changed_paths = [f["filename"] for f in files]
changed_tests = [p for p in changed_paths if p.startswith("tests/") and p.endswith(".py")]
changed_utilities = [p for p in changed_paths if p.startswith("utilities/") or p.startswith("libs/")]
changed_conftests = [p for p in changed_paths if p.endswith("conftest.py")]
def run(cmd):
return subprocess.run(cmd, text=True, capture_output=True, check=False)
def file_text(path):
try:
return Path(path).read_text()
except Exception:
return ""
smoke_files = []
gating_files = []
smoke_res = run(["rg", "-l", "--glob", "tests/**/*.py", "`@pytest`\\.mark\\.smoke", "tests/"])
if smoke_res.returncode in (0, 1):
smoke_files = [line.strip() for line in smoke_res.stdout.splitlines() if line.strip()]
gating_res = run(["rg", "-l", "--glob", "tests/**/*.py", "`@pytest`\\.mark\\.gating", "tests/"])
if gating_res.returncode in (0, 1):
gating_files = [line.strip() for line in gating_res.stdout.splitlines() if line.strip()]
# Utilities/libs impact tracing if needed.
modified_symbols = {}
same_file_callers = {}
direct_callers = {}
if changed_utilities:
diff_res = run(["gh", "api", f"repos/RedHatQE/openshift-virtualization-tests/pulls/5340/files", "--paginate"])
# Fallback to working tree diff if needed is intentionally omitted; API patches are enough here.
for f in files:
path = f["filename"]
if not (path.startswith("utilities/") or path.startswith("libs/")):
continue
patch = f.get("patch") or ""
symbols = []
for line in patch.splitlines():
if re.match(r'^[+-]\s*def\s+[A-Za-z_][A-Za-z0-9_]*\s*\(', line):
symbols.append(re.sub(r'^[+-]\s*def\s+([A-Za-z_][A-Za-z0-9_]*)\s*\(.*$', r'\1', line))
if re.match(r'^[+-]\s*class\s+[A-Za-z_][A-Za-z0-9_]*\s*[\(:]', line):
symbols.append(re.sub(r'^[+-]\s*class\s+([A-Za-z_][A-Za-z0-9_]*)[\(:].*$', r'\1', line))
modified_symbols[path] = sorted(set(symbols))
text = file_text(path)
callers = set()
for sym in modified_symbols[path]:
pattern = rf'\b{re.escape(sym)}\s*\('
if re.search(pattern, text):
callers.add(sym)
same_file_callers[path] = sorted(callers)
path_callers = set()
for sym in modified_symbols[path]:
res = run(["rg", "-n", "--glob", "tests/**/*.py", "--glob", "tests/**/conftest.py", rf"\b{sym}\b", "tests/"])
if res.returncode in (0, 1):
for ln in res.stdout.splitlines():
if ln.strip():
path_callers.add(ln.split(":", 1)[0])
direct_callers[path] = sorted(path_callers)
# Determine disabled placeholder test files.
affected_entries = []
notes = []
for path in changed_tests:
text = file_text(path)
disabled = "__test__ = False" in text
has_smoke = "`@pytest.mark.smoke`" in text
has_gating = "`@pytest.mark.gating`" in text
if disabled:
affected_entries.append(f"`{path} --collect-only`")
notes.append(f"`{path}` defines placeholder tests with `__test__ = False`, so collection should verify they stay non-runnable.")
else:
affected_entries.append(f"`{path}`")
if has_smoke:
notes.append(f"`{path}` is directly smoke-marked.")
if has_gating:
notes.append(f"`{path}` is directly gating-marked.")
# Determine smoke/gating decision with concrete paths only.
run_smoke = False
run_gating = False
smoke_reason = "No verified dependency path from a smoke-marked test to a changed utility, shared fixture, or collected test target."
gating_reason = "No verified dependency path from a gating-marked test to a changed utility, shared fixture, or collected test target."
# Directly changed smoke/gating tests count as affected if they are actually collected.
for path in changed_tests:
text = file_text(path)
disabled = "__test__ = False" in text
if not disabled and "`@pytest.mark.smoke`" in text:
run_smoke = True
smoke_reason = f"Direct impact: `{path}` is a changed collected smoke-marked test file."
if not disabled and "`@pytest.mark.gating`" in text:
run_gating = True
gating_reason = f"Direct impact: `{path}` is a changed collected gating-marked test file."
# Utilities/shared-conftest would require broader expansion; only set True if a concrete file was found.
if changed_utilities:
smoke_hit = None
gating_hit = None
affected_test_paths = set()
for _, callers in direct_callers.items():
affected_test_paths.update(p for p in callers if p.startswith("tests/"))
for p in sorted(affected_test_paths):
if p in smoke_files and smoke_hit is None:
smoke_hit = p
if p in gating_files and gating_hit is None:
gating_hit = p
if smoke_hit:
run_smoke = True
smoke_reason = f"Verified utility dependency path reaches smoke-marked file `{smoke_hit}`."
if gating_hit:
run_gating = True
gating_reason = f"Verified utility dependency path reaches gating-marked file `{gating_hit}`."
if affected_test_paths:
for p in sorted(affected_test_paths):
affected_entries.append(f"`{p}`")
# Shared conftest changes affect collection/runtime within their tree.
if changed_conftests:
for conftest in changed_conftests:
affected_entries.append(f"`{conftest}`")
# Only mark smoke/gating true if changed conftest is in hierarchy of known smoke/gating files.
for conftest in changed_conftests:
conftest_dir = str(Path(conftest).parent)
for s in smoke_files:
if s.startswith(conftest_dir.rstrip("/") + "/"):
run_smoke = True
smoke_reason = f"Shared fixture path: `{s}` uses `{conftest}` during collection/runtime."
break
for g in gating_files:
if g.startswith(conftest_dir.rstrip("/") + "/"):
run_gating = True
gating_reason = f"Shared fixture path: `{g}` uses `{conftest}` during collection/runtime."
break
# Deduplicate while preserving order.
seen = set()
affected_entries = [x for x in affected_entries if not (x in seen or seen.add(x))]
if not affected_entries:
if changed_tests:
for p in changed_tests:
affected_entries.append(f"`{p}`")
else:
affected_entries.append("`pytest --collect-only`")
body_lines = []
body_lines.append("**Test Execution Plan**")
body_lines.append("")
body_lines.append(f"- **Run smoke tests: {'True' if run_smoke else 'False'}** — {smoke_reason}")
body_lines.append(f"- **Run gating tests: {'True' if run_gating else 'False'}** — {gating_reason}")
body_lines.append("")
body_lines.append("**Affected tests to run**")
for entry in affected_entries:
body_lines.append(f"- {entry}")
# Real test commands only when runtime/session code changed.
if changed_utilities or changed_conftests:
body_lines.append("")
body_lines.append("**Real tests (cluster required)**")
body_lines.append("Error/fix path:")
if changed_tests:
body_lines.append(f"`pytest {changed_tests[0]} --collect-only`")
body_lines.append("Expected: collection/runtime reaches the updated shared code without fixture/setup errors.")
else:
body_lines.append("`pytest --collect-only`")
body_lines.append("Expected: collection succeeds with the modified shared code.")
body_lines.append("")
body_lines.append("Happy path (regression):")
if changed_tests:
body_lines.append(f"`pytest {changed_tests[0]} -k test_ --maxfail=1`")
body_lines.append("Expected: an affected test still runs successfully on a real cluster.")
else:
body_lines.append("`pytest -k test_ --maxfail=1`")
body_lines.append("Expected: a representative affected test still runs successfully on a real cluster.")
elif notes:
body_lines.append("")
for n in notes[:3]:
body_lines.append(f"> {n}")
Path(out_path).write_text("\n".join(body_lines) + "\n")
print(Path(out_path).read_text())
PY
echo
echo "== Post inline review comment =="
COMMIT_SHA="$(jq -r '.head.sha' "$workdir/pr.json")"
comment_body="$(cat "$workdir/comment_body.md")"
echo "Target path: $first_path"
echo "Target line: $first_line"
echo "Commit SHA: $COMMIT_SHA"
post_review() {
gh api "repos/$REPO/pulls/$PR_NUMBER/reviews" \
--method POST \
--field commit_id="$COMMIT_SHA" \
--field event="COMMENT" \
--field body="" \
--field "comments[][path]=$first_path" \
--field "comments[][line]=$first_line" \
--field "comments[][side]=RIGHT" \
--field "comments[][body]=$comment_body"
}
post_fallback_comment() {
gh api "repos/$REPO/pulls/$PR_NUMBER/comments" \
--method POST \
--field commit_id="$COMMIT_SHA" \
--field path="$first_path" \
--field line="$first_line" \
--field side="RIGHT" \
--field body="$comment_body"
}
set +e
post_review
status=$?
set -e
if [[ $status -ne 0 ]]; then
echo "Primary inline review POST failed; retrying once..." >&2
set +e
post_review
status=$?
set -e
fi
if [[ $status -ne 0 ]]; then
echo "Review POST failed after retry; trying fallback inline comment endpoint..." >&2
post_fallback_comment
fiLength of output: 13256 🏁 Script executed: #!/bin/bash
set -euo pipefail
REPO="RedHatQE/openshift-virtualization-tests"
PR_NUMBER="5340"
workdir="$(mktemp -d)"
trap 'rm -rf "$workdir"' EXIT
echo "== Fetch PR metadata =="
gh api "repos/$REPO/pulls/$PR_NUMBER" > "$workdir/pr.json"
jq -r '.head.sha, .base.ref, .head.ref' "$workdir/pr.json"
echo
echo "== Fetch changed files (robust pagination) =="
gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/files?per_page=100" | jq -s 'add' > "$workdir/pr_files.json"
jq 'length' "$workdir/pr_files.json"
jq -r '.[] | "\(.status)\t\(.filename)"' "$workdir/pr_files.json"
echo
echo "== Determine first changed file and first added line =="
python <<'PY' "$workdir/pr_files.json" "$workdir/first_target.json"
import json, re, sys
files = json.load(open(sys.argv[1]))
target = None
for f in files:
patch = f.get("patch") or ""
new_line = None
for raw in patch.splitlines():
m = re.match(r'^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@', raw)
if m:
new_line = int(m.group(1))
continue
if new_line is None:
continue
if raw.startswith('+') and not raw.startswith('+++'):
target = {"path": f["filename"], "line": new_line}
break
if raw.startswith('-') and not raw.startswith('---'):
continue
new_line += 1
if target:
break
if not target:
raise SystemExit("No commentable added line found in PR patches")
json.dump(target, open(sys.argv[2], "w"))
print(json.dumps(target, indent=2))
PY
first_path="$(jq -r '.path' "$workdir/first_target.json")"
first_line="$(jq -r '.line' "$workdir/first_target.json")"
echo
echo "== Show changed test/python/utilities/conftest files =="
jq -r '.[] | .filename' "$workdir/pr_files.json" | while IFS= read -r path; do
case "$path" in
tests/*.py|utilities/*.py|libs/*.py|*/conftest.py|conftest.py|*.py)
echo "--- $path"
if [[ -f "$path" ]]; then
ast-grep outline "$path" --view expanded || true
sed -n '1,220p' "$path"
else
echo "(not present in working tree)"
fi
echo
;;
esac
done
echo
echo "== Smoke/gating inventory =="
echo "-- smoke files"
rg -l --glob 'tests/**/*.py' '`@pytest`\.mark\.smoke' tests/ || true
echo "-- gating files"
rg -l --glob 'tests/**/*.py' '`@pytest`\.mark\.gating' tests/ || true
echo
echo "== Build inline comment body =="
python <<'PY' "$workdir/pr_files.json" "$workdir/comment_body.md"
import json, subprocess, sys
from pathlib import Path
files = json.load(open(sys.argv[1]))
out = Path(sys.argv[2])
changed_paths = [f["filename"] for f in files]
changed_tests = [p for p in changed_paths if p.startswith("tests/") and p.endswith(".py")]
changed_utils = [p for p in changed_paths if p.startswith("utilities/") or p.startswith("libs/")]
changed_conftests = [p for p in changed_paths if p.endswith("conftest.py")]
def read_text(path):
try:
return Path(path).read_text()
except Exception:
return ""
def rg_files(pattern):
r = subprocess.run(
["rg", "-l", "--glob", "tests/**/*.py", pattern, "tests/"],
text=True,
capture_output=True,
check=False,
)
return [line.strip() for line in r.stdout.splitlines() if line.strip()]
smoke_files = rg_files(r"`@pytest`\.mark\.smoke")
gating_files = rg_files(r"`@pytest`\.mark\.gating")
run_smoke = False
run_gating = False
smoke_reason = "No verified dependency path from a smoke-marked collected test to the modified files."
gating_reason = "No verified dependency path from a gating-marked collected test to the modified files."
affected = []
notes = []
for path in changed_tests:
text = read_text(path)
disabled = "__test__ = False" in text
if disabled:
affected.append(f"`{path} --collect-only`")
notes.append(f"`{path}` contains placeholder classes with `__test__ = False`; collection should confirm they remain non-runnable.")
else:
affected.append(f"`{path}`")
if "`@pytest.mark.smoke`" in text:
run_smoke = True
smoke_reason = f"Direct impact: `{path}` is a changed collected smoke-marked test file."
if "`@pytest.mark.gating`" in text:
run_gating = True
gating_reason = f"Direct impact: `{path}` is a changed collected gating-marked test file."
if changed_utils:
notes.append("Utilities/libs changes detected; broader transitive impact tracing would be required.")
if changed_conftests:
for conftest in changed_conftests:
affected.append(f"`{conftest}`")
parent = str(Path(conftest).parent).rstrip("/")
smoke_hit = next((p for p in smoke_files if p.startswith(parent + "/")), None)
gating_hit = next((p for p in gating_files if p.startswith(parent + "/")), None)
if smoke_hit:
run_smoke = True
smoke_reason = f"Shared fixture path: `{smoke_hit}` depends on `{conftest}` at collection/runtime."
if gating_hit:
run_gating = True
gating_reason = f"Shared fixture path: `{gating_hit}` depends on `{conftest}` at collection/runtime."
if not affected:
affected.append("`pytest --collect-only`")
seen = set()
affected = [x for x in affected if not (x in seen or seen.add(x))]
body = []
body.append("**Test Execution Plan**")
body.append("")
body.append(f"- **Run smoke tests: {'True' if run_smoke else 'False'}** — {smoke_reason}")
body.append(f"- **Run gating tests: {'True' if run_gating else 'False'}** — {gating_reason}")
body.append("")
body.append("**Affected tests to run**")
for entry in affected:
body.append(f"- {entry}")
if changed_utils or changed_conftests:
body.append("")
body.append("**Real tests (cluster required)**")
body.append("Error/fix path:")
target = changed_tests[0] if changed_tests else "tests/"
body.append(f"`pytest {target} --collect-only`")
body.append("Expected: collection/runtime reaches the modified shared code without fixture/setup errors.")
body.append("")
body.append("Happy path (regression):")
body.append(f"`pytest {target} -k test_ --maxfail=1`")
body.append("Expected: an affected test still runs successfully on a real cluster.")
else:
for note in notes[:3]:
body.append("")
body.append(f"> {note}")
out.write_text("\n".join(body) + "\n")
print(out.read_text())
PY
echo
echo "== Post non-blocking inline review comment =="
COMMIT_SHA="$(jq -r '.head.sha' "$workdir/pr.json")"
comment_body="$(cat "$workdir/comment_body.md")"
echo "Target path: $first_path"
echo "Target line: $first_line"
echo "Commit SHA: $COMMIT_SHA"
post_review() {
gh api "repos/$REPO/pulls/$PR_NUMBER/reviews" \
--method POST \
--field commit_id="$COMMIT_SHA" \
--field event="COMMENT" \
--field body="" \
--field "comments[][path]=$first_path" \
--field "comments[][line]=$first_line" \
--field "comments[][side]=RIGHT" \
--field "comments[][body]=$comment_body"
}
post_fallback() {
gh api "repos/$REPO/pulls/$PR_NUMBER/comments" \
--method POST \
--field commit_id="$COMMIT_SHA" \
--field path="$first_path" \
--field line="$first_line" \
--field side="RIGHT" \
--field body="$comment_body"
}
set +e
post_review
status=$?
set -e
if [[ $status -ne 0 ]]; then
echo "Primary review API failed, retrying once..."
set +e
post_review
status=$?
set -e
fi
if [[ $status -ne 0 ]]; then
echo "Review API failed twice, trying fallback inline comment endpoint..."
post_fallback
fiLength of output: 8214 🏁 Script executed: #!/bin/bash
set -euo pipefail
REPO="RedHatQE/openshift-virtualization-tests"
PR_NUMBER="5340"
workdir="$(mktemp -d)"
trap 'rm -rf "$workdir"' EXIT
gh api "repos/$REPO/pulls/$PR_NUMBER" > "$workdir/pr.json"
gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/files?per_page=100" | jq -s 'add' > "$workdir/pr_files.json"
echo "== Changed files =="
jq -r '.[] | "\(.status)\t\(.filename)"' "$workdir/pr_files.json"
echo
echo "== Compute first commentable line =="
python - "$workdir/pr_files.json" "$workdir/target.json" <<'PY'
import json, re, sys
files_path, out_path = sys.argv[1], sys.argv[2]
files = json.load(open(files_path))
target = None
for f in files:
patch = f.get("patch") or ""
new_line = None
for raw in patch.splitlines():
m = re.match(r'^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@', raw)
if m:
new_line = int(m.group(1))
continue
if new_line is None:
continue
if raw.startswith('+') and not raw.startswith('+++'):
target = {"path": f["filename"], "line": new_line}
break
if raw.startswith('-') and not raw.startswith('---'):
continue
new_line += 1
if target:
break
if not target:
raise SystemExit("No commentable added line found")
with open(out_path, "w") as fh:
json.dump(target, fh)
print(json.dumps(target, indent=2))
PY
TARGET_PATH="$(jq -r '.path' "$workdir/target.json")"
TARGET_LINE="$(jq -r '.line' "$workdir/target.json")"
COMMIT_SHA="$(jq -r '.head.sha' "$workdir/pr.json")"
echo
echo "== Inspect changed file =="
sed -n '1,220p' "$TARGET_PATH"
echo
echo "== Build inline comment body =="
python - "$TARGET_PATH" "$workdir/comment_body.md" <<'PY'
import sys
from pathlib import Path
path, out_path = sys.argv[1], sys.argv[2]
text = Path(path).read_text()
disabled = "__test__ = False" in text
has_smoke = "`@pytest.mark.smoke`" in text
has_gating = "`@pytest.mark.gating`" in text
run_smoke = False
run_gating = False
smoke_reason = "No verified dependency path from a smoke-marked collected test to the modified file."
gating_reason = "No verified dependency path from a gating-marked collected test to the modified file."
affected = []
notes = []
if disabled:
affected.append(f"`{path} --collect-only`")
notes.append(f"`{path}` contains placeholder classes with `__test__ = False`; collection should confirm they remain non-runnable.")
else:
affected.append(f"`{path}`")
if has_smoke:
run_smoke = True
smoke_reason = f"Direct impact: `{path}` is a changed collected smoke-marked test file."
if has_gating:
run_gating = True
gating_reason = f"Direct impact: `{path}` is a changed collected gating-marked test file."
body = []
body.append("**Test Execution Plan**")
body.append("")
body.append(f"- **Run smoke tests: {'True' if run_smoke else 'False'}** — {smoke_reason}")
body.append(f"- **Run gating tests: {'True' if run_gating else 'False'}** — {gating_reason}")
body.append("")
body.append("**Affected tests to run**")
for entry in affected:
body.append(f"- {entry}")
for note in notes:
body.append("")
body.append(f"> {note}")
Path(out_path).write_text("\n".join(body) + "\n")
print(Path(out_path).read_text())
PY
COMMENT_BODY="$(cat "$workdir/comment_body.md")"
echo
echo "== Post inline non-blocking review comment =="
echo "Path: $TARGET_PATH"
echo "Line: $TARGET_LINE"
echo "Commit: $COMMIT_SHA"
post_review() {
gh api "repos/$REPO/pulls/$PR_NUMBER/reviews" \
--method POST \
--field commit_id="$COMMIT_SHA" \
--field event="COMMENT" \
--field body="" \
--field "comments[][path]=$TARGET_PATH" \
--field "comments[][line]=$TARGET_LINE" \
--field "comments[][side]=RIGHT" \
--field "comments[][body]=$COMMENT_BODY"
}
post_fallback() {
gh api "repos/$REPO/pulls/$PR_NUMBER/comments" \
--method POST \
--field commit_id="$COMMIT_SHA" \
--field path="$TARGET_PATH" \
--field line="$TARGET_LINE" \
--field side="RIGHT" \
--field body="$COMMENT_BODY"
}
set +e
post_review
status=$?
set -e
if [[ $status -ne 0 ]]; then
set +e
post_review
status=$?
set -e
fi
if [[ $status -ne 0 ]]; then
post_fallback
fiLength of output: 8420 |
8 participants
What this PR does / why we need it:
Introduce STD for Velero Hooks opt-out.
STP here: https://github.com/RedHatQE/openshift-virtualization-tests-design-docs/blob/main/stps/sig-storage/remove-velero-hooks-stp.md
Which issue(s) this PR fixes:
Special notes for reviewer:
Assisted by Quality Flow AI tool
jira-ticket:
https://redhat.atlassian.net/browse/CNV-88654
Summary by CodeRabbit
Tests