Skip to content

fix: remove blanket vsphere marker check that skipped multi-provider tests - #591

Merged
krcmarik merged 2 commits into
RedHatQE:mainfrom
krcmarik:fix/vsphere-skip-logic
Jul 8, 2026
Merged

krcmarik merged 2 commits into
RedHatQE:mainfrom
krcmarik:fix/vsphere-skip-logic

Conversation

@krcmarik

@krcmarik krcmarik commented Jul 8, 2026 •

Copy link
Copy Markdown
Collaborator

The "vsphere" keyword check in pytest_collection_modifyitems incorrectly skipped test classes that use @pytest.mark.vsphere alongside other provider markers (rhv, openstack, etc.) to indicate multi-provider support. Truly vSphere-only tests are naturally excluded by pytest's -m filter expression.
Now all the tests are filtered out for non-vsphere providers.

Summary by CodeRabbit

  • Bug Fixes
    • Adjusted test selection so non-vSphere environments no longer skip tests solely because they are tagged for vSphere, while still excluding copy offload, shared disk, and deep inspection scenarios.

…tests

The "vsphere" keyword check in pytest_collection_modifyitems incorrectly
skipped test classes that use @pytest.mark.vsphere alongside other provider
markers (rhv, openstack, etc.) to indicate multi-provider support. Truly
vSphere-only tests are naturally excluded by pytest's -m filter expression.
@coderabbitai

coderabbitai Bot commented Jul 8, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 58baeeb7-8ee3-43a0-91b4-b3a9a6f8dba9

📥 Commits

Reviewing files that changed from the base of the PR and between 4fca5fa and 02aa8fa.

📒 Files selected for processing (1)
  • conftest.py
💤 Files with no reviewable changes (1)
  • conftest.py

Walkthrough

The pytest_collection_modifyitems function in conftest.py was modified to remove "vsphere" from the keyword conditions that trigger the vsphere-only skip marker for non-vSphere source_provider_type, leaving "copyoffload", "shared_disk", and "deep_inspection" as skip triggers.

Changes

Test Collection Skip Logic

Layer / File(s) Summary
Update vsphere-only skip condition
conftest.py
Removes "vsphere" keyword from the skip condition in pytest_collection_modifyitems, so tests marked "vsphere" are no longer skipped for non-vSphere providers; skip still applies for "copyoffload", "shared_disk", or "deep_inspection" keywords.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Possibly related PRs

  • RedHatQE/mtv-api-tests#574: Adds @pytest.mark.vsphere to warm ClusterRole tests, directly interacting with the updated skip condition in this PR.

Suggested reviewers: myakove, solenoci

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: removing the broad vsphere marker check that wrongly skipped multi-provider tests.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

@rh-bot-1

rh-bot-1 commented Jul 8, 2026

Copy link
Copy Markdown

Report bugs in Issues

Welcome! 🎉

This pull request will be automatically processed with the following features:

🔄 Automatic Actions

  • Reviewer Assignment: Reviewers are automatically assigned based on the OWNERS file in the repository root
  • Size Labeling: PR size labels (XS, S, M, L, XL, XXL) are automatically applied based on changes
  • Issue Creation: Disabled for this repository
  • Branch Labeling: Branch-specific labels are applied to track the target branch
  • Auto-verification: Auto-verified users have their PRs automatically marked as verified
  • Labels: All label categories are enabled (default configuration)

📋 Available Commands

PR Status Management

  • /wip - Mark PR as work in progress (adds WIP: prefix to title)
  • /wip cancel - Remove work in progress status
  • /hold - Block PR merging (approvers only)
  • /hold cancel - Unblock PR merging
  • /verified - Mark PR as verified
  • /verified cancel - Remove verification status
  • /reprocess - Trigger complete PR workflow reprocessing (useful if webhook failed or configuration changed)
  • /regenerate-welcome - Regenerate this welcome message
  • /security-override - Set security check runs to pass (maintainers only)
  • /security-override cancel - Re-run security checks

Review & Approval

  • /lgtm - Approve changes (looks good to me)
  • /approve - Approve PR (approvers only)
  • /automerge - Enable automatic merging when all requirements are met (maintainers and approvers only)
  • /assign-reviewers - Assign reviewers based on OWNERS file
  • /assign-reviewer @username - Assign specific reviewer
  • /check-can-merge - Check if PR meets merge requirements

