Repository navigation
fix(e2e): resolve marker MRO mismatch and add missing model marker - #569
Conversation
📝 WalkthroughWalkthroughAdds an MRO-aware pytest marker resolver and updates fixtures to use it; adjusts marker-related helper signatures; annotates two test classes with a model marker; increases token limits and adds skipped Harmony-related test stubs in chat completion tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors pytest marker resolution logic to correctly handle class inheritance, ensuring that markers defined on child classes take precedence over those on parent classes. This enhancement centralizes marker resolution into a new utility function, Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a helpful refactoring to centralize MRO-aware marker resolution for pytest, improving code clarity and maintainability. The new resolve_class_marker function correctly handles marker precedence in class inheritance. My review includes a fix for a potential bug in this new function that could affect keyword-argument-only markers, and a minor performance improvement suggestion regarding an import statement placement.
key4ng
changed the title
key4ng
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
key4ng
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/fixtures/markers.py`:
- Around line 25-32: The condition that checks "and marker.args" inside the loop
that inspects class-level pytest marks prevents detecting kwargs-only markers;
update the logic in the function that iterates item.cls.__mro__ (the block that
builds "markers" from cls.pytestmark and checks "if marker.name == marker_name
and marker.args") to remove the "and marker.args" clause so markers with only
kwargs (e.g., `@pytest.mark.workers`(count=1)) are returned; keep the rest of the
behavior the same (get_marker_value still guards on marker.args elsewhere).
In `@e2e_test/responses/test_builtin_tools.py`:
- Around line 624-626: Add the same pytest model marker to the
TestWebSearchStreamingEventsGrpc class: above the class definition for
TestWebSearchStreamingEventsGrpc, add `@pytest.mark.model`("openai/gpt-oss-20b")
(matching the marker used on TestBuiltinToolRoutingGrpc) so that tests using the
gateway_with_mcp_config_grpc fixture are pre-scanned with the correct model;
ensure the decorator is applied directly above the class definition.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
e2e_test/fixtures/hooks.pye2e_test/fixtures/markers.pye2e_test/responses/test_builtin_tools.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
e2e_test/responses/test_builtin_tools.py (1)
695-696:⚠️ Potential issue | 🟠 Major
TestWebSearchStreamingEventsGrpcalso needs the model marker.This class uses the same
gateway_with_mcp_config_grpcfixture (which requests"openai/gpt-oss-20b"from the model pool) but lacks the@pytest.mark.modelmarker. Without it, the model won't be pre-scanned during collection, causing the same issue this PR aims to fix.🔧 Proposed fix
`@pytest.mark.e2e` +@pytest.mark.model("openai/gpt-oss-20b") class TestWebSearchStreamingEventsGrpc:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/responses/test_builtin_tools.py` around lines 695 - 696, Add the pytest model marker to the TestWebSearchStreamingEventsGrpc test class so the model is pre-scanned during collection; specifically, annotate the class with the same pytest.mark.model marker used elsewhere (the model name requested by the gateway_with_mcp_config_grpc fixture, e.g., "openai/gpt-oss-20b") so tests using the gateway_with_mcp_config_grpc fixture get the model pre-scanned.e2e_test/fixtures/markers.py (1)
25-32:⚠️ Potential issue | 🟠 MajorThe
and marker.argscondition breaks kwargs-only marker resolution.The condition on line 30 filters out markers that have no positional arguments, which breaks
get_marker_kwargs()for kwargs-only markers like@pytest.mark.workers(count=1)and@pytest.mark.gateway(policy="round_robin"). Sinceget_marker_value()already guards onmarker.argsat line 58, this condition is redundant for that caller and harmful forget_marker_kwargs().🔧 Proposed fix
if hasattr(item, "cls") and item.cls is not None: for cls in item.cls.__mro__: if hasattr(cls, "pytestmark"): markers = cls.pytestmark if isinstance(cls.pytestmark, list) else [cls.pytestmark] for marker in markers: - if marker.name == marker_name and marker.args: + if marker.name == marker_name: return marker return item.get_closest_marker(marker_name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/fixtures/markers.py` around lines 25 - 32, The check "and marker.args" in the class-mark scanning block inside markers.py incorrectly filters out kwargs-only pytest markers; remove that condition so the loop returns markers regardless of positional args (i.e., change the conditional that currently reads marker.name == marker_name and marker.args to only check marker.name == marker_name). This preserves behavior for get_marker_kwargs() while get_marker_value() can still guard on marker.args where needed; update the logic around the class-level pytestmark handling in the same function so markers with only keyword args (e.g., `@pytest.mark.workers`(count=1)) are returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/fixtures/hooks.py`:
- Around line 135-139: The import of resolve_class_marker is inside the loop;
move "from .markers import resolve_class_marker" out of the loop (either to
module top or just before iterating over items_to_scan) and remove the in-loop
import, then keep using resolve_class_marker(item, PARAM_MODEL) and
model_marker.args access as-is so the behavior with PARAM_MODEL and model_marker
remains unchanged.
---
Duplicate comments:
In `@e2e_test/fixtures/markers.py`:
- Around line 25-32: The check "and marker.args" in the class-mark scanning
block inside markers.py incorrectly filters out kwargs-only pytest markers;
remove that condition so the loop returns markers regardless of positional args
(i.e., change the conditional that currently reads marker.name == marker_name
and marker.args to only check marker.name == marker_name). This preserves
behavior for get_marker_kwargs() while get_marker_value() can still guard on
marker.args where needed; update the logic around the class-level pytestmark
handling in the same function so markers with only keyword args (e.g.,
`@pytest.mark.workers`(count=1)) are returned.
In `@e2e_test/responses/test_builtin_tools.py`:
- Around line 695-696: Add the pytest model marker to the
TestWebSearchStreamingEventsGrpc test class so the model is pre-scanned during
collection; specifically, annotate the class with the same pytest.mark.model
marker used elsewhere (the model name requested by the
gateway_with_mcp_config_grpc fixture, e.g., "openai/gpt-oss-20b") so tests using
the gateway_with_mcp_config_grpc fixture get the model pre-scanned.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
e2e_test/fixtures/hooks.pye2e_test/fixtures/markers.pye2e_test/responses/test_builtin_tools.py
github-actions
Bot
added
grpc
653a386 to
4fd6780
Compare
github-actions
Bot
removed
grpc
…ytest - Introduced `resolve_class_marker` function to improve MRO-aware marker resolution, ensuring child class markers take precedence over parent class markers. - Updated `get_marker_value` and `get_marker_kwargs` to utilize the new resolution method. - Simplified marker extraction logic in `pytest_collection_modifyitems` to enhance readability and maintainability. - Added model marker to `TestBuiltinToolRoutingGrpc` for integration testing. Signed-off-by: key4ng <rukeyang@gmail.com>
…yParserAdapter - Added `extract_stop_string` method to retrieve stop strings from matched_stop JSON values. - Introduced `trim_stop_sequence` method to remove trailing stop sequences from text outputs. - Integrated stop string trimming in both `parse_complete` and `parse_incomplete` methods to ensure clean output. Signed-off-by: [Your Name] [Your Email] Signed-off-by: key4ng <rukeyang@gmail.com>
…tions - Implemented tests for handling stop sequences in both standard and streaming responses. - Verified that stop sequences do not appear in the output content. - Added a test to ensure streaming token count matches the number of content chunks, accounting for Harmony's channel markers. Signed-off-by: key4ng <rukeyang@gmail.com>
- Revised test descriptions to clarify the behavior of stop sequences and token consumption in Harmony model. - Increased `max_tokens` in tests to ensure proper output generation before token limits are reached. - Adjusted assertions to account for Harmony's channel markers and their impact on token counts. Signed-off-by: key4ng <rukeyang@gmail.com>
…from HarmonyParserAdapter - Deleted `extract_stop_string` and `trim_stop_sequence` methods as they are no longer needed. - Removed associated calls to these methods in `parse_complete` and `parse_incomplete` to streamline the parsing process. Signed-off-by: key4ng <rukeyang@gmail.com>
…mony model - Updated `max_tokens` from 50 to 200 in multiple test cases to ensure adequate output generation. - Removed obsolete test methods related to stop sequences, streamlining the test suite for Harmony model. Signed-off-by: key4ng <rukeyang@gmail.com>
…y model - Updated `max_tokens` from 200 to 1024 in chat completion tests to enhance output capacity. - Added new skipped test methods for stop sequences and streaming token count to address current limitations in Harmony model. Signed-off-by: key4ng <rukeyang@gmail.com>
…ests
- Refactored `resolve_class_marker` function to simplify marker extraction logic in `pytest_collection_modifyitems`.
- Removed redundant import of `resolve_class_marker` within the function.
- Added model marker `@pytest.mark.model("openai/gpt-oss-20b")` to `TestWebSearchStreamingEventsGrpc` for enhanced test coverage.
Signed-off-by: key4ng <rukeyang@gmail.com>
… sequences - Reduced `max_tokens` from 1024 to 200 in chat completion tests to align with current output requirements. - Added a skipped test method for streaming stop sequences to address limitations in the Harmony model. Signed-off-by: key4ng <rukeyang@gmail.com>
f1a5fc2 to
976ad32
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e_test/chat_completions/test_openai_server.py (1)
346-367: 🧹 Nitpick | 🔵 TrivialKeep
max_tokenssymmetric between client and SmgClient in stop-sequence comparisons.Line 346/Line 388 were increased, but comparison calls still use
max_tokens=50(Line 365 and Line 417). This introduces a second variable (different truncation budget) and makes parity checks noisier.♻️ Suggested alignment
with smg_compare(): smg_resp = smg.chat.completions.create( @@ - max_tokens=50, + max_tokens=200, stop=[","], ) @@ with smg_compare(): with smg.chat.completions.create( @@ - max_tokens=50, + max_tokens=1024, stop=[","], stream=True, ) as stream:Also applies to: 388-418
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/chat_completions/test_openai_server.py` around lines 346 - 367, The test uses max_tokens=200 for the primary client call but the SmgClient calls (smg.chat.completions.create producing smg_resp) still use max_tokens=50, causing inconsistent truncation during the stop-sequence parity check; update the two smg.chat.completions.create invocations to use the same max_tokens value (200) as the corresponding primary client call so the stop-sequence comparison between response and smg_resp is symmetric.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/fixtures/markers.py`:
- Around line 25-32: The resolver currently returns a class-level marker found
via item.cls.__mro__ before checking function-level markers, causing per-test
overrides to be ignored; change the lookup order in the marker resolver so you
first call item.get_closest_marker(marker_name) and if it returns a marker
return it immediately, otherwise fall back to iterating item.cls.__mro__
(preserving the existing pytestmark list handling and marker.name checks); this
preserves function-level precedence and fixes downstream callers
get_marker_value, get_marker_kwargs and the behavior used in
e2e_test/fixtures/hooks.py.
---
Outside diff comments:
In `@e2e_test/chat_completions/test_openai_server.py`:
- Around line 346-367: The test uses max_tokens=200 for the primary client call
but the SmgClient calls (smg.chat.completions.create producing smg_resp) still
use max_tokens=50, causing inconsistent truncation during the stop-sequence
parity check; update the two smg.chat.completions.create invocations to use the
same max_tokens value (200) as the corresponding primary client call so the
stop-sequence comparison between response and smg_resp is symmetric.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
e2e_test/chat_completions/test_openai_server.pye2e_test/fixtures/hooks.pye2e_test/fixtures/markers.pye2e_test/responses/test_builtin_tools.py
| if hasattr(item, "cls") and item.cls is not None: | ||
| for cls in item.cls.__mro__: | ||
| if hasattr(cls, "pytestmark"): | ||
| markers = cls.pytestmark if isinstance(cls.pytestmark, list) else [cls.pytestmark] | ||
| for marker in markers: | ||
| if marker.name == marker_name: | ||
| return marker | ||
| return item.get_closest_marker(marker_name) |
There was a problem hiding this comment.
Preserve function-level marker precedence before class MRO lookup.
At Line 25, the resolver returns a class marker before checking marks directly attached to the test item. That prevents per-test overrides (function-level marker over class marker), and the regression propagates to get_marker_value (Line 55), get_marker_kwargs (Line 82), and e2e_test/fixtures/hooks.py (Line 147).
🔧 Proposed fix
def resolve_class_marker(
item: pytest.Item,
marker_name: str,
) -> pytest.Mark | None:
@@
+ # Keep pytest "closest" behavior for marks defined on the test item itself.
+ for marker in reversed(getattr(item, "own_markers", [])):
+ if marker.name == marker_name:
+ return marker
+
if hasattr(item, "cls") and item.cls is not None:
for cls in item.cls.__mro__:
if hasattr(cls, "pytestmark"):
markers = cls.pytestmark if isinstance(cls.pytestmark, list) else [cls.pytestmark]
for marker in markers:
if marker.name == marker_name:
return marker
return item.get_closest_marker(marker_name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e_test/fixtures/markers.py` around lines 25 - 32, The resolver currently
returns a class-level marker found via item.cls.__mro__ before checking
function-level markers, causing per-test overrides to be ignored; change the
lookup order in the marker resolver so you first call
item.get_closest_marker(marker_name) and if it returns a marker return it
immediately, otherwise fall back to iterating item.cls.__mro__ (preserving the
existing pytestmark list handling and marker.name checks); this preserves
function-level precedence and fixes downstream callers get_marker_value,
get_marker_kwargs and the behavior used in e2e_test/fixtures/hooks.py.
There was a problem hiding this comment.
Fixed — resolve_class_marker now checks item.own_markers (function-level) first, then walks class MRO. This preserves per-test override precedence.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current implementation of resolve_class_marker in the PR branch
cat e2e_test/fixtures/markers.py | grep -A 30 "def resolve_class_marker"
Length of output: 1101
🏁 Script executed:
#!/bin/bash
# Check recent commits on this branch to see if there's a newer fix
git log --oneline -5
# Also check if there are any uncommitted changes to the markers.py file
git status e2e_test/fixtures/markers.py
# Show the latest commit for this specific file
git log -1 --oneline -- e2e_test/fixtures/markers.py
Length of output: 593
🏁 Script executed:
#!/bin/bash
# Fetch the latest changes from the remote branch
git fetch origin keyang/fix-oss-test
# Check if there are newer commits
git log origin/keyang/fix-oss-test --oneline -5
# Show the diff of markers.py on the remote branch
git show origin/keyang/fix-oss-test:e2e_test/fixtures/markers.py | grep -A 35 "def resolve_class_marker"
Length of output: 315
🏁 Script executed:
#!/bin/bash
# List all remote branches
git branch -r | head -20
# Check current branch info
git branch -a
# Search for any mentions of own_markers in the codebase
rg "own_markers" --type=py
# Also check if there's a recent change to this function that I might have missed
git log --all --oneline --grep="marker" -10
Length of output: 632
🏁 Script executed:
#!/bin/bash
# Check the merged version on main
git show d4aec6d:e2e_test/fixtures/markers.py | grep -A 40 "def resolve_class_marker"
# Also show what changed in that commit
git show d4aec6d --stat
# And see the diff for markers.py specifically
git show d4aec6d -- e2e_test/fixtures/markers.py | head -80
Length of output: 4836
@key4ng I checked the merged code in commit d4aec6d, and I don't see the item.own_markers check you mentioned. The current implementation still walks the class MRO first before falling back to get_closest_marker:
def resolve_class_marker(
item: pytest.Item,
marker_name: str,
) -> pytest.Mark | None:
if hasattr(item, "cls") and item.cls is not None:
for cls in item.cls.__mro__:
if hasattr(cls, "pytestmark"):
markers = cls.pytestmark if isinstance(cls.pytestmark, list) else [cls.pytestmark]
for marker in markers:
if marker.name == marker_name:
return marker # Returns class marker before checking function-level
return item.get_closest_marker(marker_name)This means function-level markers (via @pytest.mark.model(...) on individual test methods) would still be overridden by class-level markers found in the MRO.
Could you clarify:
- Was the fix applied in a way different from what you described?
- Or should we add the
item.own_markerscheck as originally suggested?
🏁 Script executed:
#!/bin/bash
# Check if own_markers is used anywhere in pytest or if there's an alternative
python3 << 'EOF'
import pytest
import inspect
# Check what attributes are available on pytest.Item
print("Checking pytest.Item attributes related to markers:")
for attr in dir(pytest.Item):
if 'marker' in attr.lower():
print(f" - {attr}")
# Check the signature of get_closest_marker
print("\nget_closest_marker signature:")
print(inspect.signature(pytest.Item.get_closest_marker))
EOF
Length of output: 232
🏁 Script executed:
#!/bin/bash
# Check pytest documentation or source to understand marker resolution order
python3 << 'EOF'
import pytest
# Create a simple test to see what attributes an item has
class TestExample:
pytestmark = pytest.mark.model("class-model")
`@pytest.mark.model`("function-model")
def test_example(self):
pass
# We can't actually run this, but let's check pytest docs/source
# Check if own_markers exists in any pytest version
import sys
print("Searching for marker-related attributes in pytest.Item...")
# Try to instantiate a minimal item to see attributes
try:
from _pytest.nodes import Item
print("\nItem instance attributes (if accessible):")
# Can't easily instantiate without a full pytest session
print(" - Markers are typically accessed via iter_markers() or get_closest_marker()")
print(" - own_markers may be internal/undocumented")
except Exception as e:
print(f"Could not inspect: {e}")
# Check pytest version
print(f"\nPytest version: {pytest.__version__}")
EOF
# Also search the codebase for any usage of own_markers
rg "own_markers" --type=py -C2 || echo "No usage of own_markers found in codebase"
# Search pytest source if available
fd -t f "nodes.py" | xargs grep -l "own_markers" 2>/dev/null || echo "own_markers not found in pytest source files"
Length of output: 333
🏁 Script executed:
#!/bin/bash
# Let's understand how get_closest_marker works and verify the concern
python3 << 'EOF'
import pytest
# Check pytest documentation for get_closest_marker behavior
print("Testing marker resolution order with get_closest_marker:\n")
# Simulate what happens with current implementation
print("Current implementation logic:")
print("1. If item has a class, walk MRO and return first class marker found")
print("2. Only if NO class marker found, call get_closest_marker()\n")
print("Problem scenario:")
print("- Function has `@pytest.mark.model`('function-level')")
print("- Class has `@pytest.mark.model`('class-level')")
print("- Current code returns 'class-level' (step 1 succeeds)")
print("- get_closest_marker() is never called\n")
print("Expected behavior:")
print("- Function-level markers should override class-level markers")
print("- get_closest_marker() natively respects this precedence\n")
# Check if we can verify this
from _pytest.mark.structures import Mark
print(f"Mark class available: {Mark}")
# According to pytest docs, get_closest_marker searches in this order:
# 1. Function markers (own_markers)
# 2. Class markers
# 3. Module markers
# 4. Package markers
print("\nget_closest_marker() precedence (from pytest docs):")
print("1. Function/method → 2. Class → 3. Module → 4. Package")
EOF
# Check if there are any test cases in the codebase that might validate this behavior
rg "pytest.mark.model.*pytest.mark.model" --type=py -C3
Length of output: 735
@key4ng I checked the merged code in commit d4aec6d, and I don't see the item.own_markers check you described. The current implementation still walks the class MRO first:
def resolve_class_marker(
item: pytest.Item,
marker_name: str,
) -> pytest.Mark | None:
if hasattr(item, "cls") and item.cls is not None:
for cls in item.cls.__mro__:
if hasattr(cls, "pytestmark"):
markers = cls.pytestmark if isinstance(cls.pytestmark, list) else [cls.pytestmark]
for marker in markers:
if marker.name == marker_name:
return marker # Returns class marker immediately
return item.get_closest_marker(marker_name) # Only called if no class markerThe concern: This prevents function-level markers from overriding class-level markers.
Example scenario:
`@pytest.mark.model`("class-model")
class TestExample:
`@pytest.mark.model`("function-model") # This override is ignored
def test_something(self):
passWith the current code, resolve_class_marker() returns "class-model" because it finds the class marker first and never checks the function-level marker.
get_closest_marker() natively respects pytest's precedence: function → class → module → package. By checking class MRO first, we bypass this.
Question: Do you actually use function-level marker overrides in your test suite? If not, the current implementation may be fine for your use case. If you do need function-level overrides, we should fix this by checking function markers first (either via item.iter_markers() or by calling get_closest_marker() before the MRO walk).
🧠 Learnings used
Learnt from: key4ng
Repo: lightseekorg/smg PR: 556
File: e2e_test/fixtures/hooks.py:239-245
Timestamp: 2026-02-27T19:34:12.191Z
Learning: When implementing or reviewing affinity tracking in e2e tests, prefer deriving backend grouping from aggregated backends specified by pytest parametrize markers rather than per-item backends derived from callspec.params. This mirrors the worker pre-launch pattern and yields a conservative GPU affinity grouping (all variants share a single group) in cases where parametrizations mix local and cloud variants, reducing risk of missed grouping. Apply this guideline to Python e2e test hooks and fixtures files under e2e_test/ (not just a single file).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 976ad32157
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if hasattr(cls, "pytestmark"): | ||
| markers = cls.pytestmark if isinstance(cls.pytestmark, list) else [cls.pytestmark] | ||
| for marker in markers: | ||
| if marker.name == marker_name: | ||
| return marker |
There was a problem hiding this comment.
resolve_class_marker() returns the first class-level match from item.cls.__mro__ before checking the test function node, so get_marker_value()/get_marker_kwargs() now ignore per-test overrides when a class has the same marker. In practice, a method-level marker like @pytest.mark.model("...") (or workers/gateway) inside a marked class will be silently ignored, which can route that test to the wrong model/backend and make marker behavior diverge from normal pytest precedence for function overrides.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — resolve_class_marker now checks item.own_markers (function-level) first, then walks class MRO. This preserves per-test override precedence.
|
this pr correct the tests using right gpt-oss model. It causes 3 tests failure:
First two failure among all 3 backends, the 3rd one is for 1 backend. These should be fixed in following PR |
2 participants
Description
Problem
Two bugs in the E2E test infrastructure cause tests to silently run against wrong models:
Marker resolution mismatch:
get_marker_value()andget_marker_kwargs()useget_closest_marker()which doesn't respect class MRO for inherited markers. The hooks layer walks MRO correctly during collection, but the fixture layer doesn't — so collection-time scanning sees the child's model while runtime fixtures resolve to the parent's model. This affectsTestChatCompletionGptOss(16 tests running on Llama-8B instead of gpt-oss-20b).Missing model marker:
TestBuiltinToolRoutingGrpclacks@pytest.mark.model(...), so its model isn't pre-scanned during collection. It only works by accident when another test class launches the same model.Solution
Add MRO-aware marker resolution to the fixture layer (matching what hooks already did), extract the shared logic into a reusable helper, and add the missing marker.
Changes
e2e_test/fixtures/markers.py: Addresolve_class_marker()helper that walkscls.__mro__child-first before falling back toget_closest_marker(). Updateget_marker_value()andget_marker_kwargs()to use it instead ofget_closest_marker()directly.e2e_test/fixtures/hooks.py: Replace 15-line inline MRO walk inpytest_collection_modifyitemswith a 2-line call toresolve_class_marker(), eliminating code duplication.e2e_test/responses/test_builtin_tools.py: Add@pytest.mark.model("openai/gpt-oss-20b")toTestBuiltinToolRoutingGrpc.Test Plan
ruff check,ruff format --check, andmypyall pass on the changed files._TestToolChoiceBasechildren are fine since the base has no markers).Summary by CodeRabbit
Tests
Chores