Skip to content

fix: remove overlaps between tier1/2 and smoke and mark tests as tier2 by default#1302

Merged
dbasunag merged 2 commits intoopendatahub-io:mainfrom
dbasunag:tier2_marker
Mar 26, 2026
Merged

fix: remove overlaps between tier1/2 and smoke and mark tests as tier2 by default#1302
dbasunag merged 2 commits intoopendatahub-io:mainfrom
dbasunag:tier2_marker

Conversation

@dbasunag
Copy link
Copy Markdown
Collaborator

@dbasunag dbasunag commented Mar 25, 2026

Pull Request

Summary

Related Issues

  • Fixes:
  • JIRA:

Please review and indicate how it has been tested

  • Locally
  • Jenkins

Additional Requirements

  • If this PR introduces a new test image, did you create a PR to mirror it in disconnected environment?
  • If this PR introduces new marker(s)/adds a new component, was relevant ticket created to update relevant Jenkins job?

Summary by CodeRabbit

  • Chores
    • Updated test infrastructure and categorization processes for internal testing workflows.

…2 by default

Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com>

Co-Authored-By: Claude <noreply@anthropic.com>
@dbasunag dbasunag requested review from a team, fege and lugi0 as code owners March 25, 2026 22:15
@github-actions
Copy link
Copy Markdown

The following are automatically added/executed:

  • PR size label.
  • Run pre-commit
  • Run tox
  • Add PR author as the PR assignee
  • Build image based on the PR

Available user actions:

  • To mark a PR as WIP, add /wip in a comment. To remove it from the PR comment /wip cancel to the PR.
  • To block merging of a PR, add /hold in a comment. To un-block merging of PR comment /hold cancel.
  • To mark a PR as approved, add /lgtm in a comment. To remove, add /lgtm cancel.
    lgtm label removed on each new commit push.
  • To mark PR as verified comment /verified to the PR, to un-verify comment /verified cancel to the PR.
    verified label removed on each new commit push.
  • To Cherry-pick a merged PR /cherry-pick <target_branch_name> to the PR. If <target_branch_name> is valid,
    and the current PR is merged, a cherry-picked PR would be created and linked to the current PR.
  • To build and push image to quay, add /build-push-pr-image in a comment. This would create an image with tag
    pr-<pr_number> to quay repository. This image tag, however would be deleted on PR merge or close action.
Supported labels

{'/lgtm', '/cherry-pick', '/wip', '/hold', '/verified', '/build-push-pr-image'}

@dbasunag dbasunag changed the title fix: remove overlaps between tier1/2 and smoke and mark tests as tier… fix: remove overlaps between tier1/2 and smoke and mark tests as tier2 by default Mar 25, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 25, 2026

📝 Walkthrough

Walkthrough

Adds automatic tier2 marker assignment during pytest collection for tests in specific paths (model_registry, model_explainability, model_serving/maas_billing), unless already marked with smoke/tier1/tier2/tier3/pre_upgrade/post_upgrade. Updates test parametrization mark syntax and removes explicit tier2 decorators from test functions.

Changes

Cohort / File(s) Summary
Pytest Configuration
conftest.py
Added tier2 auto-marker logic with path-based scoping via _add_default_tier2_marker(), excludes tests already bearing smoke/tier1/tier2/tier3/pre_upgrade/post_upgrade markers. Introduces DEFAULT_TIER2_MARKER_TEST_PATHS and EXCLUDE_MARKERS_FROM_DEFAULT_TIER2 module constants.
Test Markers Refactoring
tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py
Updated pytest.param marks from tuple to direct marker syntax; removed explicit @pytest.mark.tier2 decorators from 8 test functions, now relying on auto-marker logic; modified parametrization mark assignment for postgres case from tier2 to smoke.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Issues & Observations

Path matching fragility: The startswith() check against DEFAULT_TIER2_MARKER_TEST_PATHS lacks delimiter boundaries. A test path like tests/model_registry_extended/... will incorrectly match "tests/model_registry". Use path-normalized matching (e.g., split by / and compare segments, or anchor with /).

Implicit marker reassignment: Removing explicit @pytest.mark.tier2 decorators and delegating to auto-marker logic creates subtle behavioral dependencies. If DEFAULT_TIER2_MARKER_TEST_PATHS is later modified or the exclusion list changes, test categorization silently shifts. Document the deprecation or provide a transition window.

Marker precedence unclear: The logic applies tier2 only if no marker in EXCLUDE_MARKERS_FROM_DEFAULT_TIER2 exists. Verify this doesn't create test selection gaps (e.g., an undecorated test in model_serving/maas_billing that should be tier2 but accidentally inherits a pre_upgrade marker from inheritance).

🚥 Pre-merge checks | ✅ 1 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is a template with all sections empty or unchecked; no actual summary, related issues, or testing information is provided. Fill in the Summary section with specific details about marker changes and default tier2 logic. Add related JIRA/issue links. Confirm testing approach (Local/Jenkins). Address additional requirements about Jenkins job updates for new markers.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: removing marker overlaps between tier1/2 and smoke, and adding default tier2 marking to specific test paths.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@conftest.py`:
- Around line 304-305: The prefix check using any(item.nodeid.startswith(path)
for path in DEFAULT_TIER2_MARKER_TEST_PATHS) can false-match substrings (e.g.,
"tests/model_registry_extra/..."); update the logic in conftest.py to extract
the test file path from item.nodeid (split off any "::" node part), normalize
both the test path and each candidate in DEFAULT_TIER2_MARKER_TEST_PATHS with
pathlib.Path (or os.path.normpath), and compare using directory-boundary-safe
checks (e.g., compare Path.parts prefixes or use Path.relative_to in a
try/except) so only true path-prefix matches are tagged tier2.

ℹ️ Review info
⚙️ Run configuration

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

Review profile: CHILL

Plan: Pro

Run ID: fd16c1d9-f91f-4370-b9f0-3a7d0467d485

📥 Commits

Reviewing files that changed from the base of the PR and between 8a67a44 and 0c22bb4.

📒 Files selected for processing (2)
  • conftest.py
  • tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py

Comment thread conftest.py
@fege
Copy link
Copy Markdown
Contributor

fege commented Mar 26, 2026

@dbasunag did you verified with --collect-only if -m smoke and -m tier1 for example are counting correctly and we do not need anymore -m 'smoke and not tier1 and not tier3' and -m 'tier1 and not smoke and not tier3'

Copy link
Copy Markdown
Contributor

@fege fege left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Copy Markdown
Contributor

@kpunwatk kpunwatk left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Copy Markdown
Contributor

@sheltoncyril sheltoncyril left a comment

Choose a reason for hiding this comment

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

/lgtm

@dbasunag dbasunag merged commit 301abb3 into opendatahub-io:main Mar 26, 2026
10 checks passed
@dbasunag dbasunag deleted the tier2_marker branch March 26, 2026 12:15
@github-actions
Copy link
Copy Markdown

Status of building tag latest: success.
Status of pushing tag latest to image registry: success.

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.

6 participants