Testing & Validation

  • /retest tox - Run Python test suite with tox
  • /retest build-container - Rebuild and test container image
  • /retest conventional-title - Validate commit message format
  • /retest all - Run all available tests

Container Operations

  • /build-and-push-container - Build and push container image (tagged with PR number)
    • Supports additional build arguments: /build-and-push-container --build-arg KEY=value

Cherry-pick Operations

  • /cherry-pick <branch> - Schedule cherry-pick to target branch when PR is merged
    • Multiple branches: /cherry-pick branch1 branch2 branch3
  • /cherry-pick-retry <branch> - Retry a failed cherry-pick (merged PRs only)

Branch Management

  • /rebase - Rebase this PR branch onto its base branch

Label Management

  • /<label-name> - Add a label to the PR
  • /<label-name> cancel - Remove a label from the PR

✅ Merge Requirements

This PR will be automatically approved when the following conditions are met:

  1. Approval: /approve from at least one approver
  2. Status Checks: All required status checks must pass
  3. No Blockers: No wip, hold, has-conflicts labels and PR must be mergeable (no conflicts)
  4. Verified: PR must be marked as verified

📊 Review Process

Approvers and Reviewers

Approvers:

  • krcmarik
  • myakove
  • solenoci

Reviewers:

  • krcmarik
  • myakove
  • solenoci
Available Labels
  • hold
  • verified
  • wip
  • lgtm
  • approve
  • automerge
AI Features
  • Conventional Title: Mode: fix (claude/claude-opus-4-6-1m)
  • Cherry-Pick Conflict Resolution: Enabled (claude/claude-opus-4-6-1m)
  • Test Oracle: Triggers: approved (cursor/gpt-5.4-xhigh-fast); /test-oracle can be used anytime
Security Checks
  • Suspicious Path Detection: Monitors paths: .claude/, .vscode/, .cursor/, .devcontainer/, .pi/, .github/workflows/, .github/actions/
  • Committer Identity Check: Verifies last committer matches PR author
  • Mandatory: Security checks block merge (use /security-override to bypass — maintainers only)

💡 Tips

  • WIP Status: Use /wip when your PR is not ready for review
  • Verification: The verified label is removed on new commits unless the push is detected as a clean rebase
  • Cherry-picking: Cherry-pick labels are processed when the PR is merged
  • Container Builds: Container images are automatically tagged with the PR number
  • Permission Levels: Some commands require approver permissions
  • Auto-verified Users: Certain users have automatic verification and merge privileges

For more information, please refer to the project documentation or contact the maintainers.

@rh-bot-1
rh-bot-1 requested a review from solenoci July 8, 2026 13:24
@rh-bot-1
rh-bot-1 requested a review from myakove July 8, 2026 13:24
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix pytest collection skip to stop excluding multi-provider tests with vsphere marker

🐞 Bug fix 🧪 Tests 🕐 Less than 10 minutes

Grey Divider

AI Description

• Stop treating any @pytest.mark.vsphere test as vSphere-only during collection
• Rely on pytest -m marker expressions to select provider-specific test runs
• Prevent accidental skipping of multi-provider test classes in non-vSphere runs
Diagram

graph TD
  A["pytest runner"] --> B["conftest.py hook"] --> C["collected items"] --> D{Check markers}
  D -->|"copyoffload/shared_disk/deep_inspection"| E["add vsphere_only_skip"]
  D -->|"vsphere marker"| F["no longer skipped"]
  F --> G["pytest -m filter"]
  subgraph Legend
    direction LR
    _proc["Process/Step"] ~~~ _dec{"Decision"} ~~~ _out["Outcome"]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Introduce an explicit vsphere_only marker
  • ➕ Eliminates ambiguity between 'supports vsphere' and 'requires vsphere'
  • ➕ Simplifies collection-time skip logic and reduces accidental exclusions
  • ➖ Requires updating existing tests/markers across the suite
  • ➖ Needs agreement/enforcement to prevent marker drift
2. Use a provider capability map (marker → providers)
  • ➕ Centralizes provider support rules and makes intent explicit
  • ➕ Scales better as more providers/capabilities are added
  • ➖ More code and maintenance than the current minimal fix
  • ➖ Risk of mismatches with existing -m workflows if not designed carefully

