Skip to content

fix(test): add missing pytest markers to video/audio protocol tests#8849

Merged
dmitry-tokarev-nv merged 2 commits into
mainfrom
kprashanth/fix-precommit-markers
Apr 29, 2026
Merged

fix(test): add missing pytest markers to video/audio protocol tests#8849
dmitry-tokarev-nv merged 2 commits into
mainfrom
kprashanth/fix-precommit-markers

Conversation

@KrishnanPrash
Copy link
Copy Markdown
Contributor

@KrishnanPrash KrishnanPrash commented Apr 29, 2026

Overview

The video streaming PR (#8491) added 3 test files to components/src/ right after the marker enforcement PR (#7796) expanded the pre-commit check to that directory. Since the new tests didn't include the required markers, every PR's pre-commit hook started failing — 49 tests flagged as missing Lifecycle, Test Type, and Hardware markers.

This adds the standard pytestmark block to those 3 files so pre-commit passes again.

Repro:

python3 tests/report_pytest_markers.py
# Before: Missing sets: 49 (exit 1)
# After:  Missing sets: 0

Details

The marker enforcement hook (pytest-marker-report) requires every test to carry at least one marker from each of three categories: Lifecycle (pre_merge, post_merge, etc.), Test Type (unit, integration, etc.), and Hardware (gpu_0, gpu_1, etc.).

The video streaming PR landed ~40 minutes after the enforcement expansion, and its tests didn't have any markers. Since the hook scans all files (not just changed ones), this broke pre-commit for every subsequent PR.

Files fixed:

  • components/src/dynamo/common/tests/test_audio_protocol.py
  • components/src/dynamo/common/tests/test_video_protocol.py
  • components/src/dynamo/common/tests/test_video_utils.py

All three are pure pydantic/protocol validation tests — no GPU, no external services — so unit, gpu_0, pre_merge is the right fit, matching adjacent files in the same directory.

Where should the reviewer start?

Any of the 3 files — the change is identical in each: a pytestmark list after the imports.

Test Plan

  • Ran python3 tests/report_pytest_markers.py on main → Missing sets: 49
  • Ran same after fix → Missing sets: 0
  • Checked the enforcement commit (before video streaming landed): Missing sets: 0 — confirms the video streaming PR is the source
  • Checked the video streaming commit: Missing sets: 49 — confirms root cause

Related Issues

🤖 Generated with Claude Code


Open in Devin Review

Summary by CodeRabbit

  • Tests
    • Added global test markers to three test modules (audio protocol, video protocol, and video utilities tests)
    • Tests now tagged with unit, gpu_0, and pre_merge classification

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@KrishnanPrash KrishnanPrash requested review from a team as code owners April 29, 2026 19:22
@github-actions github-actions Bot added the fix label Apr 29, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 29, 2026

Walkthrough

Three test modules in the dynamo/common/tests directory now declare a module-level pytestmark variable with unit, gpu_0, and pre_merge markers. This ensures all tests within these modules inherit these markers during test collection and execution. No test logic or assertions were modified.

Changes

Cohort / File(s) Summary
Pytest markers configuration
components/src/dynamo/common/tests/test_audio_protocol.py, components/src/dynamo/common/tests/test_video_protocol.py, components/src/dynamo/common/tests/test_video_utils.py
Added module-level pytestmark variable with three pytest markers (unit, gpu_0, pre_merge) to apply globally to all tests in each module. No test logic changes.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: adding missing pytest markers to test files related to video and audio protocols.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.
Description check ✅ Passed The pull request description is comprehensive and well-structured, following the template with clear Overview, Details, Where to Start, and Related Issues sections.

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


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 and usage tips.

@KrishnanPrash KrishnanPrash enabled auto-merge (squash) April 29, 2026 19:27
…io is installed

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@dmitry-tokarev-nv dmitry-tokarev-nv merged commit cc1af6a into main Apr 29, 2026
78 of 80 checks passed
@dmitry-tokarev-nv dmitry-tokarev-nv deleted the kprashanth/fix-precommit-markers branch April 29, 2026 20:59
furionw pushed a commit that referenced this pull request May 2, 2026
…8849)

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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