Recommendation: The PR’s approach is the right minimal fix: removing the blanket 'vsphere' keyword skip restores intended behavior while keeping provider selection driven by pytest’s -m expression. If marker ambiguity continues to cause issues, consider adding a dedicated vsphere_only marker (or a capability map) to clearly separate 'multi-provider' from 'vSphere-only' semantics.

Files changed (1) +0 / -1

Bug fix (1) +0 / -1
conftest.pyStop skipping tests solely due to presence of the vsphere marker +0/-1

Stop skipping tests solely due to presence of the vsphere marker

• Removes the 'vsphere' keyword from the collection-time condition that adds the vSphere-only skip marker. This allows multi-provider tests that include @pytest.mark.vsphere to remain eligible for execution and be correctly selected via pytest -m filtering.

conftest.py

@qodo-code-review

qodo-code-review Bot commented Jul 8, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 94 rules

Grey Divider


Action required

1. vSphere-only tests not skipped ✗ Dismissed 🐞 Bug ≡ Correctness
Description
In pytest_collection_modifyitems, non-vSphere runs no longer skip tests that are marked only with
@pytest.mark.vsphere, so selecting generic categories like -m tier1 or -m upgrade can execute
vSphere-only suites on RHV/OpenStack/… providers and fail at setup/runtime. For example, the LUKS
tier1 tests require luks_passphrase from provider config and will raise when it is missing on
non-vSphere provider entries.
Code

conftest.py[R331-335]

                        "copyoffload" in item.keywords
                        or "shared_disk" in item.keywords
                        or "deep_inspection" in item.keywords
-                        or "vsphere" in item.keywords
                    ):
                        item.add_marker(vsphere_only_skip)
Relevance

⭐⭐⭐ High

Repo relies on collection-time provider gating via vsphere keyword; used to skip non-vSphere runs
(PR #574, #323).

PR-#574
PR-#323

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The collection-time skip logic for non-vSphere providers no longer accounts for
@pytest.mark.vsphere, while the documented execution model encourages running -m tier1/`-m
upgrade with non-vSphere provider keys; there are tests marked only vsphere` that will then be
selected and can fail due to missing provider-specific config (e.g., LUKS passphrase).

conftest.py[326-335]
README.md[255-277]
tests/luks/test_luks_cold_migration.py[161-172]
tests/luks/conftest.py[49-62]
tests/upgrade/test_upgrade_migration.py[37-40]
PR-#323

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Non-vSphere runs currently only skip items with `copyoffload`, `shared_disk`, or `deep_inspection` keywords; after removing the blanket `vsphere` keyword check, tests that are *truly* vSphere-only (marked `@pytest.mark.vsphere` and **not** marked for any other provider) can now run on non-vSphere provider types when users select generic categories like `-m tier1` or `-m upgrade`.

## Issue Context
Provider selection in docs and CLI is typically via `--tc=source_provider:<key>` plus a **category marker** (tier0/tier1/warm/upgrade/...), not via `-m <provider>`.

## Fix Focus Areas
- conftest.py[326-335]

### Concrete fix
Inside the `if source_provider_type != Provider.ProviderType.VSPHERE:` block, update the skip condition to skip **only** vSphere-only tests, e.g.:
- If `"vsphere" in item.keywords` AND none of the other provider markers are present (e.g. `rhv`, `openstack`, `openshift`, `ova`, `esxi`), then add `vsphere_only_skip`.
- Keep the existing vSphere-only feature skips (`copyoffload`, `shared_disk`, `deep_inspection`) as-is.

This preserves the PR’s intent (multi-provider tests that include `vsphere` + other provider markers will not be skipped) while preventing vSphere-only tests from running on non-vSphere providers.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

2. Stale vsphere marker meaning ✗ Dismissed 🐞 Bug ⚙ Maintainability ⭐ New
Description
@pytest.mark.vsphere is now used on multi-provider test classes (alongside
rhv/openstack/openshift), but repo docs and marker descriptions still define vsphere as
provider-specific/vSphere-only. This can confuse contributors and anyone using -m vsphere
expecting it to mean “vSphere-only tests”.
Code

conftest.py[R331-335]

                        "copyoffload" in item.keywords
                        or "shared_disk" in item.keywords
                        or "deep_inspection" in item.keywords
-                        or "vsphere" in item.keywords
                    ):
                        item.add_marker(vsphere_only_skip)
Relevance

⭐⭐⭐ High

Team accepted updating marker docs when vsphere marker usage changed (PR #562); provider markers
semantics introduced in PR #454.

PR-#562
PR-#454

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The PR removes the non-vSphere collection-time skip trigger for the vsphere keyword, and the
codebase contains test classes marked with vsphere plus other provider markers, while the marker
is still documented as provider-specific/vSphere-only.

conftest.py[326-335]
pytest.ini[16-34]
AGENTS.md[983-1006]
tests/cold/test_cold_migration_comprehensive.py[26-35]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The suite now allows `@pytest.mark.vsphere` to appear on multi-provider tests (because `pytest_collection_modifyitems` no longer blanket-skips `vsphere`-marked tests on non-vSphere runs). However, `pytest.ini` and contributor docs still describe `vsphere` as “vSphere provider-specific tests”, which no longer matches usage.

### Issue Context
This is documentation/semantics drift: the marker is being used as a *capability/supported-provider* label in some places, not strictly as a *vSphere-only* label.

### Fix Focus Areas
- pytest.ini[16-35]
- AGENTS.md[983-1006]
- (optional) docs pages mentioning provider markers

### Suggested fix
- Update the `vsphere` marker description to reflect that it may be combined with other provider markers (e.g., “tests that support vSphere as a source provider”).
- If the project still needs an explicit “vSphere-only” semantic for selection, consider introducing a dedicated marker (e.g., `vsphere_only`) and document when to use it.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Removed vsphere skip without comment 📘 Rule violation ☼ Reliability
Description
The pytest_collection_modifyitems hook no longer checks "vsphere" in item.keywords when applying
the non-vSphere skip marker, but the change is not justified with an in-code comment near the
modified logic. This weakens a test gating safeguard without the required colocated rationale,
making future maintenance/audits harder.
Code

conftest.py[R331-335]

                        "copyoffload" in item.keywords
                        or "shared_disk" in item.keywords
                        or "deep_inspection" in item.keywords
-                        or "vsphere" in item.keywords
                    ):
                        item.add_marker(vsphere_only_skip)
Relevance

⭐ Low

Team often rejects “add inline justification comment” requests (e.g., Any-justification rejected in
PR #572).

PR-#572

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The hook explicitly performs provider-based skipping (“Skip vSphere-only tests...”), but the current
condition only checks copyoffload/shared_disk/deep_inspection markers and contains no nearby
justification for excluding the vsphere keyword from this gating logic, which is a weakening of
the prior safeguard without the required in-code rationale.

Rule 1261398: Do not remove or weaken validation/health/error-handling logic without in-code justification
conftest.py[326-335]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A safeguard/gating condition inside `pytest_collection_modifyitems` was weakened by removing the `"vsphere" in item.keywords` clause, but there is no colocated comment explaining why this is correct now.

## Issue Context
Compliance requires an in-code justification (within ~5 lines) whenever validation/health/error-handling or similar safeguard logic is removed or weakened.

## Fix Focus Areas
- conftest.py[326-335]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Previous review results

Review updated until commit 02aa8fa

Results up to commit b3e72be


🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Action required
1. vSphere-only tests not skipped ✗ Dismissed 🐞 Bug ≡ Correctness
Description
In pytest_collection_modifyitems, non-vSphere runs no longer skip tests that are marked only with
@pytest.mark.vsphere, so selecting generic categories like -m tier1 or -m upgrade can execute
vSphere-only suites on RHV/OpenStack/… providers and fail at setup/runtime. For example, the LUKS
tier1 tests require luks_passphrase from provider config and will raise when it is missing on
non-vSphere provider entries.
Code

conftest.py[R331-335]

                        "copyoffload" in item.keywords
                        or "shared_disk" in item.keywords
                        or "deep_inspection" in item.keywords
-                        or "vsphere" in item.keywords
                    ):
                        item.add_marker(vsphere_only_skip)
Relevance

⭐⭐⭐ High

Repo relies on collection-time provider gating via vsphere keyword; used to skip non-vSphere runs
(PR #574, #323).

PR-#574
PR-#323

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The collection-time skip logic for non-vSphere providers no longer accounts for
@pytest.mark.vsphere, while the documented execution model encourages running -m tier1/`-m
upgrade with non-vSphere provider keys; there are tests marked only vsphere` that will then be
selected and can fail due to missing provider-specific config (e.g., LUKS passphrase).

conftest.py[326-335]
README.md[255-277]
tests/luks/test_luks_cold_migration.py[161-172]
tests/luks/conftest.py[49-62]
tests/upgrade/test_upgrade_migration.py[37-40]
PR-#323

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Non-vSphere runs currently only skip items with `copyoffload`, `shared_disk`, or `deep_inspection` keywords; after removing the blanket `vsphere` keyword check, tests that are *truly* vSphere-only (marked `@pytest.mark.vsphere` and **not** marked for any other provider) can now run on non-vSphere provider types when users select generic categories like `-m tier1` or `-m upgrade`.

## Issue Context
Provider selection in docs and CLI is typically via `--tc=source_provider:<key>` plus a **category marker** (tier0/tier1/warm/upgrade/...), not via `-m <provider>`.

## Fix Focus Areas
- conftest.py[326-335]

### Concrete fix
Inside the `if source_provider_type != Provider.ProviderType.VSPHERE:` block, update the skip condition to skip **only** vSphere-only tests, e.g.:
- If `"vsphere" in item.keywords` AND none of the other provider markers are present (e.g. `rhv`, `openstack`, `openshift`, `ova`, `esxi`), then add `vsphere_only_skip`.
- Keep the existing vSphere-only feature skips (`copyoffload`, `shared_disk`, `deep_inspection`) as-is.

This preserves the PR’s intent (multi-provider tests that include `vsphere` + other provider markers will not be skipped) while preventing vSphere-only tests from running on non-vSphere providers.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational
2. Removed vsphere skip without comment 📘 Rule violation ☼ Reliability
Description
The pytest_collection_modifyitems hook no longer checks "vsphere" in item.keywords when applying
the non-vSphere skip marker, but the change is not justified with an in-code comment near the
modified logic. This weakens a test gating safeguard without the required colocated rationale,
making future maintenance/audits harder.
Code

conftest.py[R331-335]

                        "copyoffload" in item.keywords
                        or "shared_disk" in item.keywords
                        or "deep_inspection" in item.keywords
-                        or "vsphere" in item.keywords
                    ):
                        item.add_marker(vsphere_only_skip)
Relevance

⭐ Low

Team often rejects “add inline justification comment” requests (e.g., Any-justification rejected in
PR #572).

PR-#572

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The hook explicitly performs provider-based skipping (“Skip vSphere-only tests...”), but the current
condition only checks copyoffload/shared_disk/deep_inspection markers and contains no nearby
justification for excluding the vsphere keyword from this gating logic, which is a weakening of
the prior safeguard without the required in-code rationale.

Rule 1261398: Do not remove or weaken validation/health/error-handling logic without in-code justification
conftest.py[326-335]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A safeguard/gating condition inside `pytest_collection_modifyitems` was weakened by removing the `"vsphere" in item.keywords` clause, but there is no colocated comment explaining why this is correct now.

## Issue Context
Compliance requires an in-code justification (within ~5 lines) whenever validation/health/error-handling or similar safeguard logic is removed or weakened.

## Fix Focus Areas
- conftest.py[326-335]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Qodo Logo

Comment thread conftest.py Resolved
@redhat-qe-bot

Copy link
Copy Markdown

Clean rebase detected — no code changes compared to previous head (b3e72be).
The following labels were preserved: commented-krcmarik, lgtm-coderabbitai[bot], commented-qodo-code-review[bot].

Comment thread conftest.py Resolved
@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 02aa8fa

@krcmarik

krcmarik commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

/verified

@krcmarik

krcmarik commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

/approve
/lgtm

@krcmarik
krcmarik merged commit 06c741c into RedHatQE:main Jul 8, 2026
9 checks passed
@redhat-qe-bot

Copy link
Copy Markdown

New container for ghcr.io/redhatqe/mtv-api-tests:latest published

@krcmarik
krcmarik deleted the fix/vsphere-skip-logic branch September 15, 2026 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants